Patchwork [1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

login
register
mail settings
Submitter Laszlo Ersek
Date March 4, 2013, 10:19 p.m.
Message ID <1362435597-20018-2-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/224849/
State New
Headers show

Comments

Laszlo Ersek - March 4, 2013, 10:19 p.m.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/qapi-schema.json |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-posix.c |   11 +++++++++++
 qga/commands-win32.c |   11 +++++++++++
 3 files changed, 72 insertions(+), 0 deletions(-)
Eric Blake - March 5, 2013, 9:08 p.m.
On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---

> +# @guest-set-vcpus:
> +#
> +# Attempt to reconfigure (currently: enable/disable) logical processors inside
> +# the guest.
> +#
> +# The input list is processed node by node in order. In each node @logical-id
> +# is used to look up the guest VCPU, for which @online specifies the requested
> +# state. The set of distinct @logical-id's is only required to be a subset of
> +# guest-supported identifiers. There's no restriction on list length or on
> +# repeating the same @logical-id (with possibly different @online field).
> +# Preferably the input list should describe a modified subset of
> +# @guest-get-vcpus' return value.
> +#
> +# If part or whole of the requested operation can't be carried out, the guest
> +# VCPU state will be unspecified.

Completely unspecified?  Or is it guaranteed that a subsequent
successful guest-get-vcpus will still be reliably to learn after the
fact what happened?  Would it make any more sense to have only a
guest-set-vcpu, which attempts to set the state of a single vcpu,
instead of an open-ended array of successive vcpu modifications in
guest-set-vcpus?

The interface seems relatively sane, though, and it looks like something
that libvirt would be able to use without having to add any new APIs
(just a new flag value to the existing virDomainSetVcpusFlags() function).
Laszlo Ersek - March 5, 2013, 11:05 p.m.
On 03/05/13 22:08, Eric Blake wrote:
> On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
> 
>> +# @guest-set-vcpus:
>> +#
>> +# Attempt to reconfigure (currently: enable/disable) logical processors inside
>> +# the guest.
>> +#
>> +# The input list is processed node by node in order. In each node @logical-id
>> +# is used to look up the guest VCPU, for which @online specifies the requested
>> +# state. The set of distinct @logical-id's is only required to be a subset of
>> +# guest-supported identifiers. There's no restriction on list length or on
>> +# repeating the same @logical-id (with possibly different @online field).
>> +# Preferably the input list should describe a modified subset of
>> +# @guest-get-vcpus' return value.
>> +#
>> +# If part or whole of the requested operation can't be carried out, the guest
>> +# VCPU state will be unspecified.
> 
> Completely unspecified?

Yes. "Unspecified" means "valid" (ie. at least one VCPU will be online,
the guest won't be "dead"), but no further info will be returned at once.

> Or is it guaranteed that a subsequent
> successful guest-get-vcpus will still be reliably to learn after the
> fact what happened?

Yes, that is both the intent and implied by "unspecified" (as opposed to
"undefined").

> Would it make any more sense to have only a
> guest-set-vcpu, which attempts to set the state of a single vcpu,
> instead of an open-ended array of successive vcpu modifications in
> guest-set-vcpus?

The current interface can be special-cased into that type of call,
however I wanted to provide a batch interface (flipping 100 VCPUs
shouldn't take 100 round trips).

> The interface seems relatively sane, though, and it looks like something
> that libvirt would be able to use without having to add any new APIs
> (just a new flag value to the existing virDomainSetVcpusFlags() function).

Oh.

virDomainSetVcpusFlags() [src/libvirt.c]
  qemuDomainSetVcpusFlags() [src/qemu/qemu_driver.c]
    qemuDomainHotplugVcpus()
      qemuMonitorSetCPU() [src/qemu/qemu_monitor.c]
        qemuMonitorTextSetCPU()
          "cpu_set %d %s"

Does this work? I can't find any trace of the "cpu_set" (or the
"set_cpu") monitor command in upstream qemu.

The relevant libvirt commits are:
- e8d6c289 Support VCPU hotplug in QEMU guests
  ("NB, currently untested since QEMU segvs when running this!")
- a980d123 Fix CPU hotplug command names

If this works and I'm just not seeing something then I have no reason to
pursue this series.

... Ah I understand now. "cpu_set" *is* supported by the qemu-kvm
project at <git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git> -- and
by RHEL-6 qemu-kvm --, via ACPI.

I'll have to test this in RHEL-6. If it doesn't work, I should check
why. If it does, I'll have to figure out if I should continue to work on
this.

I wonder why <git://git.qemu.org/qemu.git> doesn't have "cpu_set".

Thanks
Laszlo
Eric Blake - March 5, 2013, 11:12 p.m.
On 03/05/2013 04:05 PM, Laszlo Ersek wrote:

>> The interface seems relatively sane, though, and it looks like something
>> that libvirt would be able to use without having to add any new APIs
>> (just a new flag value to the existing virDomainSetVcpusFlags() function).
> 
> Oh.
> 
> virDomainSetVcpusFlags() [src/libvirt.c]
>   qemuDomainSetVcpusFlags() [src/qemu/qemu_driver.c]
>     qemuDomainHotplugVcpus()
>       qemuMonitorSetCPU() [src/qemu/qemu_monitor.c]
>         qemuMonitorTextSetCPU()
>           "cpu_set %d %s"
> 
> Does this work? I can't find any trace of the "cpu_set" (or the
> "set_cpu") monitor command in upstream qemu.
> 

The old cpu_set HMP command "worked" in something like qemu 0.10, and
was ripped out when we realized it didn't actually work in a way that
was guaranteed to be safe to the guest.  Since then, the libvirt command
has been a guaranteed failure on qemu, although it continues to work on
xen (and since it has been several YEARS now of not working, people are
laughing at qemu for not getting cpu hotplug working when xen has had it
for so long).

Basically, libvirt would add a new flag that requests using the guest
agent command instead of the monitor command (supposing that we ever do
get around to having a working monitor command that uses ACPI cpu hot
unplug).

> The relevant libvirt commits are:
> - e8d6c289 Support VCPU hotplug in QEMU guests
>   ("NB, currently untested since QEMU segvs when running this!")
> - a980d123 Fix CPU hotplug command names
> 
> If this works and I'm just not seeing something then I have no reason to
> pursue this series.

No, it doesn't work because the HMP command was (intentionally) removed
several years ago when it was determined to be broken.

> 
> ... Ah I understand now. "cpu_set" *is* supported by the qemu-kvm
> project at <git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git> -- and
> by RHEL-6 qemu-kvm --, via ACPI.

Except that the ACPI approach didn't quite work, so qemu-kvm doesn't
expose that right now.

> 
> I'll have to test this in RHEL-6. If it doesn't work, I should check
> why. If it does, I'll have to figure out if I should continue to work on
> this.

Yes, PLEASE continue to work on this - having the guest agent as an
alternative to ACPI has proven useful in other respects (for example, we
wired up virDomainShutdownFlags() to let the user choose between
guest-agent, ACPI, or hypervisor choice).

> 
> I wonder why <git://git.qemu.org/qemu.git> doesn't have "cpu_set".

Because getting ACPI hotplug to work correctly has been harder than
anyone anticipated.
Laszlo Ersek - March 5, 2013, 11:32 p.m.
On 03/06/13 00:12, Eric Blake wrote:

> The old cpu_set HMP command "worked" in something like qemu 0.10, and
> was ripped out when we realized it didn't actually work in a way that
> was guaranteed to be safe to the guest.  Since then, the libvirt command
> has been a guaranteed failure on qemu, although it continues to work on
> xen (and since it has been several YEARS now of not working, people are
> laughing at qemu for not getting cpu hotplug working when xen has had it
> for so long).

Under xen there's a separate comms method for requesting this (dom0 side
massaging of a specific node in xenstore + xenstore watch in the guest
kernel on that node).

http://wiki.xen.org/wiki/XenBus
http://wiki.xen.org/wiki/Event_Channel_Internals

>> I'll have to test this in RHEL-6. If it doesn't work, I should check
>> why. If it does, I'll have to figure out if I should continue to work on
>> this.
> 
> Yes, PLEASE continue to work on this - having the guest agent as an
> alternative to ACPI has proven useful in other respects (for example, we
> wired up virDomainShutdownFlags() to let the user choose between
> guest-agent, ACPI, or hypervisor choice).

OK.

Laszlo
Andrew Jones - March 6, 2013, 7:40 a.m.
----- Original Message -----
> On 03/05/13 22:08, Eric Blake wrote:
> > On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> > 
> >> +# @guest-set-vcpus:
> >> +#
> >> +# Attempt to reconfigure (currently: enable/disable) logical
> >> processors inside
> >> +# the guest.
> >> +#
> >> +# The input list is processed node by node in order. In each node
> >> @logical-id
> >> +# is used to look up the guest VCPU, for which @online specifies
> >> the requested
> >> +# state. The set of distinct @logical-id's is only required to be
> >> a subset of
> >> +# guest-supported identifiers. There's no restriction on list
> >> length or on
> >> +# repeating the same @logical-id (with possibly different @online
> >> field).
> >> +# Preferably the input list should describe a modified subset of
> >> +# @guest-get-vcpus' return value.
> >> +#
> >> +# If part or whole of the requested operation can't be carried
> >> out, the guest
> >> +# VCPU state will be unspecified.
> > 
> > Completely unspecified?
> 
> Yes. "Unspecified" means "valid" (ie. at least one VCPU will be
> online,
> the guest won't be "dead"), but no further info will be returned at
> once.
> 
> > Or is it guaranteed that a subsequent
> > successful guest-get-vcpus will still be reliably to learn after
> > the
> > fact what happened?
> 
> Yes, that is both the intent and implied by "unspecified" (as opposed
> to
> "undefined").
> 
> > Would it make any more sense to have only a
> > guest-set-vcpu, which attempts to set the state of a single vcpu,
> > instead of an open-ended array of successive vcpu modifications in
> > guest-set-vcpus?
> 
> The current interface can be special-cased into that type of call,
> however I wanted to provide a batch interface (flipping 100 VCPUs
> shouldn't take 100 round trips).
> 
> > The interface seems relatively sane, though, and it looks like
> > something
> > that libvirt would be able to use without having to add any new
> > APIs
> > (just a new flag value to the existing virDomainSetVcpusFlags()
> > function).
> 
> Oh.
> 
> virDomainSetVcpusFlags() [src/libvirt.c]
>   qemuDomainSetVcpusFlags() [src/qemu/qemu_driver.c]
>     qemuDomainHotplugVcpus()
>       qemuMonitorSetCPU() [src/qemu/qemu_monitor.c]
>         qemuMonitorTextSetCPU()
>           "cpu_set %d %s"
> 
> Does this work? I can't find any trace of the "cpu_set" (or the
> "set_cpu") monitor command in upstream qemu.
> 
> The relevant libvirt commits are:
> - e8d6c289 Support VCPU hotplug in QEMU guests
>   ("NB, currently untested since QEMU segvs when running this!")
> - a980d123 Fix CPU hotplug command names
> 
> If this works and I'm just not seeing something then I have no reason
> to
> pursue this series.
> 
> ... Ah I understand now. "cpu_set" *is* supported by the qemu-kvm
> project at <git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git> --
> and
> by RHEL-6 qemu-kvm --, via ACPI.
> 
> I'll have to test this in RHEL-6. If it doesn't work, I should check
> why. If it does, I'll have to figure out if I should continue to work
> on
> this.

cpu hotplug works for rhel6 and Igor is also pushing it to upstream
qemu now. But unplug doesn't. We need this alternative solution to
support both plug and unplug while unplug gets worked out.

> 
> I wonder why <git://git.qemu.org/qemu.git> doesn't have "cpu_set".
> 
> Thanks
> Laszlo
>
Eric Blake - March 6, 2013, 1:49 p.m.
On 03/05/2013 04:05 PM, Laszlo Ersek wrote:
>>> +# If part or whole of the requested operation can't be carried out, the guest
>>> +# VCPU state will be unspecified.
>>
>> Completely unspecified?
> 
> Yes. "Unspecified" means "valid" (ie. at least one VCPU will be online,
> the guest won't be "dead"), but no further info will be returned at once.

Hmm, just thinking aloud here (not saying we need to swap interfaces,
unless you like this alternative):

What if we have guest-set-vcpus return a non-negative integer on
success; namely, the number of consecutive array actions that were
completed, and guarantee successful exit on first failure if any prior
element was acted on?  Passing an empty array, or failing on the first
array element, would give an error; otherwise, the error is lost if a
user batches commands, but they would know how much of the batch failed,
and can retry the command with the failing entry first to see what the
failure was (assuming the failure is reproducible).  Basically, this
would make guest-set-vcpus do partial write detection somewhat like write().
Laszlo Ersek - March 6, 2013, 4:37 p.m.
On 03/06/13 14:49, Eric Blake wrote:
> On 03/05/2013 04:05 PM, Laszlo Ersek wrote:
>>>> +# If part or whole of the requested operation can't be carried out, the guest
>>>> +# VCPU state will be unspecified.
>>>
>>> Completely unspecified?
>>
>> Yes. "Unspecified" means "valid" (ie. at least one VCPU will be online,
>> the guest won't be "dead"), but no further info will be returned at once.
> 
> Hmm, just thinking aloud here (not saying we need to swap interfaces,
> unless you like this alternative):
> 
> What if we have guest-set-vcpus return a non-negative integer on
> success; namely, the number of consecutive array actions that were
> completed, and guarantee successful exit on first failure if any prior
> element was acted on?  Passing an empty array, or failing on the first
> array element, would give an error; otherwise, the error is lost if a
> user batches commands, but they would know how much of the batch failed,
> and can retry the command with the failing entry first to see what the
> failure was (assuming the failure is reproducible).  Basically, this
> would make guest-set-vcpus do partial write detection somewhat like write().

You can sell me anything POSIX :)

Thanks!
Laszlo

Patch

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index d91d903..1bf5952 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -515,3 +515,53 @@ 
 ##
 { 'command': 'guest-network-get-interfaces',
   'returns': ['GuestNetworkInterface'] }
+
+##
+# @GuestLogicalProcessor:
+#
+# @logical-id: Arbitrary guest-specific unique identifier of the VCPU.
+#
+# @online: Whether the VCPU is enabled.
+#
+# Since: 1.5
+##
+{ 'type': 'GuestLogicalProcessor',
+  'data': {'logical-id': 'int',
+           'online': 'bool'} }
+
+##
+# @guest-get-vcpus:
+#
+# Retrieve the list of the guest's logical processors.
+#
+# This is a read-only operation.
+#
+# Returns: The list of all VCPUs the guest knows about. Each VCPU is put on the
+# list exactly once, but their order is unspecified.
+#
+# Since: 1.5
+##
+{ 'command': 'guest-get-vcpus',
+  'returns': ['GuestLogicalProcessor'] }
+
+##
+# @guest-set-vcpus:
+#
+# Attempt to reconfigure (currently: enable/disable) logical processors inside
+# the guest.
+#
+# The input list is processed node by node in order. In each node @logical-id
+# is used to look up the guest VCPU, for which @online specifies the requested
+# state. The set of distinct @logical-id's is only required to be a subset of
+# guest-supported identifiers. There's no restriction on list length or on
+# repeating the same @logical-id (with possibly different @online field).
+# Preferably the input list should describe a modified subset of
+# @guest-get-vcpus' return value.
+#
+# If part or whole of the requested operation can't be carried out, the guest
+# VCPU state will be unspecified.
+#
+# Since: 1.5
+##
+{ 'command': 'guest-set-vcpus',
+  'data':    {'vcpus': ['GuestLogicalProcessor'] } }
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7a0202e..1ad231a 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1083,6 +1083,17 @@  void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+void qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+}
+
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..0e83423 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -278,6 +278,17 @@  GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **err)
     return NULL;
 }
 
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+void qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+}
+
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {