[Qemu-devel] [PATCH 0/3] qga/Linux: online/offline/query VCPUs via guest sysfs

[Qemu-devel] [PATCH 0/3] qga/Linux: online/offline/query VCPUs via guest sysfs

[Laszlo Ersek]

Until the uncomparably harder task of real VCPU hotplug / hot-unplug is
completed, here's a small guest agent series that imitates the same
thing through the sysfs of the Linux guest. We've heard that people
migrating from another VMM might be transitorily interested in this.

Laszlo Ersek (3):
  qga: introduce guest-get-vcpus / guest-set-vcpus with stubs
  qga: implement qmp_guest_get_vcpus() for Linux with sysfs
  qga: implement qmp_guest_set_vcpus() for Linux with sysfs

 qga/qapi-schema.json |   50 +++++++++++++++++
 qga/commands-posix.c |  149 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |   11 ++++
 3 files changed, 210 insertions(+), 0 deletions(-)

[Qemu-devel] [PATCH 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Laszlo Ersek]

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(-)

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)
 {
-- 
1.7.1

[Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1ad231a..d4b6bdc 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -15,6 +15,9 @@
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdio.h>
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
 #include "qapi/qmp/qerror.h"
@@ -1083,9 +1086,93 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
+#if defined(__linux__)
+#define SYSCONF_EXACT(name, err) sysconf_exact((name), #name, (err))
+
+static long sysconf_exact(int name, const char *name_str, Error **err)
+{
+    long ret;
+
+    errno = 0;
+    ret = sysconf(name);
+    if (ret == -1) {
+        if (errno == 0) {
+            error_setg(err, "sysconf(%s): value indefinite", name_str);
+        } else {
+            error_setg_errno(err, errno, "sysconf(%s)", name_str);
+        }
+    }
+    return ret;
+}
+
+/*
+ * Store a VCPU structure under the link, and return the link to store into
+ * at the next time.
+ */
+static GuestLogicalProcessorList **
+append_vcpu(int64_t logical_id, bool online, GuestLogicalProcessorList **link)
+{
+    GuestLogicalProcessor *vcpu;
+    GuestLogicalProcessorList *entry;
+
+    vcpu = g_malloc0(sizeof *vcpu);
+    vcpu->logical_id = logical_id;
+    vcpu->online = online;
+
+    entry = g_malloc0(sizeof *entry);
+    entry->value = vcpu;
+
+    *link = entry;
+    return &entry->next;
+}
+#endif
+
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
+#if defined(__linux__)
+    long current;
+    GuestLogicalProcessorList **link, *head;
+    long sc_max;
+    Error *local_err = NULL;
+
+    current = 0;
+    link = append_vcpu(current++, true, &head);
+
+    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
+    while (local_err == NULL && current < sc_max) {
+        char *buf;
+        FILE *f;
+
+        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
+                              current);
+        f = fopen(buf, "r");
+        if (f == NULL) {
+            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
+        } else {
+            unsigned online;
+
+            if (fscanf(f, "%u", &online) != 1) {
+                error_setg(&local_err, "failed to read or parse \"%s\"", buf);
+            } else {
+                link = append_vcpu(current++, online != 0, link);
+            }
+
+            if (fclose(f) == EOF && local_err == NULL) {
+                error_setg_errno(&local_err, errno, "fclose(\"%s\")", buf);
+            }
+        }
+        g_free(buf);
+    }
+
+    if (local_err == NULL) {
+        return head;
+    }
+
+    qapi_free_GuestLogicalProcessorList(head);
+    error_propagate(errp, local_err);
+#else
     error_set(errp, QERR_UNSUPPORTED);
+#endif
     return NULL;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index d4b6bdc..1848df8 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -40,6 +40,7 @@ extern char **environ;
 #include <arpa/inet.h>
 #include <sys/socket.h>
 #include <net/if.h>
+#include <inttypes.h>
 
 #ifdef FIFREEZE
 #define CONFIG_FSFREEZE
@@ -1178,7 +1179,57 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 
 void qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
 {
+#if defined(__linux__)
+    const GuestLogicalProcessorList *entry;
+    Error *local_err = NULL;
+
+    for (entry = vcpus; local_err == NULL && entry != NULL;
+         entry = entry->next) {
+        const GuestLogicalProcessor *vcpu;
+
+        vcpu = entry->value;
+        if (vcpu->logical_id == 0) {
+            if (!vcpu->online) {
+                error_setg(&local_err,
+                           "unable to offline logical processor #0");
+            }
+        } else {
+            char *buf;
+            FILE *f;
+
+            buf = g_strdup_printf("/sys/devices/system/cpu/cpu%"PRId64
+                                  "/online", vcpu->logical_id);
+            f = fopen(buf, "r+");
+            if (f == NULL) {
+                error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r+\")",
+                                 buf);
+            } else {
+                unsigned online;
+
+                if (fscanf(f, "%u", &online) != 1) {
+                    error_setg(&local_err, "failed to read or parse \"%s\"",
+                               buf);
+                } else if ((online != 0) != vcpu->online) {
+                    errno = 0;
+                    rewind(f);
+                    if (errno != 0 ||
+                        fprintf(f, "%u\n", (unsigned)vcpu->online) < 0) {
+                        error_setg_errno(&local_err, errno,
+                                         "rewind()/fprintf() on \"%s\"", buf);
+                    }
+                }
+
+                if (fclose(f) == EOF && local_err == NULL) {
+                    error_setg_errno(&local_err, errno, "fclose(\"%s\")", buf);
+                }
+            }
+            g_free(buf);
+        }
+    }
+    error_propagate(errp, local_err);
+#else
     error_set(errp, QERR_UNSUPPORTED);
+#endif
 }
 
 /* register init/cleanup routines for stateful command groups */
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[mdroth]

On Mon, Mar 04, 2013 at 11:19:56PM +0100, Laszlo Ersek wrote:
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 87 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1ad231a..d4b6bdc 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -15,6 +15,9 @@
>  #include <sys/types.h>
>  #include <sys/ioctl.h>
>  #include <sys/wait.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <stdio.h>
>  #include "qga/guest-agent-core.h"
>  #include "qga-qmp-commands.h"
>  #include "qapi/qmp/qerror.h"
> @@ -1083,9 +1086,93 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
>  }
>  #endif
> 
> +#if defined(__linux__)

There's a section in commands-posix.c set aside under "linux-specific
implementations" for these, and another underneath for stubs so we can
avoid having too many ifdef's.

> +#define SYSCONF_EXACT(name, err) sysconf_exact((name), #name, (err))
> +
> +static long sysconf_exact(int name, const char *name_str, Error **err)
> +{
> +    long ret;
> +
> +    errno = 0;
> +    ret = sysconf(name);
> +    if (ret == -1) {
> +        if (errno == 0) {
> +            error_setg(err, "sysconf(%s): value indefinite", name_str);
> +        } else {
> +            error_setg_errno(err, errno, "sysconf(%s)", name_str);
> +        }
> +    }
> +    return ret;
> +}
> +
> +/*
> + * Store a VCPU structure under the link, and return the link to store into
> + * at the next time.
> + */
> +static GuestLogicalProcessorList **
> +append_vcpu(int64_t logical_id, bool online, GuestLogicalProcessorList **link)
> +{
> +    GuestLogicalProcessor *vcpu;
> +    GuestLogicalProcessorList *entry;
> +
> +    vcpu = g_malloc0(sizeof *vcpu);
> +    vcpu->logical_id = logical_id;
> +    vcpu->online = online;
> +
> +    entry = g_malloc0(sizeof *entry);
> +    entry->value = vcpu;
> +
> +    *link = entry;
> +    return &entry->next;
> +}
> +#endif
> +
>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  {
> +#if defined(__linux__)
> +    long current;
> +    GuestLogicalProcessorList **link, *head;
> +    long sc_max;
> +    Error *local_err = NULL;
> +
> +    current = 0;
> +    link = append_vcpu(current++, true, &head);
> +
> +    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
> +    while (local_err == NULL && current < sc_max) {
> +        char *buf;
> +        FILE *f;
> +
> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
> +                              current);
> +        f = fopen(buf, "r");
> +        if (f == NULL) {
> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
> +        } else {
> +            unsigned online;
> +
> +            if (fscanf(f, "%u", &online) != 1) {

On Fedora 18 and Ubuntu 12.04 at least there doesn't seem to be per-cpu
values for online/offline/etc, but instead just a 'global' entry at
/sys/devices/system/cpu/{online,offline} that provides a range. This is
what's currently described in
linux/Documentation/ABI/testing/sysfs-devices-system-cpu as well.

Is that file also available on the distro you're testing with? Hopefully
there's a single interfaces we can rely on.

> +                error_setg(&local_err, "failed to read or parse \"%s\"", buf);
> +            } else {
> +                link = append_vcpu(current++, online != 0, link);
> +            }
> +
> +            if (fclose(f) == EOF && local_err == NULL) {
> +                error_setg_errno(&local_err, errno, "fclose(\"%s\")", buf);
> +            }
> +        }
> +        g_free(buf);
> +    }
> +
> +    if (local_err == NULL) {
> +        return head;
> +    }
> +
> +    qapi_free_GuestLogicalProcessorList(head);
> +    error_propagate(errp, local_err);
> +#else
>      error_set(errp, QERR_UNSUPPORTED);
> +#endif
>      return NULL;
>  }
> 
> -- 
> 1.7.1
> 
> 

Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[mdroth]

On Mon, Mar 04, 2013 at 11:19:57PM +0100, Laszlo Ersek wrote:
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index d4b6bdc..1848df8 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -40,6 +40,7 @@ extern char **environ;
>  #include <arpa/inet.h>
>  #include <sys/socket.h>
>  #include <net/if.h>
> +#include <inttypes.h>
> 
>  #ifdef FIFREEZE
>  #define CONFIG_FSFREEZE
> @@ -1178,7 +1179,57 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
> 
>  void qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>  {
> +#if defined(__linux__)
> +    const GuestLogicalProcessorList *entry;
> +    Error *local_err = NULL;
> +
> +    for (entry = vcpus; local_err == NULL && entry != NULL;
> +         entry = entry->next) {
> +        const GuestLogicalProcessor *vcpu;
> +
> +        vcpu = entry->value;
> +        if (vcpu->logical_id == 0) {
> +            if (!vcpu->online) {
> +                error_setg(&local_err,
> +                           "unable to offline logical processor #0");
> +            }
> +        } else {
> +            char *buf;
> +            FILE *f;
> +
> +            buf = g_strdup_printf("/sys/devices/system/cpu/cpu%"PRId64
> +                                  "/online", vcpu->logical_id);
> +            f = fopen(buf, "r+");
> +            if (f == NULL) {
> +                error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r+\")",
> +                                 buf);
> +            } else {
> +                unsigned online;
> +
> +                if (fscanf(f, "%u", &online) != 1) {
> +                    error_setg(&local_err, "failed to read or parse \"%s\"",
> +                               buf);
> +                } else if ((online != 0) != vcpu->online) {
> +                    errno = 0;
> +                    rewind(f);
> +                    if (errno != 0 ||
> +                        fprintf(f, "%u\n", (unsigned)vcpu->online) < 0) {

Similar to comments in patch 2: is this supposed to be similar to the
/sys/devices/system/cpu/{probe,release} entries currently documented?

> +                        error_setg_errno(&local_err, errno,
> +                                         "rewind()/fprintf() on \"%s\"", buf);
> +                    }
> +                }
> +
> +                if (fclose(f) == EOF && local_err == NULL) {
> +                    error_setg_errno(&local_err, errno, "fclose(\"%s\")", buf);
> +                }
> +            }
> +            g_free(buf);
> +        }
> +    }
> +    error_propagate(errp, local_err);
> +#else
>      error_set(errp, QERR_UNSUPPORTED);
> +#endif
>  }
> 
>  /* register init/cleanup routines for stateful command groups */
> -- 
> 1.7.1
> 

Re: [Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

On 03/05/2013 01:03 PM, mdroth wrote:

>> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
>> +                              current);
>> +        f = fopen(buf, "r");
>> +        if (f == NULL) {
>> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
>> +        } else {
>> +            unsigned online;
>> +
>> +            if (fscanf(f, "%u", &online) != 1) {
> 
> On Fedora 18 and Ubuntu 12.04 at least there doesn't seem to be per-cpu
> values for online/offline/etc, but instead just a 'global' entry at
> /sys/devices/system/cpu/{online,offline} that provides a range. This is
> what's currently described in
> linux/Documentation/ABI/testing/sysfs-devices-system-cpu as well.

Actually, there is both.  Here's what I have on my 2-cpu laptop, running
Fedora 18:

# find /sys/devices/system/cpu/ -name online
/sys/devices/system/cpu/cpu1/online
/sys/devices/system/cpu/online

Notice that there is NO /sys/devices/system/cpu/cpu0/online, because
this particular laptop's chipset requires that cpu0 ALWAYS be online.
The per-cpu online file exists only for cpus that can safely be
offlined; if it does not exist, then you must leave that cpu on.

> 
> Is that file also available on the distro you're testing with? Hopefully
> there's a single interfaces we can rely on.

Libvirt also relies on the per-cpu online files, and hasn't had any
complaints across the distros.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  {
> +#if defined(__linux__)

> +
> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
> +                              current);
> +        f = fopen(buf, "r");
> +        if (f == NULL) {
> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);

NACK to this portion.  If the file doesn't exist, but the
/sys/devices/system/cpu/cpu%ld/ directory exists, then the cpu should be
treated as always online, and not an error.  In fact, on many machines,
cpu0 does not have an online file precisely because it cannot be taken
offline, even if the rest of the cpus can.  It is also the case that on
older kernels that did not support offline cpus (such as RHEL 5), there
will be no per-cpu online file; but again, such kernels cannot support
hot unplug, so all per-cpu directories imply which cpus are online.  In
other words, you need a sane fallback if the online file does not exist
but the per-cpu directory does exist.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[mdroth]

On Tue, Mar 05, 2013 at 01:22:03PM -0700, Eric Blake wrote:
> On 03/05/2013 01:03 PM, mdroth wrote:
> 
> >> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
> >> +                              current);
> >> +        f = fopen(buf, "r");
> >> +        if (f == NULL) {
> >> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
> >> +        } else {
> >> +            unsigned online;
> >> +
> >> +            if (fscanf(f, "%u", &online) != 1) {
> > 
> > On Fedora 18 and Ubuntu 12.04 at least there doesn't seem to be per-cpu
> > values for online/offline/etc, but instead just a 'global' entry at
> > /sys/devices/system/cpu/{online,offline} that provides a range. This is
> > what's currently described in
> > linux/Documentation/ABI/testing/sysfs-devices-system-cpu as well.
> 
> Actually, there is both.  Here's what I have on my 2-cpu laptop, running
> Fedora 18:
> 
> # find /sys/devices/system/cpu/ -name online
> /sys/devices/system/cpu/cpu1/online
> /sys/devices/system/cpu/online
> 
> Notice that there is NO /sys/devices/system/cpu/cpu0/online, because

Ahh, didn't think to check the others. This seems to be the case for me
as well. I agree on your later note about special casing this though.

> this particular laptop's chipset requires that cpu0 ALWAYS be online.
> The per-cpu online file exists only for cpus that can safely be
> offlined; if it does not exist, then you must leave that cpu on.
> 
> > 
> > Is that file also available on the distro you're testing with? Hopefully
> > there's a single interfaces we can rely on.
> 
> Libvirt also relies on the per-cpu online files, and hasn't had any
> complaints across the distros.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Laszlo Ersek]

On 03/05/13 21:03, mdroth wrote:> On Mon, Mar 04, 2013 at 11:19:56PM +0100, Laszlo Ersek wrote:

>> +#if defined(__linux__)
>
> There's a section in commands-posix.c set aside under "linux-specific
> implementations" for these, and another underneath for stubs so we can
> avoid having too many ifdef's.

I'll check it, thanks.

>> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
>> +                              current);
>> +        f = fopen(buf, "r");
>> +        if (f == NULL) {
>> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
>> +        } else {
>> +            unsigned online;
>> +
>> +            if (fscanf(f, "%u", &online) != 1) {
>
> On Fedora 18 and Ubuntu 12.04 at least there doesn't seem to be per-cpu
> values for online/offline/etc, but instead just a 'global' entry at
> /sys/devices/system/cpu/{online,offline} that provides a range. This is
> what's currently described in
> linux/Documentation/ABI/testing/sysfs-devices-system-cpu as well.
>
> Is that file also available on the distro you're testing with? Hopefully
> there's a single interfaces we can rely on.

On RHEL-6, both "/sys/devices/system/cpu/cpu*/online" and
"/sys/devices/system/cpu/online " are available.

On RHEL-5, only "/sys/devices/system/cpu/cpu*/online" is available.

"/sys/devices/system/cpu/cpu*/online" is documented in
"Documentation/cpu-hotplug.txt", in all of RHEL-5, RHEL-6, and upstream
Linux.

            #pwd
            #/sys/devices/system/cpu
            #ls -l
            total 0
            drwxr-xr-x  10 root root 0 Sep 19 07:44 .
            drwxr-xr-x  13 root root 0 Sep 19 07:45 ..
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu0
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu1
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu2
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu3
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu4
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu5
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu6
            drwxr-xr-x   3 root root 0 Sep 19 07:48 cpu7

    Under each directory you would find an "online" file which is the control
    file to logically online/offline a processor.

    Q: Does hot-add/hot-remove refer to physical add/remove of cpus?
    A: The usage of hot-add/remove may not be very consistently used in the code.
    CONFIG_HOTPLUG_CPU enables logical online/offline capability in the kernel.
    To support physical addition/removal, one would need some BIOS hooks and
    the platform should have something like an attention button in PCI hotplug.
    CONFIG_ACPI_HOTPLUG_CPU enables ACPI support for physical add/remove of CPUs.

The kernels you mention may not have CONFIG_HOTPLUG_CPU enabled
(consequently they would probably not support the functionality either).

... I just checked "/boot/config-3.8.2-201.fc18.x86_64" in
"kernel-3.8.2-201.fc18.x86_64.rpm" and all required config options seem
to be set. I checked on a much older F18 guest as well
(3.6.10-4.fc18.x86_64), and the per-cpu "online" files are there. (Not
for cpu0, but I'll address that in response to Eric's review.) I'm not
sure why you don't get them under F18.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

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).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Laszlo Ersek]

On 03/05/13 21:09, mdroth wrote:

> Similar to comments in patch 2: is this supposed to be similar to the
> /sys/devices/system/cpu/{probe,release} entries currently documented?

I don't have those on RHEL-6, and they aren't mentioned in upstream
"Documentation/cpu-hotplug.txt".

"Documentation/ABI/testing/sysfs-devices-system-cpu" says about probe &
release:

    Description:    Dynamic addition and removal of CPU's.  This is not hotplug
                    removal, this is meant complete removal/addition of the CPU
                    from the system.

That's not my purpose here, since that would be probably even more
complex than (or the same as) real ACPI CPU hotplug / hot-unplug.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3771 bytes --]

On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index d4b6bdc..1848df8 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -40,6 +40,7 @@ extern char **environ;
>  #include <arpa/inet.h>
>  #include <sys/socket.h>
>  #include <net/if.h>
> +#include <inttypes.h>
>  
>  #ifdef FIFREEZE
>  #define CONFIG_FSFREEZE
> @@ -1178,7 +1179,57 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  
>  void qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>  {
> +#if defined(__linux__)
> +    const GuestLogicalProcessorList *entry;
> +    Error *local_err = NULL;
> +
> +    for (entry = vcpus; local_err == NULL && entry != NULL;
> +         entry = entry->next) {
> +        const GuestLogicalProcessor *vcpu;
> +
> +        vcpu = entry->value;
> +        if (vcpu->logical_id == 0) {
> +            if (!vcpu->online) {
> +                error_setg(&local_err,
> +                           "unable to offline logical processor #0");
> +            }

This is not quite accurate; my understanding is that there are setups
where cpu0 can be offlined.  More accurate would be to reject attempts
to offline ANY cpu where the cpu/cpuNN/online file does not exist (which
will catch the fact that cpu0 must always be on for the hardware you are
testing with).

> +        } else {
> +            char *buf;
> +            FILE *f;
> +
> +            buf = g_strdup_printf("/sys/devices/system/cpu/cpu%"PRId64
> +                                  "/online", vcpu->logical_id);
> +            f = fopen(buf, "r+");
> +            if (f == NULL) {
> +                error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r+\")",
> +                                 buf);

Again, if the file doesn't exist, but the user asked for online, then
this should not be an error.

> +            } else {
> +                unsigned online;
> +
> +                if (fscanf(f, "%u", &online) != 1) {
> +                    error_setg(&local_err, "failed to read or parse \"%s\"",
> +                               buf);

Does doing a scan of the file's existing contents buy us any safety?
Why not just blindly write into the file, instead of first reading it?

> +                } else if ((online != 0) != vcpu->online) {
> +                    errno = 0;
> +                    rewind(f);
> +                    if (errno != 0 ||
> +                        fprintf(f, "%u\n", (unsigned)vcpu->online) < 0) {

Do you really want to be printing NUL or \1?  I though the kernel
interface expected the literal character '0' or '1' (in ascii, \x30 or
\x31).

Why even bother with stdio buffering?  Wouldn't it be simpler to
open()/write() instead of fopen()/fprintf()?

> +                        error_setg_errno(&local_err, errno,
> +                                         "rewind()/fprintf() on \"%s\"", buf);
> +                    }
> +                }
> +
> +                if (fclose(f) == EOF && local_err == NULL) {
> +                    error_setg_errno(&local_err, errno, "fclose(\"%s\")", buf);
> +                }
> +            }
> +            g_free(buf);
> +        }
> +    }
> +    error_propagate(errp, local_err);
> +#else
>      error_set(errp, QERR_UNSUPPORTED);
> +#endif
>  }
>  
>  /* register init/cleanup routines for stateful command groups */
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Laszlo Ersek]

On 03/05/13 21:25, Eric Blake wrote:

> On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>>  {
>> +#if defined(__linux__)
>
>> +
>> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
>> +                              current);
>> +        f = fopen(buf, "r");
>> +        if (f == NULL) {
>> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
>
> NACK to this portion.  If the file doesn't exist, but the
> /sys/devices/system/cpu/cpu%ld/ directory exists, then the cpu should be
> treated as always online, and not an error.  In fact, on many machines,
> cpu0 does not have an online file precisely because it cannot be taken
> offline, even if the rest of the cpus can.

The code always reports cpu0 online (without looking at the sysfs) and
never attempts to remove it (refuses any such requests), but see below.

> It is also the case that on
> older kernels that did not support offline cpus (such as RHEL 5), there
> will be no per-cpu online file; but again, such kernels cannot support
> hot unplug, so all per-cpu directories imply which cpus are online.  In
> other words, you need a sane fallback if the online file does not exist
> but the per-cpu directory does exist.

In upstream "Documentation/cpu-hotplug.txt", there's

    Q: Why can't i remove CPU0 on some systems?
    A: Some architectures may have some special dependency on a certain
    CPU.

    For e.g in IA64 platforms we have ability to sent platform
    interrupts to the OS. a.k.a Corrected Platform Error Interrupts
    (CPEI). In current ACPI specifications, we didn't have a way to
    change the target CPU. Hence if the current ACPI version doesn't
    support such re-direction, we disable that CPU by making it
    not-removable.

    In such cases you will also notice that the online file is missing
    under cpu0.

    Q: Is CPU0 removable on X86?
    A: Yes. If kernel is compiled with CONFIG_BOOTPARAM_HOTPLUG_CPU0=y,
    CPU0 is removable by default. Otherwise, CPU0 is also removable by
    kernel option cpu0_hotplug.

    But some features depend on CPU0. Two known dependencies are:

    1. Resume from hibernate/suspend depends on CPU0. Hibernate/suspend
    will fail if CPU0 is offline and you need to online CPU0 before
    hibernate/suspend can continue.
    2. PIC interrupts also depend on CPU0. CPU0 can't be removed if a
    PIC interrupt is detected.

    It's said poweroff/reboot may depend on CPU0 on some machines
    although I haven't seen any poweroff/reboot failure so far after
    CPU0 is offline on a few tested machines.

    Please let me know if you know or see any other dependencies of
    CPU0.

    If the dependencies are under your control, you can turn on CPU0
    hotplug feature either by CONFIG_BOOTPARAM_HOTPLUG_CPU0 or by kernel
    parameter cpu0_hotplug.

    --Fenghua Yu <fenghua.yu@intel.com>

I got this wrong in the series. The get method always reports an online
CPU#0 (without even looking at sysfs), plus 'set' always refuses an
attempt to offline it (even if the guest would support it; ie. 'set' too
omits looking at the sysfs for cpu#0). This matches the common specific
case (exactly cpu#0 is fixed online), but it is wrong in general.

I think I should introduce another boolean flag in the element structure
(only for the get method) that would say whether you can attempt to
offline the VCPU. Then the caller of the set method can either comply or
ignore the hint, and in the latter case it would be smacked down
rightfully.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[mdroth]

On Tue, Mar 05, 2013 at 10:34:08PM +0100, Laszlo Ersek wrote:
> On 03/05/13 21:25, Eric Blake wrote:
> 
> > On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
> >>  {
> >> +#if defined(__linux__)
> >
> >> +
> >> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
> >> +                              current);
> >> +        f = fopen(buf, "r");
> >> +        if (f == NULL) {
> >> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
> >
> > NACK to this portion.  If the file doesn't exist, but the
> > /sys/devices/system/cpu/cpu%ld/ directory exists, then the cpu should be
> > treated as always online, and not an error.  In fact, on many machines,
> > cpu0 does not have an online file precisely because it cannot be taken
> > offline, even if the rest of the cpus can.
> 
> The code always reports cpu0 online (without looking at the sysfs) and
> never attempts to remove it (refuses any such requests), but see below.
> 
> > It is also the case that on
> > older kernels that did not support offline cpus (such as RHEL 5), there
> > will be no per-cpu online file; but again, such kernels cannot support
> > hot unplug, so all per-cpu directories imply which cpus are online.  In
> > other words, you need a sane fallback if the online file does not exist
> > but the per-cpu directory does exist.
> 
> In upstream "Documentation/cpu-hotplug.txt", there's
> 
>     Q: Why can't i remove CPU0 on some systems?
>     A: Some architectures may have some special dependency on a certain
>     CPU.
> 
>     For e.g in IA64 platforms we have ability to sent platform
>     interrupts to the OS. a.k.a Corrected Platform Error Interrupts
>     (CPEI). In current ACPI specifications, we didn't have a way to
>     change the target CPU. Hence if the current ACPI version doesn't
>     support such re-direction, we disable that CPU by making it
>     not-removable.
> 
>     In such cases you will also notice that the online file is missing
>     under cpu0.
> 
>     Q: Is CPU0 removable on X86?
>     A: Yes. If kernel is compiled with CONFIG_BOOTPARAM_HOTPLUG_CPU0=y,
>     CPU0 is removable by default. Otherwise, CPU0 is also removable by
>     kernel option cpu0_hotplug.
> 
>     But some features depend on CPU0. Two known dependencies are:
> 
>     1. Resume from hibernate/suspend depends on CPU0. Hibernate/suspend
>     will fail if CPU0 is offline and you need to online CPU0 before
>     hibernate/suspend can continue.
>     2. PIC interrupts also depend on CPU0. CPU0 can't be removed if a
>     PIC interrupt is detected.
> 
>     It's said poweroff/reboot may depend on CPU0 on some machines
>     although I haven't seen any poweroff/reboot failure so far after
>     CPU0 is offline on a few tested machines.
> 
>     Please let me know if you know or see any other dependencies of
>     CPU0.
> 
>     If the dependencies are under your control, you can turn on CPU0
>     hotplug feature either by CONFIG_BOOTPARAM_HOTPLUG_CPU0 or by kernel
>     parameter cpu0_hotplug.
> 
>     --Fenghua Yu <fenghua.yu@intel.com>
> 
> I got this wrong in the series. The get method always reports an online
> CPU#0 (without even looking at sysfs), plus 'set' always refuses an
> attempt to offline it (even if the guest would support it; ie. 'set' too
> omits looking at the sysfs for cpu#0). This matches the common specific
> case (exactly cpu#0 is fixed online), but it is wrong in general.
> 
> I think I should introduce another boolean flag in the element structure
> (only for the get method) that would say whether you can attempt to
> offline the VCPU. Then the caller of the set method can either comply or
> ignore the hint, and in the latter case it would be smacked down
> rightfully.

I think that seems reasonable.

> 
> Thanks!
> Laszlo
> 

Re: [Qemu-devel] [PATCH 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2757 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Laszlo Ersek]

On 03/05/13 22:19, Eric Blake wrote:
> On 03/04/2013 03:19 PM, Laszlo Ersek wrote:

>> +            } else {
>> +                unsigned online;
>> +
>> +                if (fscanf(f, "%u", &online) != 1) {
>> +                    error_setg(&local_err, "failed to read or parse \"%s\"",
>> +                               buf);
> 
> Does doing a scan of the file's existing contents buy us any safety?
> Why not just blindly write into the file, instead of first reading it?

:)

For an already online CPU:

# dd of=/sys/devices/system/cpu/cpu1/online bs=1 count=1 <<<1
dd: writing `/sys/devices/system/cpu/cpu1/online': Invalid argument
[...]

In the kernel,

store_online() [drivers/base/cpu.c]
  cpu_up() [kernel/cpu.c]
    _cpu_up()

	if (cpu_online(cpu) || !cpu_present(cpu)) {
		ret = -EINVAL;
		goto out;
	}

This logic seems to have been present since the origin of the current
Linux repo (1da177e4 "Linux-2.6.12-rc2").

Checking the history tree at
<git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git>, the
logic dates back to the very first committed version of cpu_up():

commit c5e062079a7090891ea5cd1b23a7eab52b156b2a
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Fri Jul 26 01:28:07 2002 -0700

    [PATCH] Hot-plug CPU Boot Changes
...

> 
>> +                } else if ((online != 0) != vcpu->online) {
>> +                    errno = 0;
>> +                    rewind(f);
>> +                    if (errno != 0 ||
>> +                        fprintf(f, "%u\n", (unsigned)vcpu->online) < 0) {
> 
> Do you really want to be printing NUL or \1?  I though the kernel
> interface expected the literal character '0' or '1' (in ascii, \x30 or
> \x31).

I'm using the %u conversion specifier, which turns the unsigned int
argument into decimal string representation. Same as %d for signed int.

Thanks for all review comments!
Laszlo

Re: [Qemu-devel] [PATCH 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

On 03/05/2013 04:23 PM, Laszlo Ersek wrote:
> For an already online CPU:
> 
> # dd of=/sys/devices/system/cpu/cpu1/online bs=1 count=1 <<<1
> dd: writing `/sys/devices/system/cpu/cpu1/online': Invalid argument
> [...]

So we really do have to read existing state to avoid an error when the
user didn't request a state change.  Thanks for the research, and a
convincing answer :)

>>> +                    if (errno != 0 ||
>>> +                        fprintf(f, "%u\n", (unsigned)vcpu->online) < 0) {
>>
>> Do you really want to be printing NUL or \1?  I though the kernel
>> interface expected the literal character '0' or '1' (in ascii, \x30 or
>> \x31).
> 
> I'm using the %u conversion specifier, which turns the unsigned int
> argument into decimal string representation. Same as %d for signed int.

I guess I had in my mind %c instead of %u; still, I can't help but
wonder if fprintf() and buffering is overkill, compared to just doing
something like this:
 write(fd, &"01"[vcpu->online], 1);

(okay, I hope you would favor readability over my compact
representation, but you get the point).  Oh, and I guess I didn't check
whether a trailing newline is essential to the kernel interpreting the
input, so maybe it would have to be more like:

 char buf[2] = { "01"[vcpu->online], '\n' };
 write(fd, buf, 2);

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Laszlo Ersek]

On 03/06/13 00:37, Eric Blake wrote:

> I guess I had in my mind %c instead of %u; still, I can't help but
> wonder if fprintf() and buffering is overkill, compared to just doing
> something like this:
>  write(fd, &"01"[vcpu->online], 1);
> 
> (okay, I hope you would favor readability over my compact
> representation, but you get the point).

I'd be crucified on qemu-devel if I tried to pull off something like the
above :)

> Oh, and I guess I didn't check
> whether a trailing newline is essential to the kernel interpreting the
> input, so maybe it would have to be more like:
> 
>  char buf[2] = { "01"[vcpu->online], '\n' };
>  write(fd, buf, 2);

The newline is probably not important.

Anyway I'd prefer to avoid direct write()s with nbyte > 1 as I'd have to
handle partial transfers, if for nothing else than principle. (IIRC
avoiding that loop was my main motivation for stdio.)

I guess I'll use open(O_DIRECTORY) + openat("online") + read(..., 1) +
pwrite(..., 1, 0). RHEL-5 seems to support all of these (I can't find
O_SEARCH in the manual there, which is why I'll probably omit it from
open()). If people still complain then I can switch from open(directory)
+ openat("online") to lstat(directory) + open("/.../online").

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Andrew Jones]

----- 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
> 

Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Paolo Bonzini]

> On 03/06/13 00:37, Eric Blake wrote:
> 
> > I guess I had in my mind %c instead of %u; still, I can't help but
> > wonder if fprintf() and buffering is overkill, compared to just
> > doing
> > something like this:
> >  write(fd, &"01"[vcpu->online], 1);
> > 
> > (okay, I hope you would favor readability over my compact
> > representation, but you get the point).
> 
> I'd be crucified on qemu-devel if I tried to pull off something like
> the above :)

I think stdio is just fine...

Paolo

Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

On 03/05/2013 05:44 PM, Laszlo Ersek wrote:
> 
> Anyway I'd prefer to avoid direct write()s with nbyte > 1 as I'd have to
> handle partial transfers, if for nothing else than principle. (IIRC
> avoiding that loop was my main motivation for stdio.)

Fair enough - this code is not a hotspot, so stdio is fine.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

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().

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Laszlo Ersek]

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