diff mbox

[7/8,RFC,v2] s390-qemu: cpu hotplug - Infrastructure for Cpu Devices

Message ID 1370626087-840-8-git-send-email-jjherne@us.ibm.com
State New
Headers show

Commit Message

Jason J. Herne June 7, 2013, 5:28 p.m. UTC 
From: "Jason J. Herne" <jjherne@us.ibm.com>

Add infrastructure for treating cpus as devices. This patch allows cpus to be
specified using a combination of '-smp' and '-device cpu'.  This approach
forces a change in the way cpus are counted via smp_cpus.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 include/hw/boards.h |    3 ++
 vl.c                |   95 +++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 84 insertions(+), 14 deletions(-)

Comments

Andreas Färber June 8, 2013, 10:50 p.m. UTC | #1 
Hi,

Am 07.06.2013 19:28, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Add infrastructure for treating cpus as devices. This patch allows cpus to be
> specified using a combination of '-smp' and '-device cpu'.  This approach
> forces a change in the way cpus are counted via smp_cpus.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  include/hw/boards.h |    3 ++
>  vl.c                |   95 +++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 84 insertions(+), 14 deletions(-)

This feels like an ugly hack. And nack to CPUS_ARE_DEVICES(), since all
CPUs are devices already.

Could you please give a bit more rationale for each change? What's the
intended use case here? This is not just an s390 change but a design
question, so again please CC me as CPU maintainer.

There's a number of questions that your series touches on but doesn't
discuss the concept in the cover letter or in the commit messages:

1) Mixing -smp N and -device s390-cpu

I would expect to get N+1 CPUs. Do you agree or disagree?

-smp 0 is probably not well tested, if at all, so why specify -device
s390-cpu on the command line at all? Of course if there's bugs I'll be
happy to accept fixes, but I'm seeing device_add as more relevant than
-device honestly.

2) QEMUMachine::max_cpus

I believe -device s390-cpu should honor the limit, do you agree?
If so, then there's no need to iterate over -device options, because
that'll miss hot-added devices, but instead move any checks to the CPU's
realize function. If a user creates more CPUs than
qom/cpu.c:cpu_common_realizefn() should be made to fail. Note that my
upcoming CPUState series moves qemu_init_vcpu() there, so we will be
able to bail out before creating vCPU threads etc. for that CPU.

3) vCPU initialization hooks

We already have a QEMUMachine::hot_add_cpu hook. If you need additional
hooks, I'd like to first understand why that cannot go into S390CPU's
realize function, and then we can think about a more generic solution
like adding a Notifier that anyone can use.

Regards,
Andreas
Jason J. Herne June 10, 2013, 4 p.m. UTC | #2 
On 06/08/2013 06:50 PM, Andreas Färber wrote:
> Hi,
>
> Am 07.06.2013 19:28, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Add infrastructure for treating cpus as devices. This patch allows cpus to be
>> specified using a combination of '-smp' and '-device cpu'.  This approach
>> forces a change in the way cpus are counted via smp_cpus.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>> ---
>>   include/hw/boards.h |    3 ++
>>   vl.c                |   95 +++++++++++++++++++++++++++++++++++++++++++--------
>>   2 files changed, 84 insertions(+), 14 deletions(-)
>
> This feels like an ugly hack. And nack to CPUS_ARE_DEVICES(), since all
> CPUs are devices already.
>
> Could you please give a bit more rationale for each change? What's the
> intended use case here? This is not just an s390 change but a design
> question, so again please CC me as CPU maintainer.

As per upstream request from the last S390 cpu hotplug submission, the 
end goal here is cpu hotplug capability using the QOM infrastructure.
This means  no new "cpu_add" command. Instead make hotplug work through 
device_add.  All cpu creation/initialization details should be handled 
through QOM object construction routines.

>
> There's a number of questions that your series touches on but doesn't
> discuss the concept in the cover letter or in the commit messages:
>
> 1) Mixing -smp N and -device s390-cpu
>
> I would expect to get N+1 CPUs. Do you agree or disagree?
>

I agree. This patch has the desired outcome here.

> -smp 0 is probably not well tested, if at all, so why specify -device
> s390-cpu on the command line at all? Of course if there's bugs I'll be
> happy to accept fixes, but I'm seeing device_add as more relevant than
> -device honestly.
>

I agree here as well.  but considering the parsing of the command line 
-device and the device_add qmp/hmp command are common I see no easy way 
of enabling hotpluging via device_add bit rejecting the -device command 
line form.  I suppose we could explicitly check for "-device s390-cpu" 
in main() and fail with an error message. But then we need to add a new 
one for each architecture?

> 2) QEMUMachine::max_cpus
>
> I believe -device s390-cpu should honor the limit, do you agree?
> If so, then there's no need to iterate over -device options, because
> that'll miss hot-added devices, but instead move any checks to the CPU's
> realize function. If a user creates more CPUs than
> qom/cpu.c:cpu_common_realizefn() should be made to fail. Note that my
> upcoming CPUState series moves qemu_init_vcpu() there, so we will be
> able to bail out before creating vCPU threads etc. for that CPU.
>

I agree.  -device should honor max_cpus.  This patch does exactly that. 
  The check during a cpu hotplug happens in qdev_device_add.  I was not 
aware that this could be done in qom/cpu.c:cpu_common_realizefn(). 
Sounds like a good place to put it if we can effectively use that one 
check to remove both the vl.c check and the qdev_device_add.  I'll 
investigate that.

> 3) vCPU initialization hooks
>
> We already have a QEMUMachine::hot_add_cpu hook. If you need additional
> hooks, I'd like to first understand why that cannot go into S390CPU's
> realize function, and then we can think about a more generic solution
> like adding a Notifier that anyone can use.

You are again talking about the new post_cpu_init() hook I added?  If 
so, I've addressed that concern in my reply to your last set of 
comments.  If not, can you please elaborate?

The hot_add_cpu hook is called by qmp_cpu_add.  I would need to call it 
from qmp_device_add.  In which case, I still have no indication if we're 
truly hot adding or if we're still in pre-boot guest initialization.  Is 
there a way to tell?

I guess we should step back and look at the "end goal" here.  I was 
attempting to make the cpu initialization and hot plug as similar as 
possible.  It seems as though that is required if we're going to be 
using the same basic code paths (through QOM object creation routines) 
for both cases.  Am I way off track here? Should this not be my goal?

>
> Regards,
> Andreas
>
Igor Mammedov July 30, 2013, 7:59 a.m. UTC | #3 
On Fri,  7 Jun 2013 13:28:06 -0400
"Jason J. Herne" <jjherne@us.ibm.com> wrote:

> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Add infrastructure for treating cpus as devices. This patch allows cpus to be
> specified using a combination of '-smp' and '-device cpu'.  This approach
> forces a change in the way cpus are counted via smp_cpus.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  include/hw/boards.h |    3 ++
>  vl.c                |   95 +++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 84 insertions(+), 14 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index ed427a1..b0c86bf 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -47,8 +47,11 @@ typedef struct QEMUMachine {
>      GlobalProperty *compat_props;
>      struct QEMUMachine *next;
>      const char *hw_version;
> +    const char *cpu_device_str;
>  } QEMUMachine;
>  
> +#define CPUS_ARE_DEVICES(qemu_mach)    (qemu_mach->cpu_device_str != NULL)
> +
>  int qemu_register_machine(QEMUMachine *m);
>  QEMUMachine *find_default_machine(void);
>  
> diff --git a/vl.c b/vl.c
> index 71e1e6d..873834f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -546,6 +546,46 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
>      return 0;
>  }
>  
> +static void convert_smp_to_cpu_devices(QEMUMachine *machine)
> +{
> +    int i;
> +    QemuOpts *opts;
> +
> +    for (i = 0; i < smp_cpus; i++) {
> +        opts = qemu_opts_create_nofail(qemu_find_opts("device"));
> +        qemu_opt_set(opts, "driver", machine->cpu_device_str);
                                                 ^^ could be default_cpu
and probably stored in .default_machine_opts?

> +    }
> +    smp_cpus = 0;
> +}
> +
> +static int count_cpu_devices(QemuOpts *opts, void *opaque)
> +{
> +    const char *driver = qemu_opt_get(opts, "driver");
> +    QEMUMachine *machine = (QEMUMachine *)opaque;
> +
> +    /* Skip non-cpu devices*/
> +    if (!driver || strcmp(driver, machine->cpu_device_str) != 0) {
find type by driver name and dynamic cast it to common CPU, if cast is successful
it's CPU device

> +        return 0;
> +    }
> +
> +    smp_cpus += 1;
> +    return 0;
> +}
-smp smp_cpus could be treated as a number of startup cpus including CPUs
specified by -device

if number of "-device CPU" more than smp_cpus QEMU could just print error and die
at init stage asking user to correct command line.

> +static int handle_cpu_device(QemuOpts *opts, void *opaque)
> +{
> +    const char *driver = qemu_opt_get(opts, "driver");
> +    QEMUMachine *machine = (QEMUMachine *)opaque;
> +
> +    /* Skip non-cpu devices*/
> +    if (!driver || strcmp(driver, machine->cpu_device_str) != 0) {
> +        return 0;
> +    }
> +
> +    qdev_device_add(opts);
> +    return 0;
> +}
> +
>  /***********************************************************/
>  /* QEMU state */
>  
> @@ -2318,6 +2358,13 @@ static int device_help_func(QemuOpts *opts, void *opaque)
>  static int device_init_func(QemuOpts *opts, void *opaque)
>  {
>      DeviceState *dev;
> +    const char *driver = qemu_opt_get(opts, "driver");
> +    QEMUMachine *machine = (QEMUMachine *)opaque;
> +
> +    /* Skip cpu devices*/
> +    if (!driver || strcmp(driver, machine->cpu_device_str) == 0) {
> +        return 0;
> +    }
>  
>      dev = qdev_device_add(opts);
>      if (!dev)
> @@ -3630,19 +3677,6 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_smp:
>                  smp_parse(optarg);
> -                if (smp_cpus < 1) {
> -                    fprintf(stderr, "Invalid number of CPUs\n");
> -                    exit(1);
> -                }
> -                if (max_cpus < smp_cpus) {
> -                    fprintf(stderr, "maxcpus must be equal to or greater than "
> -                            "smp\n");
> -                    exit(1);
> -                }
> -                if (max_cpus > 255) {
> -                    fprintf(stderr, "Unsupported number of maxcpus\n");
> -                    exit(1);
> -                }
>                  break;
>  	    case QEMU_OPTION_vnc:
>  #ifdef CONFIG_VNC
> @@ -3965,6 +3999,16 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      /*
> +     * Count cpu devices. Cpu count is determied by adding -device cpu
> +     * statements to the number of cpus specified on the -smp statement.
> +     */
> +    if (CPUS_ARE_DEVICES(machine)) {
> +        convert_smp_to_cpu_devices(machine);
> +        qemu_opts_foreach(qemu_find_opts("device"), count_cpu_devices,
> +                          machine, 0);
> +    }
> +
> +    /*
>       * Default to max_cpus = smp_cpus, in case the user doesn't
>       * specify a max_cpus value.
>       */
> @@ -3979,6 +4023,20 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> +    if (smp_cpus < 1) {
> +        fprintf(stderr, "Invalid number of CPUs\n");
> +        exit(1);
> +    }
> +    if (max_cpus < smp_cpus) {
> +        fprintf(stderr, "maxcpus must be equal to or greater than the number of"
> +                " cpus defined\n");
> +        exit(1);
> +    }
> +    if (max_cpus > 255) {
> +        fprintf(stderr, "Unsupported number of maxcpus\n");
> +        exit(1);
> +    }
> +
>      /*
>       * Get the default machine options from the machine if it is not already
>       * specified either by the configuration file or by the command line.
> @@ -4305,6 +4363,13 @@ int main(int argc, char **argv, char **envp)
>                                   .cpu_model = cpu_model };
>      machine->init(&args);
>  
> +    /* Create cpu devices */
> +    if (CPUS_ARE_DEVICES(machine)) {
> +        smp_cpus = 0;  /* Reset this because each cpu will count itself */
> +        qemu_opts_foreach(qemu_find_opts("device"), handle_cpu_device,
> +                          machine, 0);
> +    }
> +
>      if (machine->post_cpu_init) {
>          machine->post_cpu_init();
>      }
> @@ -4324,8 +4389,10 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      /* init generic devices */
> -    if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)
> +    if (qemu_opts_foreach(qemu_find_opts("device"),
> +                          device_init_func, machine, 1) != 0) {
>          exit(1);
> +    }
>  
>      net_check_clients();
>
diff mbox

Patch

diff --git a/include/hw/boards.h b/include/hw/boards.h
index ed427a1..b0c86bf 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -47,8 +47,11 @@  typedef struct QEMUMachine {
     GlobalProperty *compat_props;
     struct QEMUMachine *next;
     const char *hw_version;
+    const char *cpu_device_str;
 } QEMUMachine;
 
+#define CPUS_ARE_DEVICES(qemu_mach)    (qemu_mach->cpu_device_str != NULL)
+
 int qemu_register_machine(QEMUMachine *m);
 QEMUMachine *find_default_machine(void);
 
diff --git a/vl.c b/vl.c
index 71e1e6d..873834f 100644
--- a/vl.c
+++ b/vl.c
@@ -546,6 +546,46 @@  static int default_driver_check(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+static void convert_smp_to_cpu_devices(QEMUMachine *machine)
+{
+    int i;
+    QemuOpts *opts;
+
+    for (i = 0; i < smp_cpus; i++) {
+        opts = qemu_opts_create_nofail(qemu_find_opts("device"));
+        qemu_opt_set(opts, "driver", machine->cpu_device_str);
+    }
+    smp_cpus = 0;
+}
+
+static int count_cpu_devices(QemuOpts *opts, void *opaque)
+{
+    const char *driver = qemu_opt_get(opts, "driver");
+    QEMUMachine *machine = (QEMUMachine *)opaque;
+
+    /* Skip non-cpu devices*/
+    if (!driver || strcmp(driver, machine->cpu_device_str) != 0) {
+        return 0;
+    }
+
+    smp_cpus += 1;
+    return 0;
+}
+
+static int handle_cpu_device(QemuOpts *opts, void *opaque)
+{
+    const char *driver = qemu_opt_get(opts, "driver");
+    QEMUMachine *machine = (QEMUMachine *)opaque;
+
+    /* Skip non-cpu devices*/
+    if (!driver || strcmp(driver, machine->cpu_device_str) != 0) {
+        return 0;
+    }
+
+    qdev_device_add(opts);
+    return 0;
+}
+
 /***********************************************************/
 /* QEMU state */
 
@@ -2318,6 +2358,13 @@  static int device_help_func(QemuOpts *opts, void *opaque)
 static int device_init_func(QemuOpts *opts, void *opaque)
 {
     DeviceState *dev;
+    const char *driver = qemu_opt_get(opts, "driver");
+    QEMUMachine *machine = (QEMUMachine *)opaque;
+
+    /* Skip cpu devices*/
+    if (!driver || strcmp(driver, machine->cpu_device_str) == 0) {
+        return 0;
+    }
 
     dev = qdev_device_add(opts);
     if (!dev)
@@ -3630,19 +3677,6 @@  int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_smp:
                 smp_parse(optarg);
-                if (smp_cpus < 1) {
-                    fprintf(stderr, "Invalid number of CPUs\n");
-                    exit(1);
-                }
-                if (max_cpus < smp_cpus) {
-                    fprintf(stderr, "maxcpus must be equal to or greater than "
-                            "smp\n");
-                    exit(1);
-                }
-                if (max_cpus > 255) {
-                    fprintf(stderr, "Unsupported number of maxcpus\n");
-                    exit(1);
-                }
                 break;
 	    case QEMU_OPTION_vnc:
 #ifdef CONFIG_VNC
@@ -3965,6 +3999,16 @@  int main(int argc, char **argv, char **envp)
     }
 
     /*
+     * Count cpu devices. Cpu count is determied by adding -device cpu
+     * statements to the number of cpus specified on the -smp statement.
+     */
+    if (CPUS_ARE_DEVICES(machine)) {
+        convert_smp_to_cpu_devices(machine);
+        qemu_opts_foreach(qemu_find_opts("device"), count_cpu_devices,
+                          machine, 0);
+    }
+
+    /*
      * Default to max_cpus = smp_cpus, in case the user doesn't
      * specify a max_cpus value.
      */
@@ -3979,6 +4023,20 @@  int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    if (smp_cpus < 1) {
+        fprintf(stderr, "Invalid number of CPUs\n");
+        exit(1);
+    }
+    if (max_cpus < smp_cpus) {
+        fprintf(stderr, "maxcpus must be equal to or greater than the number of"
+                " cpus defined\n");
+        exit(1);
+    }
+    if (max_cpus > 255) {
+        fprintf(stderr, "Unsupported number of maxcpus\n");
+        exit(1);
+    }
+
     /*
      * Get the default machine options from the machine if it is not already
      * specified either by the configuration file or by the command line.
@@ -4305,6 +4363,13 @@  int main(int argc, char **argv, char **envp)
                                  .cpu_model = cpu_model };
     machine->init(&args);
 
+    /* Create cpu devices */
+    if (CPUS_ARE_DEVICES(machine)) {
+        smp_cpus = 0;  /* Reset this because each cpu will count itself */
+        qemu_opts_foreach(qemu_find_opts("device"), handle_cpu_device,
+                          machine, 0);
+    }
+
     if (machine->post_cpu_init) {
         machine->post_cpu_init();
     }
@@ -4324,8 +4389,10 @@  int main(int argc, char **argv, char **envp)
     }
 
     /* init generic devices */
-    if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)
+    if (qemu_opts_foreach(qemu_find_opts("device"),
+                          device_init_func, machine, 1) != 0) {
         exit(1);
+    }
 
     net_check_clients();