[Qemu-devel] [PATCH v7 0/6] Allow hotplug of s390 CPUs

[Qemu-devel] [PATCH v7 0/6] Allow hotplug of s390 CPUs

[Matthew Rosato]

Changes from v6->v7:

* Patch 1 - Cannot move cpu_model default setting to machine instance_init, 
  as vl.c can overwrite it again with a NULL during startup.  Left check 
  in s390_init_cpus.
* Drop cpu_last patch (Andreas)
* Patch 4 - merged with patch that adds hotplug handler / set cpu property.
* New patch 5 - Add cpu_s390x_create and s390_new_cpu to allow for error 
  handling during cpu creation (Andreas).  Re-add mechanism for tracking next
  cpu ID.  Encapsulate IDs as a CPU property & perform checking (David) -- 
  I used x86 apic_id as a template. 
* Patch 6 - Add error handling to s390_hot_add_cpu via s390_new_cpu. (Andreas)

**************

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 (6):
  s390x/cpu: Cleanup init in preparation for hotplug
  s390x/cpu: Set initial CPU state in common routine
  s390x/cpu: Move some CPU initialization into realize
  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 | 78 +++++++++++++++++++++++++++++++++++++++++++-
 hw/s390x/s390-virtio.c     | 36 +++++++++++----------
 hw/s390x/s390-virtio.h     |  3 +-
 target-s390x/cpu-qom.h     |  3 ++
 target-s390x/cpu.c         | 80 +++++++++++++++++++++++++++++++++++++++++++---
 target-s390x/cpu.h         |  1 +
 target-s390x/helper.c      | 31 ++++++++++++++++--
 7 files changed, 208 insertions(+), 24 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v7 1/6] 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 c320878..b576811 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -95,12 +95,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);
@@ -109,7 +109,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 v7 2/6] 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 b576811..b3707f4 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -107,14 +107,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 v7 3/6] s390x/cpu: Move some CPU initialization into realize

[Matthew Rosato]

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

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 target-s390x/cpu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 603c2a1..8dfd063 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -195,7 +195,13 @@ 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;
 
+#if !defined(CONFIG_USER_ONLY)
+    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
+#endif
+    env->cpu_num = cs->cpu_index;
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -213,7 +219,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
@@ -223,7 +228,6 @@ static void s390_cpu_initfn(Object *obj)
     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 +236,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;
-- 
1.9.1

Re: [Qemu-devel] [PATCH v7 3/6] s390x/cpu: Move some CPU initialization into realize

[David Hildenbrand]

> In preparation for hotplug, defer some CPU initialization
> until the device is actually being realized.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> ---
>  target-s390x/cpu.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 603c2a1..8dfd063 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -195,7 +195,13 @@ 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;
> 
> +#if !defined(CONFIG_USER_ONLY)
> +    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> +#endif
> +    env->cpu_num = cs->cpu_index;
>      s390_cpu_gdb_init(cs);
>      qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> @@ -213,7 +219,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
> @@ -223,7 +228,6 @@ static void s390_cpu_initfn(Object *obj)
>      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 +236,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;

Hmm, two things

a) env is not needed in this patch
b) This patch breaks cpu creation temporarily (cpu number rework). cpu_num is
always 0 - a problem at east for tcg.

You should introduce scc->next_cpu_id in this patch, cpu->id can be left
in the error handling patch.

David

[Qemu-devel] [PATCH v7 4/6] 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.
Additionally, maintain an array of state pointers indexed by CPU
id for fast lookup during interrupt handling.

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b05ed8b..3090e76 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -156,10 +156,40 @@ 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(qdev_get_machine()), OBJECT(cs), name,
+                             errp);
+}
+
+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 +201,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 +264,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 b3707f4..6bd9803 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -60,15 +60,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,
@@ -98,19 +99,26 @@ 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";
     }
 
-    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
+    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
 
-    for (i = 0; i < smp_cpus; i++) {
-        S390CPU *cpu;
-
-        cpu = cpu_s390x_init(machine->cpu_model);
+    for (i = 0; i < max_cpus; i++) {
+        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);
+    }
 
-        ipi_states[i] = cpu;
+    for (i = 0; i < smp_cpus; i++) {
+        cpu_s390x_init(machine->cpu_model);
     }
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v7 4/6] s390x/cpu: Add CPU property links

[Igor Mammedov]

On Tue,  1 Mar 2016 16:13:24 -0500
Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:

> 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.
> Additionally, maintain an array of state pointers indexed by CPU
> id for fast lookup during interrupt handling.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio.c     | 26 +++++++++++++++++---------
>  2 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index b05ed8b..3090e76 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -156,10 +156,40 @@ 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(qdev_get_machine()), OBJECT(cs), name,
hotplug_dev is machine, just do cast here instead of qdev_get_machine()

> +                             errp);

looks like 'name' is being leaked

> +}
> +
> +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 +201,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 +264,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 b3707f4..6bd9803 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -60,15 +60,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];
>  }
this hunk seems to be unrelated to this patch,
it probably belongs somewhere else

>  void s390_init_ipl_dev(const char *kernel_filename,
> @@ -98,19 +99,26 @@ 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";
>      }
>  
> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> +    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
>  
> -    for (i = 0; i < smp_cpus; i++) {
> -        S390CPU *cpu;
> -
> -        cpu = cpu_s390x_init(machine->cpu_model);
> +    for (i = 0; i < max_cpus; i++) {
> +        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);
> +    }
>  
> -        ipi_states[i] = cpu;
> +    for (i = 0; i < smp_cpus; i++) {
> +        cpu_s390x_init(machine->cpu_model);
>      }
>  }
>  

[Qemu-devel] [PATCH v7 5/6] 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-ccw.c | 30 +++++++++++++++++++++
 hw/s390x/s390-virtio.c     |  2 +-
 hw/s390x/s390-virtio.h     |  1 +
 target-s390x/cpu-qom.h     |  3 +++
 target-s390x/cpu.c         | 65 ++++++++++++++++++++++++++++++++++++++++++++--
 target-s390x/cpu.h         |  1 +
 target-s390x/helper.c      | 31 ++++++++++++++++++++--
 7 files changed, 128 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3090e76..4886dbf 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size)
     s390_skeys_init();
 }
 
+S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)
+{
+    S390CPU *cpu = NULL;
+    Error *local_err = NULL;
+
+    if (id >= max_cpus) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", max allowed: %d", id, max_cpus - 1);
+        goto out;
+    }
+
+    cpu = cpu_s390x_create(machine->cpu_model, &local_err);
+    if (local_err != NULL) {
+        goto out;
+    }
+
+    object_property_set_int(OBJECT(cpu), id, "id", &local_err);
+    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
+
+out:
+    if (cpu != NULL) {
+        object_unref(OBJECT(cpu));
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        cpu = NULL;
+    }
+    return cpu;
+}
+
 static void ccw_init(MachineState *machine)
 {
     int ret;
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 6bd9803..6f71ab0 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -118,7 +118,7 @@ void s390_init_cpus(MachineState *machine)
     }
 
     for (i = 0; i < smp_cpus; i++) {
-        cpu_s390x_init(machine->cpu_model);
+        s390_new_cpu(machine, i, &error_fatal);
     }
 }
 
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index ffd014c..fbcfdb8 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -29,4 +29,5 @@ void s390_create_virtio_net(BusState *bus, const char *name);
 void s390_nmi(NMIState *n, int cpu_index, Error **errp);
 void s390_machine_reset(void);
 void s390_memory_init(ram_addr_t mem_size);
+S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp);
 #endif
diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index 029a44a..1c90933 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);
@@ -66,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 8dfd063..ec66ed6 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -30,6 +30,7 @@
 #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"
 #endif
@@ -197,11 +198,26 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
     S390CPU *cpu = S390_CPU(dev);
     CPUS390XState *env = &cpu->env;
+    Error *local_err = NULL;
+
+    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, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_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 = cs->cpu_index;
+    env->cpu_num = cpu->id;
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -213,6 +229,49 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     scc->parent_realize(dev, errp);
 }
 
+static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    S390CPU *cpu = S390_CPU(obj);
+    int64_t value = cpu->id;
+
+    visit_type_int(v, name, &value, 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 *local_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, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_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;
+    }
+    if ((value != cpu->id) && cpu_exists(value)) {
+        error_setg(errp, "CPU with ID %" PRIi64 " exists", value);
+        return;
+    }
+    cpu->id = value;
+}
+
 static void s390_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -226,7 +285,8 @@ 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);
+    object_property_add(OBJECT(cpu), "id", "int64_t", s390_cpu_get_id,
+                        s390_cpu_set_id, NULL, NULL, NULL);
 #if !defined(CONFIG_USER_ONLY)
     qemu_get_timedate(&tm, 0);
     env->tod_offset = TOD_UNIX_EPOCH +
@@ -342,6 +402,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;
 
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 6ae5699..2c7d6bd 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -413,6 +413,7 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
 #endif
 
 S390CPU *cpu_s390x_init(const char *cpu_model);
+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..e562cb7 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -65,14 +65,41 @@ 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 *cpu_s390x_init(const char *cpu_model)
+{
+    Error *error = NULL;
+    S390CPU *cpu;
+    S390CPUClass *scc;
+    int64_t id;
 
+    cpu = cpu_s390x_create(cpu_model, &error);
+    if (error) {
+        goto out;
+    }
+
+    scc = S390_CPU_GET_CLASS(cpu);
+    id = scc->next_cpu_id;
+
+    object_property_set_int(OBJECT(cpu), id, "id", &error);
+    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
+
+ out:
+    if (error) {
+        error_report_err(error);
+        if (cpu != NULL) {
+            object_unref(OBJECT(cpu));
+            cpu = NULL;
+        }
+    }
     return cpu;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v7 5/6] 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-ccw.c | 30 +++++++++++++++++++++
>  hw/s390x/s390-virtio.c     |  2 +-
>  hw/s390x/s390-virtio.h     |  1 +
>  target-s390x/cpu-qom.h     |  3 +++
>  target-s390x/cpu.c         | 65 ++++++++++++++++++++++++++++++++++++++++++++--
>  target-s390x/cpu.h         |  1 +
>  target-s390x/helper.c      | 31 ++++++++++++++++++++--
>  7 files changed, 128 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3090e76..4886dbf 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size)
>      s390_skeys_init();
>  }
> 
> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)
> +{
> +    S390CPU *cpu = NULL;
> +    Error *local_err = NULL;

Think the naming schema is "err" now.

> +
> +    if (id >= max_cpus) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", max allowed: %d", id, max_cpus - 1);
> +        goto out;

Could we also move this check to the realize function?

> +    }
> +
> +    cpu = cpu_s390x_create(machine->cpu_model, &local_err);
> +    if (local_err != NULL) {
> +        goto out;
> +    }
> +
> +    object_property_set_int(OBJECT(cpu), id, "id", &local_err);

We should add a check in between

if (err) {
    goto out;
}

> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> +
> +out:
> +    if (cpu != NULL) {
> +        object_unref(OBJECT(cpu));

Is the object_unref() here correct?
I know that we have one reference from VCPU creation. Where does the second one
come from (is it from the hotplug handler? then I'd prefer a comment here :D )

> +    }
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        cpu = NULL;
> +    }
> +    return cpu;
> +}
> +
>  static void ccw_init(MachineState *machine)
>  {
>      int ret;
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 6bd9803..6f71ab0 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -118,7 +118,7 @@ void s390_init_cpus(MachineState *machine)
>      }
> 
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu_s390x_init(machine->cpu_model);
> +        s390_new_cpu(machine, i, &error_fatal);
>      }
>  }
> 
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index ffd014c..fbcfdb8 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -29,4 +29,5 @@ void s390_create_virtio_net(BusState *bus, const char *name);
>  void s390_nmi(NMIState *n, int cpu_index, Error **errp);
>  void s390_machine_reset(void);
>  void s390_memory_init(ram_addr_t mem_size);
> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp);
>  #endif
> diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
> index 029a44a..1c90933 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);
> @@ -66,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 8dfd063..ec66ed6 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -30,6 +30,7 @@
>  #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"
>  #endif
> @@ -197,11 +198,26 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>      S390CPU *cpu = S390_CPU(dev);
>      CPUS390XState *env = &cpu->env;
> +    Error *local_err = NULL;
> +
> +    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, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_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 = cs->cpu_index;
> +    env->cpu_num = cpu->id;
>      s390_cpu_gdb_init(cs);
>      qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> @@ -213,6 +229,49 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      scc->parent_realize(dev, errp);
>  }
> 
> +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    S390CPU *cpu = S390_CPU(obj);
> +    int64_t value = cpu->id;
> +
> +    visit_type_int(v, name, &value, 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 *local_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, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_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;
> +    }
> +    if ((value != cpu->id) && cpu_exists(value)) {
> +        error_setg(errp, "CPU with ID %" PRIi64 " exists", value);
> +        return;
> +    }
> +    cpu->id = value;
> +}

Just curious, what about using a simple

object_property_set_int() and doing all the checks in realize() ?

Then we could live without manual getter/setter (and without the realize check).

> +
>  static void s390_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -226,7 +285,8 @@ 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);
> +    object_property_add(OBJECT(cpu), "id", "int64_t", s390_cpu_get_id,
> +                        s390_cpu_set_id, NULL, NULL, NULL);
>  #if !defined(CONFIG_USER_ONLY)
>      qemu_get_timedate(&tm, 0);
>      env->tod_offset = TOD_UNIX_EPOCH +
> @@ -342,6 +402,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;
> 
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 6ae5699..2c7d6bd 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -413,6 +413,7 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
>  #endif
> 
>  S390CPU *cpu_s390x_init(const char *cpu_model);
> +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..e562cb7 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -65,14 +65,41 @@ 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 *cpu_s390x_init(const char *cpu_model)
> +{
> +    Error *error = NULL;
> +    S390CPU *cpu;
> +    S390CPUClass *scc;
> +    int64_t id;
> 
> +    cpu = cpu_s390x_create(cpu_model, &error);
> +    if (error) {
> +        goto out;
> +    }
> +
> +    scc = S390_CPU_GET_CLASS(cpu);
> +    id = scc->next_cpu_id;
> +
> +    object_property_set_int(OBJECT(cpu), id, "id", &error);

dito

> +    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
> +
> + out:
> +    if (error) {
> +        error_report_err(error);
> +        if (cpu != NULL) {
> +            object_unref(OBJECT(cpu));
> +            cpu = NULL;
> +        }
> +    }

Can we make both error handling blocks (s390_cpu_add) look alike?

>      return cpu;
>  }
> 

This looks much better to me!

David

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

[Matthew Rosato]

>> +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name,
>> +                            void *opaque, Error **errp)
>> +{
>> +    S390CPU *cpu = S390_CPU(obj);
>> +    int64_t value = cpu->id;
>> +
>> +    visit_type_int(v, name, &value, 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 *local_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, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_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;
>> +    }
>> +    if ((value != cpu->id) && cpu_exists(value)) {
>> +        error_setg(errp, "CPU with ID %" PRIi64 " exists", value);
>> +        return;
>> +    }
>> +    cpu->id = value;
>> +}
> 
> Just curious, what about using a simple
> 
> object_property_set_int() and doing all the checks in realize() ?
> 
> Then we could live without manual getter/setter (and without the realize check).
> 

I think we still need at least a manual setter, even if you want to move
the checks to realize.

See something like object_property_add_uint64_ptr() -- It sets a
boilerplate get routine, and no set routine -- I think this presumes you
set your property upfront (at add time), never change it for the life of
the object, but want to read it later.
By comparison, S390CPU.id is set sometime after instance_init, based on
input.

So, we call object_property_set_int() to update it --  This just passes
the provided int value to the setter routine associated with the
property.  If one doesn't exist, you get:
qemu: Insufficient permission to perform this operation

I think this is also why we want to check for dev->realized in the
setter routine, to make sure the property is not being changed "too
late" -- Once the cpu is realized, the ID is baked and can't be changed.

Or did I misunderstand your idea here?

Matt

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

[David Hildenbrand]

> >> +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name,
> >> +                            void *opaque, Error **errp)
> >> +{
> >> +    S390CPU *cpu = S390_CPU(obj);
> >> +    int64_t value = cpu->id;
> >> +
> >> +    visit_type_int(v, name, &value, 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 *local_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, &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_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;
> >> +    }
> >> +    if ((value != cpu->id) && cpu_exists(value)) {
> >> +        error_setg(errp, "CPU with ID %" PRIi64 " exists", value);
> >> +        return;
> >> +    }
> >> +    cpu->id = value;
> >> +}  
> > 
> > Just curious, what about using a simple
> > 
> > object_property_set_int() and doing all the checks in realize() ?
> > 
> > Then we could live without manual getter/setter (and without the realize check).
> >   
> 
> I think we still need at least a manual setter, even if you want to move
> the checks to realize.
> 
> See something like object_property_add_uint64_ptr() -- It sets a
> boilerplate get routine, and no set routine -- I think this presumes you
> set your property upfront (at add time), never change it for the life of
> the object, but want to read it later.
> By comparison, S390CPU.id is set sometime after instance_init, based on
> input.
> 
> So, we call object_property_set_int() to update it --  This just passes
> the provided int value to the setter routine associated with the
> property.  If one doesn't exist, you get:
> qemu: Insufficient permission to perform this operation
> 
> I think this is also why we want to check for dev->realized in the
> setter routine, to make sure the property is not being changed "too
> late" -- Once the cpu is realized, the ID is baked and can't be changed.
> 
> Or did I misunderstand your idea here?

If we care about malicious users, wanting to set id's after realize that is
true. But I am no QOM expert and don't know if that is a scenarios that
has to be taken care of. But as I see similar code for other properties,
I assume we are better off doing it also that way.

David

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

[Matthew Rosato]

>> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)
>> +{
>> +    S390CPU *cpu = NULL;
>> +    Error *local_err = NULL;
> 
> Think the naming schema is "err" now.
> 
>> +
>> +    if (id >= max_cpus) {
>> +        error_setg(errp, "Unable to add CPU: %" PRIi64
>> +                   ", max allowed: %d", id, max_cpus - 1);
>> +        goto out;
> 
> Could we also move this check to the realize function?
> 
>> +    }
>> +
>> +    cpu = cpu_s390x_create(machine->cpu_model, &local_err);
>> +    if (local_err != NULL) {
>> +        goto out;
>> +    }
>> +
>> +    object_property_set_int(OBJECT(cpu), id, "id", &local_err);
> 
> We should add a check in between
> 
> if (err) {
>     goto out;
> }
> 
>> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>> +
>> +out:
>> +    if (cpu != NULL) {
>> +        object_unref(OBJECT(cpu));
> 
> Is the object_unref() here correct?
> I know that we have one reference from VCPU creation. Where does the second one
> come from (is it from the hotplug handler? then I'd prefer a comment here :D )
> 

After some digging, I believe this unref is not necessary for s390
(bus-less) and I'm now questioning the i386 code that I used as a base...

@Igor/Andreas:

In i386, looks like the unrefs were due to the ref created when adding
the cpu to the icc bus.  Andreas moved the checks outside of pc_new_cpu
and explains their purpose here:
0e3bd562 - pc: Ensure non-zero CPU ref count after attaching to ICC bus

But then a subsequent patch removed the bus and left the unrefs:
46232aaa - cpu/apic: drop icc bus/bridge

Should that patch not have also dropped the unrefs in pc_hot_add_cpu()
and pc_cpus_init()?

Matt

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

[Igor Mammedov]

On Thu, 3 Mar 2016 12:52:17 -0500
Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:

> >> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)
> >> +{
> >> +    S390CPU *cpu = NULL;
> >> +    Error *local_err = NULL;  
> > 
> > Think the naming schema is "err" now.
> >   
> >> +
> >> +    if (id >= max_cpus) {
> >> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> >> +                   ", max allowed: %d", id, max_cpus - 1);
> >> +        goto out;  
> > 
> > Could we also move this check to the realize function?
> >   
> >> +    }
> >> +
> >> +    cpu = cpu_s390x_create(machine->cpu_model, &local_err);
> >> +    if (local_err != NULL) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    object_property_set_int(OBJECT(cpu), id, "id", &local_err);  
> > 
> > We should add a check in between
> > 
> > if (err) {
> >     goto out;
> > }
> >   
> >> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> >> +
> >> +out:
> >> +    if (cpu != NULL) {
> >> +        object_unref(OBJECT(cpu));  
> > 
> > Is the object_unref() here correct?
> > I know that we have one reference from VCPU creation. Where does the second one
> > come from (is it from the hotplug handler? then I'd prefer a comment here :D )
> >   
> 
> After some digging, I believe this unref is not necessary for s390
> (bus-less) and I'm now questioning the i386 code that I used as a base...
> 
> @Igor/Andreas:
> 
> In i386, looks like the unrefs were due to the ref created when adding
> the cpu to the icc bus.  Andreas moved the checks outside of pc_new_cpu
> and explains their purpose here:
> 0e3bd562 - pc: Ensure non-zero CPU ref count after attaching to ICC bus
> 
> But then a subsequent patch removed the bus and left the unrefs:
> 46232aaa - cpu/apic: drop icc bus/bridge
> 
> Should that patch not have also dropped the unrefs in pc_hot_add_cpu()
> and pc_cpus_init()?
nope, bus made it own ref, nref is needed here to avoid leaking object
as device_realize() implicitly adds it to /machine/devices/unattached/
creating an extra ref along the way.

> 
> Matt
> 

Re: [Qemu-devel] [PATCH v7 5/6] 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-ccw.c | 30 +++++++++++++++++++++
>  hw/s390x/s390-virtio.c     |  2 +-
>  hw/s390x/s390-virtio.h     |  1 +
>  target-s390x/cpu-qom.h     |  3 +++
>  target-s390x/cpu.c         | 65 ++++++++++++++++++++++++++++++++++++++++++++--
>  target-s390x/cpu.h         |  1 +
>  target-s390x/helper.c      | 31 ++++++++++++++++++++--
>  7 files changed, 128 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3090e76..4886dbf 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size)
>      s390_skeys_init();
>  }
> 
> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)

Just a thought, if not passing machine but the model string, we could make

cpu_s390x_init() call s390_new_cpu().

But then s390_new_cpu() would have to be moved again. Not sure if this is
really worth it.


David

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

[Matthew Rosato]

On 03/02/2016 02:59 AM, David Hildenbrand wrote:
>> Check for and propogate errors during s390 cpu creation.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++
>>  hw/s390x/s390-virtio.c     |  2 +-
>>  hw/s390x/s390-virtio.h     |  1 +
>>  target-s390x/cpu-qom.h     |  3 +++
>>  target-s390x/cpu.c         | 65 ++++++++++++++++++++++++++++++++++++++++++++--
>>  target-s390x/cpu.h         |  1 +
>>  target-s390x/helper.c      | 31 ++++++++++++++++++++--
>>  7 files changed, 128 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 3090e76..4886dbf 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size)
>>      s390_skeys_init();
>>  }
>>
>> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)
> 
> Just a thought, if not passing machine but the model string, we could make
> 
> cpu_s390x_init() call s390_new_cpu().
> 
> But then s390_new_cpu() would have to be moved again. Not sure if this is
> really worth it.
> 

Additionally, next_cpu_id would have to be moved somewhere other than
the S390CPUClass, as we need that value for input to s390_new_cpu()
here, but won't have a cpu object to acquire it until after
s390_new_cpu() returns.

Going to leave cpu_s390x_init() calling cpu_s390x_create() unless
someone voices a strong opinion.

Matt

[Qemu-devel] [PATCH v7 6/6] 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>
---
 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 4886dbf..cfb2ef4 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -215,6 +215,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 *local_err = NULL;
+
+    s390_new_cpu(machine, id, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -223,6 +235,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 ec66ed6..6585f04 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -33,6 +33,7 @@
 #include "qapi/visitor.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
+#include "hw/s390x/sclp.h"
 #endif
 
 #define CR0_RESET       0xE0UL
@@ -227,6 +228,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_get_id(Object *obj, Visitor *v, const char *name,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v7 6/6] 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 4886dbf..cfb2ef4 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -215,6 +215,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 *local_err = NULL;
> +
> +    s390_new_cpu(machine, id, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +}
> +
>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -223,6 +235,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 ec66ed6..6585f04 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -33,6 +33,7 @@
>  #include "qapi/visitor.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "hw/s390x/sclp.h"
>  #endif
> 
>  #define CR0_RESET       0xE0UL
> @@ -227,6 +228,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_get_id(Object *obj, Visitor *v, const char *name,



David