[libvirt] [PATCH 6/6] qemu, inject-nmi: Implement the driver methods

[Eric Blake]

On 04/12/2011 11:01 PM, Lai Jiangshan wrote:
> +++ b/src/qemu/qemu_driver.c
> @@ -1701,6 +1701,48 @@ static int qemudDomainSetMaxMemory(virDomainPtr dom, unsigned long memory)
>      return qemudDomainSetMemoryFlags(dom, memory, VIR_DOMAIN_MEM_MAXIMUM);
>  }
>  
> +static int qemuDomainInjectNMI(virDomainPtr domain, unsigned int flags)
> +{
> +    struct qemud_driver *driver = domain->conn->privateData;
> +    virDomainObjPtr vm = NULL;
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv;
> +

Right here, you should have a virCheckFlags(0, -1) to enforce that we
don't honor any flags for now.  At which point, you don't need to pass
flags on down to the monitor calls.

> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2513,3 +2513,32 @@ cleanup:
>  
>      return ret;
>  }
> +
> +int qemuMonitorJSONInjectNMI(qemuMonitorPtr mon, unsigned int flags ATTRIBUTE_UNUSED)
> +{

Since neither monitor needed flags this low, you don't have to propogate
it any further than qemu_driver.c's virCheckFlags().

> +    int ret;
> +    virJSONValuePtr cmd;
> +    virJSONValuePtr reply = NULL;
> +
> +    /*
> +     * FIXME: qmp nmi is not supported until qemu-0.16.0,
> +     * use human-monitor-command instead temporary.
> +     *
> +     * FIXME: qemu's nmi command just injects NMI to a specified CPU,
> +     * use "nmi 0" instead temporary.
> +     */
> +    cmd = qemuMonitorJSONMakeCommand("human-monitor-command",
> +                                     "s:command-line", "nmi 0",
> +                                     NULL);

We've already got a preferred form for issuing HMP commands from JSON.
Rather than building up human-monitor-command manually, you should
instead be using qemuMonitorTextInjectNMI; for example, see how
qemuMonitorJSONDriveDel falls back to hmp.  This also covers the case of
a qemu binary that has JSON but not hmp giving a more useful error message.

> +++ b/src/qemu/qemu_monitor_text.c
> @@ -2628,3 +2628,23 @@ int qemuMonitorTextArbitraryCommand(qemuMonitorPtr mon, const char *cmd,
>  
>      return ret;
>  }
> +
> +int qemuMonitorTextInjectNMI(qemuMonitorPtr mon, unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    const char *cmd = "nmi 0";
> +    char *reply = NULL;
> +
> +    /*
> +     * FIXME: qemu's nmi command just injects NMI to a specified CPU,
> +     * use "nmi 0" instead temporary.
> +     */

This bothers me.  Is it possible to inject NMI to a particular CPU in
bare-metal hardware?  If so, then we ought to support that in the API.

I know what Dan said:

>>> +int virDomainSendEventNMI(virDomainPtr domain, unsigned int vcpu)
>> 
>> Your proposal to qemu-devel to add inject-nmi for QMP does not
>> include any CPU index parameter anymore. Instead it will automatically
>> inject the NMI to all present CPUs. This libvirt API would appear to
>> be incompatible with that QMP design.  For Xen, it appears the API
>> also does not allow a CPU index to be given - it just injects to the
>> first CPU AFAICT.
>> 
>> So do we really need to have a 'unsigned int vcpu' parameter in the
>> libvirt API, or can we just leave it out and always inject to
>> CPU==0 for HMP ?
>> 
>> eg simplify to
>> 
>>   int virDomainSendNMI(virDomainPtr domain)

but if there's ever any possibility that qemu might learn how to direct
an NMI to a particular vcpu, I wonder if we should instead have:

enum {
    VIR_DOMAIN_INJECT_NMI_FIRST = 1,
    VIR_DOMAIN_INJECT_NMI_ALL = 2,
}

/**
 * virDomainInjectNMI:
 * @domain: pointer to domain object, or NULL for Domain0
 * @vcpu:   which vcpu to send the NMI to
 * @flags:  the flags for controlling behaviours
 *
 * Send NMI to the guest.  If @flags contains
 * VIR_DOMAIN_INJECT_NMI_FIRST or VIR_DOMAIN_INJECT_NMI_ALL,
 * then @vcpu is ignored, and the NMI is sent to the first
 * possible vcpu or to all vcpus, respectively.  Otherwise,
 * the NMI is sent to the specified vcpu; it is an error if
 * @vcpu does not correspond to a currently online processor.
 *
 * Not all hypervisors support fine-tuned control over which
 * vcpu(s) can be targetted, and might succeed only for a
 * particular value of @flags.
 *
 * Returns 0 in case of success, -1 in case of failure.
 */

int virDomainInjectNMI(virDomainPtr domain, unsigned int vcpu,
   unsigned int flags);

Then xen would be hardcoded to require flags==_FIRST (and always ignore
vcpu), whereas qemu can honor a particular vcpu.

Or is the first vcpu always 0?  That is, are there any hypervisors that
let you offline vcpu 0 while leaving vcpu1 up, so that FIRST would imply
1?  Maybe we don't need a flag for FIRST, but document that vcpu is
ignored if ALL is passed, and then make xen error out if ALL is passed
or if vcpu != 0.

That said, I haven't looked at the proposed qemu side of the patches for
how the monitor command for nmi will be implemented in the first place,
and until that is in a formal qemu release, we may still need to be a
bit flexible here on the libvirt side.  Do any other hypervisors allow
NMI injection, and with what semantics?

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110415/5113c330/attachment-0001.sig>