[Qemu-devel] [PATCH v8 0/7] Allow hotplug of s390 CPUs

[Qemu-devel] [PATCH v8 0/7] Allow hotplug of s390 CPUs

[Matthew Rosato]

Changes from v7->v8:

* Patch 3: Rather than using cpu_index to set cpu_num temporarily, squash 
  in pieces from other patches -- specifically next_cpu_id and move of 
  cpu_exec_init to realizefn (David)
* Patch 4: New patch, splits out toleration of max_cpus (Igor)
* Patch 5: 
     * use hotplug_dev instead of qdev_get_machine (Igor)
     * Add missing g_free(name) (Igor)
* Patch 6:
     * Change s390_new_cpu to take cpu_model parm.  Move to helper.c, call 
       s390_new_cpu from cpu_s390x_init.  (David)
     * s/local_err/err/ (David)
     * Drop getter routine for id property, move some sanity checking from 
       setter to realize (David)
     * Drop unnecessary unref (David)
	
**************

As discussed in the KVM call, we will go ahead with cpu_add for 
s390x to get cpu hotplug functionality in s390x now, until
architectures that require a more robust hotplug interface
settle on a design.

To configure a guest with 2 CPUs online at 
boot and 4 maximum:

qemu -smp 2,maxcpus=4

Or, when using libvirt:
  <domain>
    ...
    <vcpu current="2">4</vcpu>
    ...
  </domain> 


To subsequently hotplug a CPU:

Issue 'cpu-add <id>' from qemu monitor, or use virsh setvcpus --count <n> 
<domain>, where <n> is the total number of desired guest CPUs.

At this point, the guest must bring the CPU online for use -- This can be 
achieved via "echo 1 > /sys/devices/system/cpu/cpuX/online" or via a management 
tool like cpuplugd.

This patch set is based on work previously done by Jason Herne.

Matthew Rosato (7):
  s390x/cpu: Cleanup init in preparation for hotplug
  s390x/cpu: Set initial CPU state in common routine
  s390x/cpu: Get rid of side effects when creating a vcpu
  s390x/cpu: Tolerate max_cpus
  s390x/cpu: Add CPU property links
  s390x/cpu: Add error handling to cpu creation
  s390x/cpu: Allow hotplug of CPUs

 hw/s390x/s390-virtio-ccw.c | 49 ++++++++++++++++++++++++++-
 hw/s390x/s390-virtio.c     | 36 +++++++++++---------
 hw/s390x/s390-virtio.h     |  2 +-
 target-s390x/cpu-qom.h     |  3 ++
 target-s390x/cpu.c         | 83 +++++++++++++++++++++++++++++++++++++++++++---
 target-s390x/cpu.h         |  2 ++
 target-s390x/helper.c      | 42 +++++++++++++++++++++--
 7 files changed, 192 insertions(+), 25 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v8 1/7] s390x/cpu: Cleanup init in preparation for hotplug

[Matthew Rosato]

Ensure a valid cpu_model is set upfront by setting the
default value directly into the MachineState when none is
specified.  This is needed to ensure hotplugged CPUs share
the same cpu_model.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 2 +-
 hw/s390x/s390-virtio.c     | 8 ++++----
 hw/s390x/s390-virtio.h     | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 89f5d0d..b05ed8b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -136,7 +136,7 @@ static void ccw_init(MachineState *machine)
     virtio_ccw_register_hcalls();
 
     /* init CPUs */
-    s390_init_cpus(machine->cpu_model);
+    s390_init_cpus(machine);
 
     if (kvm_enabled()) {
         kvm_s390_enable_css_support(s390_cpu_addr2state(0));
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 8e533ae..d40d0dc 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -93,12 +93,12 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(const char *cpu_model)
+void s390_init_cpus(MachineState *machine)
 {
     int i;
 
-    if (cpu_model == NULL) {
-        cpu_model = "host";
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = "host";
     }
 
     ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
@@ -107,7 +107,7 @@ void s390_init_cpus(const char *cpu_model)
         S390CPU *cpu;
         CPUState *cs;
 
-        cpu = cpu_s390x_init(cpu_model);
+        cpu = cpu_s390x_init(machine->cpu_model);
         cs = CPU(cpu);
 
         ipi_states[i] = cpu;
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index eebce8e..ffd014c 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -19,7 +19,7 @@
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
 
-void s390_init_cpus(const char *cpu_model);
+void s390_init_cpus(MachineState *machine);
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
                        const char *initrd_filename,
-- 
1.9.1

[Qemu-devel] [PATCH v8 2/7] s390x/cpu: Set initial CPU state in common routine

[Matthew Rosato]

Both initial and hotplugged CPUs need to set the same initial
state.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio.c | 4 ----
 target-s390x/cpu.c     | 2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index d40d0dc..c501a48 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -105,14 +105,10 @@ void s390_init_cpus(MachineState *machine)
 
     for (i = 0; i < smp_cpus; i++) {
         S390CPU *cpu;
-        CPUState *cs;
 
         cpu = cpu_s390x_init(machine->cpu_model);
-        cs = CPU(cpu);
 
         ipi_states[i] = cpu;
-        cs->halted = 1;
-        cs->exception_index = EXCP_HLT;
     }
 }
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 73a910d..603c2a1 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -219,6 +219,8 @@ static void s390_cpu_initfn(Object *obj)
 #endif
 
     cs->env_ptr = env;
+    cs->halted = 1;
+    cs->exception_index = EXCP_HLT;
     cpu_exec_init(cs, &error_abort);
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
-- 
1.9.1

[Qemu-devel] [PATCH v8 3/7] s390x/cpu: Get rid of side effects when creating a vcpu

[Matthew Rosato]

In preparation for hotplug, defer some CPU initialization
until the device is actually being realized, including
cpu_exec_init.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 target-s390x/cpu-qom.h |  2 ++
 target-s390x/cpu.c     | 20 +++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index 029a44a..56d82f2 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -47,6 +47,8 @@ typedef struct S390CPUClass {
     CPUClass parent_class;
     /*< public >*/
 
+    int64_t next_cpu_id;
+
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
     void (*load_normal)(CPUState *cpu);
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 603c2a1..76c8eaf 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -195,7 +195,20 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
+    S390CPU *cpu = S390_CPU(dev);
+    CPUS390XState *env = &cpu->env;
+    Error *err = NULL;
+
+    cpu_exec_init(cs, &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
 
+#if !defined(CONFIG_USER_ONLY)
+    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
+#endif
+    env->cpu_num = scc->next_cpu_id++;
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -213,7 +226,6 @@ static void s390_cpu_initfn(Object *obj)
     S390CPU *cpu = S390_CPU(obj);
     CPUS390XState *env = &cpu->env;
     static bool inited;
-    static int cpu_num = 0;
 #if !defined(CONFIG_USER_ONLY)
     struct tm tm;
 #endif
@@ -221,9 +233,7 @@ static void s390_cpu_initfn(Object *obj)
     cs->env_ptr = env;
     cs->halted = 1;
     cs->exception_index = EXCP_HLT;
-    cpu_exec_init(cs, &error_abort);
 #if !defined(CONFIG_USER_ONLY)
-    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
     qemu_get_timedate(&tm, 0);
     env->tod_offset = TOD_UNIX_EPOCH +
                       (time2tod(mktimegm(&tm)) * 1000000000ULL);
@@ -232,7 +242,6 @@ static void s390_cpu_initfn(Object *obj)
     env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
     s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
 #endif
-    env->cpu_num = cpu_num++;
 
     if (tcg_enabled() && !inited) {
         inited = true;
@@ -339,6 +348,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     CPUClass *cc = CPU_CLASS(scc);
     DeviceClass *dc = DEVICE_CLASS(oc);
 
+    scc->next_cpu_id = 0;
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
 
@@ -371,7 +381,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_arch_name = s390_gdb_arch_name;
 
     /*
-     * Reason: s390_cpu_initfn() calls cpu_exec_init(), which saves
+     * Reason: s390_cpu_realizefn() calls cpu_exec_init(), which saves
      * the object in cpus -> dangling pointer after final
      * object_unref().
      */
-- 
1.9.1

Re: [Qemu-devel] [PATCH v8 3/7] s390x/cpu: Get rid of side effects when creating a vcpu

[David Hildenbrand]

> In preparation for hotplug, defer some CPU initialization
> until the device is actually being realized, including
> cpu_exec_init.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

Looks good to me!

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

David

[Qemu-devel] [PATCH v8 4/7] s390x/cpu: Tolerate max_cpus

[Matthew Rosato]

Once hotplug is enabled, interrupts may come in for CPUs
with an address > smp_cpus.  Allocate for this and allow
search routines to look beyond smp_cpus.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index c501a48..90bc58a 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -58,15 +58,16 @@
 #define S390_TOD_CLOCK_VALUE_MISSING    0x00
 #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
 
-static S390CPU **ipi_states;
+static S390CPU **cpu_states;
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
-    if (cpu_addr >= smp_cpus) {
+    if (cpu_addr >= max_cpus) {
         return NULL;
     }
 
-    return ipi_states[cpu_addr];
+    /* Fast lookup via CPU ID */
+    return cpu_states[cpu_addr];
 }
 
 void s390_init_ipl_dev(const char *kernel_filename,
@@ -101,14 +102,14 @@ void s390_init_cpus(MachineState *machine)
         machine->cpu_model = "host";
     }
 
-    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
+    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
 
-    for (i = 0; i < smp_cpus; i++) {
+    for (i = 0; i < max_cpus; i++) {
         S390CPU *cpu;
 
         cpu = cpu_s390x_init(machine->cpu_model);
 
-        ipi_states[i] = cpu;
+        cpu_states[i] = cpu;
     }
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v8 4/7] s390x/cpu: Tolerate max_cpus

[David Hildenbrand]

> Once hotplug is enabled, interrupts may come in for CPUs
> with an address > smp_cpus.  Allocate for this and allow
> search routines to look beyond smp_cpus.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index c501a48..90bc58a 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -58,15 +58,16 @@
>  #define S390_TOD_CLOCK_VALUE_MISSING    0x00
>  #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
> 
> -static S390CPU **ipi_states;
> +static S390CPU **cpu_states;
> 
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> -    if (cpu_addr >= smp_cpus) {
> +    if (cpu_addr >= max_cpus) {
>          return NULL;
>      }
> 
> -    return ipi_states[cpu_addr];
> +    /* Fast lookup via CPU ID */
> +    return cpu_states[cpu_addr];
>  }
> 
>  void s390_init_ipl_dev(const char *kernel_filename,
> @@ -101,14 +102,14 @@ void s390_init_cpus(MachineState *machine)
>          machine->cpu_model = "host";
>      }
> 
> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> +    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
> 
> -    for (i = 0; i < smp_cpus; i++) {
> +    for (i = 0; i < max_cpus; i++) {
>          S390CPU *cpu;
> 
>          cpu = cpu_s390x_init(machine->cpu_model);
> 
> -        ipi_states[i] = cpu;
> +        cpu_states[i] = cpu;

This looks wrong (creating all cpus). But the net patch fixes it again.

Can you make this patch a simple rename patch and move the max_cpu stuff into
the next patch if this makes sense?

Or simply set the cpu_state for everything above smp_cpus to zero in this patch.

Whatever you think makes sense.

>      }
>  }
> 


David

Re: [Qemu-devel] [PATCH v8 4/7] s390x/cpu: Tolerate max_cpus

[Matthew Rosato]

On 03/04/2016 03:05 AM, David Hildenbrand wrote:
>> Once hotplug is enabled, interrupts may come in for CPUs
>> with an address > smp_cpus.  Allocate for this and allow
>> search routines to look beyond smp_cpus.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/s390-virtio.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index c501a48..90bc58a 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -58,15 +58,16 @@
>>  #define S390_TOD_CLOCK_VALUE_MISSING    0x00
>>  #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
>>
>> -static S390CPU **ipi_states;
>> +static S390CPU **cpu_states;
>>
>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>  {
>> -    if (cpu_addr >= smp_cpus) {
>> +    if (cpu_addr >= max_cpus) {
>>          return NULL;
>>      }
>>
>> -    return ipi_states[cpu_addr];
>> +    /* Fast lookup via CPU ID */
>> +    return cpu_states[cpu_addr];
>>  }
>>
>>  void s390_init_ipl_dev(const char *kernel_filename,
>> @@ -101,14 +102,14 @@ void s390_init_cpus(MachineState *machine)
>>          machine->cpu_model = "host";
>>      }
>>
>> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
>> +    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
>>
>> -    for (i = 0; i < smp_cpus; i++) {
>> +    for (i = 0; i < max_cpus; i++) {
>>          S390CPU *cpu;
>>
>>          cpu = cpu_s390x_init(machine->cpu_model);
>>
>> -        ipi_states[i] = cpu;
>> +        cpu_states[i] = cpu;
> 
> This looks wrong (creating all cpus). But the net patch fixes it again.
> 

Ouch.  Definitely wrong, error introduced during patch split.  We
allocate for max_cpus, but should only create smp_cpus cpus during init.

> Can you make this patch a simple rename patch and move the max_cpu stuff into
> the next patch if this makes sense?
> 
> Or simply set the cpu_state for everything above smp_cpus to zero in this patch.

This is done via the gmalloc0.  If I hadn't messed up this loop, the net
result would be the ability to handle > smp_cpus, but nobody will ever
create one (yet).

Matt

[Qemu-devel] [PATCH v8 5/7] s390x/cpu: Add CPU property links

[Matthew Rosato]

Link each CPUState as property machine/cpu[n] during initialization.
Add a hotplug handler to s390-virtio-ccw machine and set the
state during plug.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 34 ++++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio.c     | 15 +++++++++++----
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b05ed8b..7fc1879 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -156,10 +156,41 @@ static void ccw_init(MachineState *machine)
                     gtod_save, gtod_load, kvm_state);
 }
 
+static void s390_cpu_plug(HotplugHandler *hotplug_dev,
+                        DeviceState *dev, Error **errp)
+{
+    gchar *name;
+    S390CPU *cpu = S390_CPU(dev);
+    CPUState *cs = CPU(dev);
+
+    name = g_strdup_printf("cpu[%i]", cpu->env.cpu_num);
+    object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name,
+                             errp);
+    g_free(name);
+}
+
+static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        s390_cpu_plug(hotplug_dev, dev, errp);
+    }
+}
+
+static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
+                                                DeviceState *dev)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+    return NULL;
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
@@ -171,6 +202,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->no_sdcard = 1;
     mc->use_sclp = 1;
     mc->max_cpus = 255;
+    mc->get_hotplug_handler = s390_get_hotplug_handler;
+    hc->plug = s390_machine_device_plug;
     nc->nmi_monitor_handler = s390_nmi;
 }
 
@@ -232,6 +265,7 @@ static const TypeInfo ccw_machine_info = {
     .class_init    = ccw_machine_class_init,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_NMI },
+        { TYPE_HOTPLUG_HANDLER},
         { }
     },
 };
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 90bc58a..f00d6b4 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -97,6 +97,7 @@ void s390_init_ipl_dev(const char *kernel_filename,
 void s390_init_cpus(MachineState *machine)
 {
     int i;
+    gchar *name;
 
     if (machine->cpu_model == NULL) {
         machine->cpu_model = "host";
@@ -105,11 +106,17 @@ void s390_init_cpus(MachineState *machine)
     cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
 
     for (i = 0; i < max_cpus; i++) {
-        S390CPU *cpu;
-
-        cpu = cpu_s390x_init(machine->cpu_model);
+        name = g_strdup_printf("cpu[%i]", i);
+        object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU,
+                                 (Object **) &cpu_states[i],
+                                 object_property_allow_set_link,
+                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                                 &error_abort);
+        g_free(name);
+    }
 
-        cpu_states[i] = cpu;
+    for (i = 0; i < smp_cpus; i++) {
+        cpu_s390x_init(machine->cpu_model);
     }
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v8 5/7] s390x/cpu: Add CPU property links

[David Hildenbrand]

> Link each CPUState as property machine/cpu[n] during initialization.
> Add a hotplug handler to s390-virtio-ccw machine and set the
> state during plug.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

David

[Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Matthew Rosato]

Check for and propogate errors during s390 cpu creation.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio.c |  2 +-
 target-s390x/cpu-qom.h |  1 +
 target-s390x/cpu.c     | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
 target-s390x/cpu.h     |  2 ++
 target-s390x/helper.c  | 42 +++++++++++++++++++++++++++++++++++--
 5 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index f00d6b4..2ab7b94 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -116,7 +116,7 @@ void s390_init_cpus(MachineState *machine)
     }
 
     for (i = 0; i < smp_cpus; i++) {
-        cpu_s390x_init(machine->cpu_model);
+        s390_new_cpu(machine->cpu_model, i, &error_fatal);
     }
 }
 
diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index 56d82f2..1c90933 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -68,6 +68,7 @@ typedef struct S390CPU {
     /*< public >*/
 
     CPUS390XState env;
+    int64_t id;
     /* needed for live migration */
     void *irqstate;
     uint32_t irqstate_saved_size;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 76c8eaf..d1b7af9 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -30,8 +30,10 @@
 #include "qemu/error-report.h"
 #include "hw/hw.h"
 #include "trace.h"
+#include "qapi/visitor.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
+#include "sysemu/sysemu.h"
 #endif
 
 #define CR0_RESET       0xE0UL
@@ -199,16 +201,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUS390XState *env = &cpu->env;
     Error *err = NULL;
 
+#if !defined(CONFIG_USER_ONLY)
+    if (cpu->id >= max_cpus) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", max allowed: %d", cpu->id, max_cpus - 1);
+        return;
+    }
+#endif
+    if (cpu_exists(cpu->id)) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", it already exists", cpu->id);
+        return;
+    }
+    if (cpu->id != scc->next_cpu_id) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", The next available id is %" PRIi64, cpu->id,
+                   scc->next_cpu_id);
+        return;
+    }
+
     cpu_exec_init(cs, &err);
     if (err != NULL) {
         error_propagate(errp, err);
         return;
     }
+    scc->next_cpu_id = cs->cpu_index + 1;
 
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
-    env->cpu_num = scc->next_cpu_id++;
+    env->cpu_num = cpu->id;
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -220,6 +242,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     scc->parent_realize(dev, errp);
 }
 
+static void s390_cpu_set_id(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    S390CPU *cpu = S390_CPU(obj);
+    DeviceState *dev = DEVICE(obj);
+    const int64_t min = 0;
+    const int64_t max = UINT32_MAX;
+    Error *err = NULL;
+    int64_t value;
+
+    if (dev->realized) {
+        error_setg(errp, "Attempt to set property '%s' on '%s' after "
+                   "it was realized", name, object_get_typename(obj));
+        return;
+    }
+
+    visit_type_int(v, name, &value, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    if (value < min || value > max) {
+        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
+                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
+                   object_get_typename(obj), name, value, min, max);
+        return;
+    }
+    cpu->id = value;
+}
+
 static void s390_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -233,6 +285,8 @@ static void s390_cpu_initfn(Object *obj)
     cs->env_ptr = env;
     cs->halted = 1;
     cs->exception_index = EXCP_HLT;
+    object_property_add(OBJECT(cpu), "id", "int64_t", NULL, s390_cpu_set_id,
+                        NULL, NULL, NULL);
 #if !defined(CONFIG_USER_ONLY)
     qemu_get_timedate(&tm, 0);
     env->tod_offset = TOD_UNIX_EPOCH +
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 49c8415..6667cc0 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -413,6 +413,8 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
 #endif
 
 S390CPU *cpu_s390x_init(const char *cpu_model);
+S390CPU *s390_new_cpu(const char *cpu_model, int64_t id, Error **errp);
+S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
 void s390x_translate_init(void);
 int cpu_s390x_exec(CPUState *cpu);
 
diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 838bdd9..c48c816 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -30,6 +30,9 @@
 //#define DEBUG_S390
 //#define DEBUG_S390_STDOUT
 
+/* Use to track cpu ID for linux-user only */
+static int64_t next_cpu_id;
+
 #ifdef DEBUG_S390
 #ifdef DEBUG_S390_STDOUT
 #define DPRINTF(fmt, ...) \
@@ -65,14 +68,49 @@ void s390x_cpu_timer(void *opaque)
 }
 #endif
 
-S390CPU *cpu_s390x_init(const char *cpu_model)
+S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp)
 {
     S390CPU *cpu;
 
     cpu = S390_CPU(object_new(TYPE_S390_CPU));
 
-    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
+    return cpu;
+}
+
+S390CPU *s390_new_cpu(const char *cpu_model, int64_t id, Error **errp)
+{
+    S390CPU *cpu = NULL;
+    Error *err = NULL;
+
+    cpu = cpu_s390x_create(cpu_model, &err);
+    if (err != NULL) {
+        goto out;
+    }
+
+    object_property_set_int(OBJECT(cpu), id, "id", &err);
+    if (err != NULL) {
+        goto out;
+    }
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
 
+out:
+    if (err) {
+        error_propagate(errp, err);
+        object_unref(OBJECT(cpu));
+        cpu = NULL;
+    }
+    return cpu;
+}
+
+S390CPU *cpu_s390x_init(const char *cpu_model)
+{
+    Error *err = NULL;
+    S390CPU *cpu;
+
+    cpu = s390_new_cpu(cpu_model, next_cpu_id++, &err);
+    if (err) {
+        error_report_err(err);
+    }
     return cpu;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio.c |  2 +-
>  target-s390x/cpu-qom.h |  1 +
>  target-s390x/cpu.c     | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-s390x/cpu.h     |  2 ++
>  target-s390x/helper.c  | 42 +++++++++++++++++++++++++++++++++++--
>  5 files changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index f00d6b4..2ab7b94 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -116,7 +116,7 @@ void s390_init_cpus(MachineState *machine)
>      }
> 
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu_s390x_init(machine->cpu_model);
> +        s390_new_cpu(machine->cpu_model, i, &error_fatal);
>      }
>  }
> 
> diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
> index 56d82f2..1c90933 100644
> --- a/target-s390x/cpu-qom.h
> +++ b/target-s390x/cpu-qom.h
> @@ -68,6 +68,7 @@ typedef struct S390CPU {
>      /*< public >*/
> 
>      CPUS390XState env;
> +    int64_t id;
>      /* needed for live migration */
>      void *irqstate;
>      uint32_t irqstate_saved_size;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 76c8eaf..d1b7af9 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -30,8 +30,10 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "trace.h"
> +#include "qapi/visitor.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #endif
> 
>  #define CR0_RESET       0xE0UL
> @@ -199,16 +201,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUS390XState *env = &cpu->env;
>      Error *err = NULL;
> 
> +#if !defined(CONFIG_USER_ONLY)
> +    if (cpu->id >= max_cpus) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", max allowed: %d", cpu->id, max_cpus - 1);

not sure if we should report this into err and then to an error_propgate().

> +        return;
> +    }
> +#endif
> +    if (cpu_exists(cpu->id)) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", it already exists", cpu->id);

dito

> +        return;
> +    }
> +    if (cpu->id != scc->next_cpu_id) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", The next available id is %" PRIi64, cpu->id,
> +                   scc->next_cpu_id);

dito

> +        return;
> +    }
> +
>      cpu_exec_init(cs, &err);
>      if (err != NULL) {
>          error_propagate(errp, err);
>          return;
>      }
> +    scc->next_cpu_id = cs->cpu_index + 1;

Why can't we simply do a scc->next_cpu_id++ as before and leave cs->cpu_index
logic out?

> 
>  #if !defined(CONFIG_USER_ONLY)
>      qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>  #endif
> -    env->cpu_num = scc->next_cpu_id++;
> +    env->cpu_num = cpu->id;
>      s390_cpu_gdb_init(cs);
>      qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> @@ -220,6 +242,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      scc->parent_realize(dev, errp);
>  }
> 
> +static void s390_cpu_set_id(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)

s/s390_/s390x_/ ?

> +{
> +    S390CPU *cpu = S390_CPU(obj);
> +    DeviceState *dev = DEVICE(obj);
> +    const int64_t min = 0;
> +    const int64_t max = UINT32_MAX;
> +    Error *err = NULL;
> +    int64_t value;
> +
> +    if (dev->realized) {
> +        error_setg(errp, "Attempt to set property '%s' on '%s' after "
> +                   "it was realized", name, object_get_typename(obj));
> +        return;
> +    }
> +
> +    visit_type_int(v, name, &value, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    if (value < min || value > max) {
> +        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> +                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> +                   object_get_typename(obj), name, value, min, max);
> +        return;
> +    }
> +    cpu->id = value;
> +}
> +
>  static void s390_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -233,6 +285,8 @@ static void s390_cpu_initfn(Object *obj)
>      cs->env_ptr = env;
>      cs->halted = 1;
>      cs->exception_index = EXCP_HLT;
> +    object_property_add(OBJECT(cpu), "id", "int64_t", NULL, s390_cpu_set_id,
> +                        NULL, NULL, NULL);

Would it be helpful for introspection to also get the id of a cpu?


>  #if !defined(CONFIG_USER_ONLY)
>      qemu_get_timedate(&tm, 0);
>      env->tod_offset = TOD_UNIX_EPOCH +
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 49c8415..6667cc0 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -413,6 +413,8 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
>  #endif
> 
>  S390CPU *cpu_s390x_init(const char *cpu_model);
> +S390CPU *s390_new_cpu(const char *cpu_model, int64_t id, Error **errp);
> +S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
>  void s390x_translate_init(void);
>  int cpu_s390x_exec(CPUState *cpu);
> 
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index 838bdd9..c48c816 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -30,6 +30,9 @@
>  //#define DEBUG_S390
>  //#define DEBUG_S390_STDOUT
> 
> +/* Use to track cpu ID for linux-user only */
> +static int64_t next_cpu_id;

I think be best use this into the function cpu_s390x_init() then, as this is
only needed to keep linux-user working.

> +
>  #ifdef DEBUG_S390
>  #ifdef DEBUG_S390_STDOUT
>  #define DPRINTF(fmt, ...) \
> @@ -65,14 +68,49 @@ void s390x_cpu_timer(void *opaque)
>  }
>  #endif
> 
> -S390CPU *cpu_s390x_init(const char *cpu_model)
> +S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp)
>  {
>      S390CPU *cpu;
> 
>      cpu = S390_CPU(object_new(TYPE_S390_CPU));
> 
> -    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
> +    return cpu;
> +}
> +
> +S390CPU *s390_new_cpu(const char *cpu_model, int64_t id, Error **errp)

s/s390_/s390x_/ ?

> +{
> +    S390CPU *cpu = NULL;

No need to initialize cpu.

> +    Error *err = NULL;
> +
> +    cpu = cpu_s390x_create(cpu_model, &err);
> +    if (err != NULL) {
> +        goto out;
> +    }
> +
> +    object_property_set_int(OBJECT(cpu), id, "id", &err);
> +    if (err != NULL) {
> +        goto out;
> +    }
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> 
> +out:
> +    if (err) {
> +        error_propagate(errp, err);
> +        object_unref(OBJECT(cpu));
> +        cpu = NULL;
> +    }
> +    return cpu;
> +}
> +
> +S390CPU *cpu_s390x_init(const char *cpu_model)
> +{
> +    Error *err = NULL;
> +    S390CPU *cpu;
> +
> +    cpu = s390_new_cpu(cpu_model, next_cpu_id++, &err);
> +    if (err) {
> +        error_report_err(err);
> +    }
>      return cpu;
>  }
> 

This looks like the right way for me. Only minor nits.

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Bharata B Rao]

On Thu, Mar 03, 2016 at 04:30:32PM -0500, Matthew Rosato wrote:
> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio.c |  2 +-
>  target-s390x/cpu-qom.h |  1 +
>  target-s390x/cpu.c     | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-s390x/cpu.h     |  2 ++
>  target-s390x/helper.c  | 42 +++++++++++++++++++++++++++++++++++--
>  5 files changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index f00d6b4..2ab7b94 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -116,7 +116,7 @@ void s390_init_cpus(MachineState *machine)
>      }
> 
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu_s390x_init(machine->cpu_model);
> +        s390_new_cpu(machine->cpu_model, i, &error_fatal);
>      }
>  }
> 
> diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
> index 56d82f2..1c90933 100644
> --- a/target-s390x/cpu-qom.h
> +++ b/target-s390x/cpu-qom.h
> @@ -68,6 +68,7 @@ typedef struct S390CPU {
>      /*< public >*/
> 
>      CPUS390XState env;
> +    int64_t id;
>      /* needed for live migration */
>      void *irqstate;
>      uint32_t irqstate_saved_size;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 76c8eaf..d1b7af9 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -30,8 +30,10 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "trace.h"
> +#include "qapi/visitor.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #endif
> 
>  #define CR0_RESET       0xE0UL
> @@ -199,16 +201,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUS390XState *env = &cpu->env;
>      Error *err = NULL;
> 
> +#if !defined(CONFIG_USER_ONLY)
> +    if (cpu->id >= max_cpus) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", max allowed: %d", cpu->id, max_cpus - 1);
> +        return;
> +    }
> +#endif
> +    if (cpu_exists(cpu->id)) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", it already exists", cpu->id);
> +        return;
> +    }
> +    if (cpu->id != scc->next_cpu_id) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", The next available id is %" PRIi64, cpu->id,
> +                   scc->next_cpu_id);
> +        return;
> +    }
> +
>      cpu_exec_init(cs, &err);
>      if (err != NULL) {
>          error_propagate(errp, err);
>          return;
>      }
> +    scc->next_cpu_id = cs->cpu_index + 1;

It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
for you. If it is just going to be monotonically increasing like cs->cpu_index,
couldn't you just use cs->cpu_index instead of introducing additional IDs ?

Note that cpu_exec_init(cs, &err) returns with the next available cpu_index
which can be compared against max_cpus directly.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> >      cpu_exec_init(cs, &err);
> >      if (err != NULL) {
> >          error_propagate(errp, err);
> >          return;
> >      }
> > +    scc->next_cpu_id = cs->cpu_index + 1;  
> 
> It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
> for you. If it is just going to be monotonically increasing like cs->cpu_index,
> couldn't you just use cs->cpu_index instead of introducing additional IDs ?
> 
> Note that cpu_exec_init(cs, &err) returns with the next available cpu_index
> which can be compared against max_cpus directly.
> 
> Regards,
> Bharata.

I don't think that we should mix the id setting and cpu_index for now.

We can't simply set cpu_index before the device is realized. That logic
belongs to cpu_exec_init(). But I agree that cpu_index and id should always
match after a successful realize.

That id / cpu_index stuff can be cleaned up later. We should not treat cpu_index
as a property for now (by exposing it as "id").

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Bharata B Rao]

On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> 
> > >      cpu_exec_init(cs, &err);
> > >      if (err != NULL) {
> > >          error_propagate(errp, err);
> > >          return;
> > >      }
> > > +    scc->next_cpu_id = cs->cpu_index + 1;  
> > 
> > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
> > for you. If it is just going to be monotonically increasing like cs->cpu_index,
> > couldn't you just use cs->cpu_index instead of introducing additional IDs ?
> > 
> > Note that cpu_exec_init(cs, &err) returns with the next available cpu_index
> > which can be compared against max_cpus directly.
> > 
> > Regards,
> > Bharata.
> 
> I don't think that we should mix the id setting and cpu_index for now.
> 
> We can't simply set cpu_index before the device is realized. That logic
> belongs to cpu_exec_init().

Yes, I see that, but apart from the following obvious uses of the id
property from realizefn, are there other uses ?

1 Checking against max_cpus
  cpu_index can be used for this.

2 Checking if cpu with such an id exists
  cpu_exec_init() would never return with an in-use index. Hence cpu_index
  can be used here too given that you don't define ->get_arch_id()

3 Checking if the id is the next expected one
  cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
  always have the next available id to be used in cpu_index.

Did I miss any other use other than these which makes it necessary to have
an explicit id property here ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Bharata B Rao]

On Fri, Mar 04, 2016 at 05:01:29PM +0530, Bharata B Rao wrote:
> On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> > 
> > > >      cpu_exec_init(cs, &err);
> > > >      if (err != NULL) {
> > > >          error_propagate(errp, err);
> > > >          return;
> > > >      }
> > > > +    scc->next_cpu_id = cs->cpu_index + 1;  
> > > 
> > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
> > > for you. If it is just going to be monotonically increasing like cs->cpu_index,
> > > couldn't you just use cs->cpu_index instead of introducing additional IDs ?
> > > 
> > > Note that cpu_exec_init(cs, &err) returns with the next available cpu_index
> > > which can be compared against max_cpus directly.
> > > 
> > > Regards,
> > > Bharata.
> > 
> > I don't think that we should mix the id setting and cpu_index for now.
> > 
> > We can't simply set cpu_index before the device is realized. That logic
> > belongs to cpu_exec_init().
> 
> Yes, I see that, but apart from the following obvious uses of the id
> property from realizefn, are there other uses ?
> 
> 1 Checking against max_cpus
>   cpu_index can be used for this.
> 
> 2 Checking if cpu with such an id exists
>   cpu_exec_init() would never return with an in-use index. Hence cpu_index
>   can be used here too given that you don't define ->get_arch_id()
> 
> 3 Checking if the id is the next expected one
>   cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
>   always have the next available id to be used in cpu_index.
> 
> Did I miss any other use other than these which makes it necessary to have
> an explicit id property here ?

Oops, Sorry this is not device_add, but cpu-add that gets the id from the
user. So ignore the above :(

> 
> Regards,
> Bharata.

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> >   
> > > >      cpu_exec_init(cs, &err);
> > > >      if (err != NULL) {
> > > >          error_propagate(errp, err);
> > > >          return;
> > > >      }
> > > > +    scc->next_cpu_id = cs->cpu_index + 1;    
> > > 
> > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
> > > for you. If it is just going to be monotonically increasing like cs->cpu_index,
> > > couldn't you just use cs->cpu_index instead of introducing additional IDs ?
> > > 
> > > Note that cpu_exec_init(cs, &err) returns with the next available cpu_index
> > > which can be compared against max_cpus directly.
> > > 
> > > Regards,
> > > Bharata.  
> > 
> > I don't think that we should mix the id setting and cpu_index for now.
> > 
> > We can't simply set cpu_index before the device is realized. That logic
> > belongs to cpu_exec_init().  
> 
> Yes, I see that, but apart from the following obvious uses of the id
> property from realizefn, are there other uses ?
> 
> 1 Checking against max_cpus
>   cpu_index can be used for this.
> 
> 2 Checking if cpu with such an id exists
>   cpu_exec_init() would never return with an in-use index. Hence cpu_index
>   can be used here too given that you don't define ->get_arch_id()
> 
> 3 Checking if the id is the next expected one
>   cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
>   always have the next available id to be used in cpu_index.
> 
> Did I miss any other use other than these which makes it necessary to have
> an explicit id property here ?

After all the discussions about
-device-add s390-cpu,id=XX

As substitute/addition in the future for hotplug it is the straightforward
approach to allow setting the id as property. Nobody knows what crazy new
hotplug method we will come up with. But doing it the device way with properties
cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).

So I'd like to avoid reworking everything again, to realize later that we
want it as a property and rewriting it once again.

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Igor Mammedov]

On Fri, 4 Mar 2016 12:50:05 +0100
David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:

> > On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:  
> > >     
> > > > >      cpu_exec_init(cs, &err);
> > > > >      if (err != NULL) {
> > > > >          error_propagate(errp, err);
> > > > >          return;
> > > > >      }
> > > > > +    scc->next_cpu_id = cs->cpu_index + 1;      
> > > > 
> > > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
> > > > for you. If it is just going to be monotonically increasing like cs->cpu_index,
> > > > couldn't you just use cs->cpu_index instead of introducing additional IDs ?
> > > > 
> > > > Note that cpu_exec_init(cs, &err) returns with the next available cpu_index
> > > > which can be compared against max_cpus directly.
> > > > 
> > > > Regards,
> > > > Bharata.    
> > > 
> > > I don't think that we should mix the id setting and cpu_index for now.
> > > 
> > > We can't simply set cpu_index before the device is realized. That logic
> > > belongs to cpu_exec_init().    
> > 
> > Yes, I see that, but apart from the following obvious uses of the id
> > property from realizefn, are there other uses ?
> > 
> > 1 Checking against max_cpus
> >   cpu_index can be used for this.
> > 
> > 2 Checking if cpu with such an id exists
> >   cpu_exec_init() would never return with an in-use index. Hence cpu_index
> >   can be used here too given that you don't define ->get_arch_id()
> > 
> > 3 Checking if the id is the next expected one
> >   cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
> >   always have the next available id to be used in cpu_index.
> > 
> > Did I miss any other use other than these which makes it necessary to have
> > an explicit id property here ?  
> 
> After all the discussions about
> -device-add s390-cpu,id=XX
> 
> As substitute/addition in the future for hotplug it is the straightforward
> approach to allow setting the id as property. Nobody knows what crazy new
> hotplug method we will come up with. But doing it the device way with properties
> cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).
with device_add 'id' is not a vcpu concept but and arbitrary user supplied string
property owned by Device. But since s390 matches current x86 thread based model it could be migrated to device_add the same way, for example:
  device_add s390-cpu,thread=XX

> 
> So I'd like to avoid reworking everything again, to realize later that we
> want it as a property and rewriting it once again.
for s390, the thing about not rewriting everything once again could be
replacing places where cpu_index is used with CpuClass.arch_id().
arch_id() defaults to cpu_index but you can later override it with
your own id (whatever s390 uses for identifying cpus on baremetal)
so switching to device_add won't break anything.


> 
> David
> 

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> > After all the discussions about
> > -device-add s390-cpu,id=XX
> > 
> > As substitute/addition in the future for hotplug it is the straightforward
> > approach to allow setting the id as property. Nobody knows what crazy new
> > hotplug method we will come up with. But doing it the device way with properties
> > cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).  
> with device_add 'id' is not a vcpu concept but and arbitrary user supplied string
> property owned by Device. But since s390 matches current x86 thread based model it could be migrated to device_add the same way, for example:
>   device_add s390-cpu,thread=XX

So should we name the property thread then?
Looks like the id property is really special.

What do you suggest?

> 
> > 
> > So I'd like to avoid reworking everything again, to realize later that we
> > want it as a property and rewriting it once again.  
> for s390, the thing about not rewriting everything once again could be
> replacing places where cpu_index is used with CpuClass.arch_id().
> arch_id() defaults to cpu_index but you can later override it with
> your own id (whatever s390 uses for identifying cpus on baremetal)
> so switching to device_add won't break anything.

Okay, this way we could get rid of cpu_index later.

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Igor Mammedov]

On Mon, 7 Mar 2016 11:02:11 +0100
David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:

> > > After all the discussions about
> > > -device-add s390-cpu,id=XX
> > > 
> > > As substitute/addition in the future for hotplug it is the straightforward
> > > approach to allow setting the id as property. Nobody knows what crazy new
> > > hotplug method we will come up with. But doing it the device way with properties
> > > cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).    
> > with device_add 'id' is not a vcpu concept but and arbitrary user supplied string
> > property owned by Device. But since s390 matches current x86 thread based model it could be migrated to device_add the same way, for example:
> >   device_add s390-cpu,thread=XX  
> 
> So should we name the property thread then?
> Looks like the id property is really special.
> 
> What do you suggest?
I plan to add 'thread' property to x86-cpu, so you could the same for
s390 when the time for device_add comes there.

> 
> >   
> > > 
> > > So I'd like to avoid reworking everything again, to realize later that we
> > > want it as a property and rewriting it once again.    
> > for s390, the thing about not rewriting everything once again could be
> > replacing places where cpu_index is used with CpuClass.arch_id().
> > arch_id() defaults to cpu_index but you can later override it with
> > your own id (whatever s390 uses for identifying cpus on baremetal)
> > so switching to device_add won't break anything.  
> 
> Okay, this way we could get rid of cpu_index later.
> 
> David
> 
> 

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Cornelia Huck]

On Mon, 7 Mar 2016 11:12:14 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 7 Mar 2016 11:02:11 +0100
> David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
> 
> > > > After all the discussions about
> > > > -device-add s390-cpu,id=XX
> > > > 
> > > > As substitute/addition in the future for hotplug it is the straightforward
> > > > approach to allow setting the id as property. Nobody knows what crazy new
> > > > hotplug method we will come up with. But doing it the device way with properties
> > > > cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).    
> > > with device_add 'id' is not a vcpu concept but and arbitrary user supplied string
> > > property owned by Device. But since s390 matches current x86 thread based model it could be migrated to device_add the same way, for example:
> > >   device_add s390-cpu,thread=XX  
> > 
> > So should we name the property thread then?
> > Looks like the id property is really special.
> > 
> > What do you suggest?
> I plan to add 'thread' property to x86-cpu, so you could the same for
> s390 when the time for device_add comes there.

So the conclusion is to simply deal with this later, right?

If so, I'll just go ahead and apply v9 :)

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Igor Mammedov]

On Mon, 7 Mar 2016 12:49:27 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 7 Mar 2016 11:12:14 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Mon, 7 Mar 2016 11:02:11 +0100
> > David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
> >   
> > > > > After all the discussions about
> > > > > -device-add s390-cpu,id=XX
> > > > > 
> > > > > As substitute/addition in the future for hotplug it is the straightforward
> > > > > approach to allow setting the id as property. Nobody knows what crazy new
> > > > > hotplug method we will come up with. But doing it the device way with properties
> > > > > cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).      
> > > > with device_add 'id' is not a vcpu concept but and arbitrary user supplied string
> > > > property owned by Device. But since s390 matches current x86 thread based model it could be migrated to device_add the same way, for example:
> > > >   device_add s390-cpu,thread=XX    
> > > 
> > > So should we name the property thread then?
> > > Looks like the id property is really special.
> > > 
> > > What do you suggest?  
> > I plan to add 'thread' property to x86-cpu, so you could the same for
> > s390 when the time for device_add comes there.  
> 
> So the conclusion is to simply deal with this later, right?
I'd say so.

> 
> If so, I'll just go ahead and apply v9 :)
> 

[Qemu-devel] [PATCH v8 7/7] s390x/cpu: Allow hotplug of CPUs

[Matthew Rosato]

Implement cpu hotplug routine and add the machine hook.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 13 +++++++++++++
 target-s390x/cpu.c         |  7 +++++++
 2 files changed, 20 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 7fc1879..174a2f8 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -186,6 +186,18 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
     return NULL;
 }
 
+static void s390_hot_add_cpu(const int64_t id, Error **errp)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+    Error *err = NULL;
+
+    s390_new_cpu(machine->cpu_model, id, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -194,6 +206,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
 
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
+    mc->hot_add_cpu = s390_hot_add_cpu;
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
     mc->no_floppy = 1;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index d1b7af9..4533c94 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -34,6 +34,7 @@
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #include "sysemu/sysemu.h"
+#include "hw/s390x/sclp.h"
 #endif
 
 #define CR0_RESET       0xE0UL
@@ -240,6 +241,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 #endif
 
     scc->parent_realize(dev, errp);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (dev->hotplugged) {
+        raise_irq_cpu_hotplug();
+    }
+#endif
 }
 
 static void s390_cpu_set_id(Object *obj, Visitor *v, const char *name,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v8 7/7] s390x/cpu: Allow hotplug of CPUs

[David Hildenbrand]

> Implement cpu hotplug routine and add the machine hook.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 13 +++++++++++++
>  target-s390x/cpu.c         |  7 +++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 7fc1879..174a2f8 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -186,6 +186,18 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>      return NULL;
>  }
> 
> +static void s390_hot_add_cpu(const int64_t id, Error **errp)
> +{
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    Error *err = NULL;
> +
> +    s390_new_cpu(machine->cpu_model, id, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }

You could unconditionally call error_propagate(errp, err); here

> +}
> +


Still looks good to me!

David

Re: [Qemu-devel] [PATCH v8 0/7] Allow hotplug of s390 CPUs

[Cornelia Huck]

On Thu,  3 Mar 2016 16:30:26 -0500
Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:

> Matthew Rosato (7):
>   s390x/cpu: Cleanup init in preparation for hotplug
>   s390x/cpu: Set initial CPU state in common routine
>   s390x/cpu: Get rid of side effects when creating a vcpu
>   s390x/cpu: Tolerate max_cpus
>   s390x/cpu: Add CPU property links
>   s390x/cpu: Add error handling to cpu creation
>   s390x/cpu: Allow hotplug of CPUs
> 
>  hw/s390x/s390-virtio-ccw.c | 49 ++++++++++++++++++++++++++-
>  hw/s390x/s390-virtio.c     | 36 +++++++++++---------
>  hw/s390x/s390-virtio.h     |  2 +-
>  target-s390x/cpu-qom.h     |  3 ++
>  target-s390x/cpu.c         | 83 +++++++++++++++++++++++++++++++++++++++++++---
>  target-s390x/cpu.h         |  2 ++
>  target-s390x/helper.c      | 42 +++++++++++++++++++++--
>  7 files changed, 192 insertions(+), 25 deletions(-)

If I followed the discussion here correctly, there aren't any non-minor
comments left, are there?

If so, I'd be happy to apply a v9 to my branch so we can finally get
this into 2.6.