[Qemu-devel] [PATCH 0/8] [PATCH RFC v2] s390-qemu: cpu hotplug

[Qemu-devel] [PATCH 0/8] [PATCH RFC v2] s390-qemu: cpu hotplug

[Jason J. Herne]

Latest code for cpu Hotplug designed to be controled via the QOM infrastructure.
cpu on S390 are treated as devices via a new platform independent
infrastructure I designed to allow this "new way" to exist with the "old way"
of representing cpus.

The Qemu command line now allows "-device s390-cpu" which will instantiate a 
cpu device. This is additive to anything that might be specified on the -smp
parameter.

Devices can be hot plugged via the monitor command "device_add s390-cpu".
Hotplugged cpus are created in the configured state and can be used by the
guest after the guest onlines the cpu by: 
"echo 1 > /sys/bus/cpu/devices/cpuN/online"

Hot unplugging is currently not implemented by this code. 

[Qemu-devel] [PATCH 1/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Define New SCLP Codes

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Define new SCLP codes to improve code readability.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/sclp.c         |    2 +-
 include/hw/s390x/sclp.h |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 86d6ae0..cb53d7e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
 {
     S390SCLPDevice *sdev = get_event_facility();
 
-    switch (code) {
+    switch (code & SCLP_NO_CMD_PARM) {
     case SCLP_CMDW_READ_SCP_INFO:
     case SCLP_CMDW_READ_SCP_INFO_FORCED:
         read_SCP_info(sccb);
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 231a38a..174097d 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -26,6 +26,14 @@
 #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
 #define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
 
+/* CPU hotplug SCLP codes */
+#define SCLP_NO_CMD_PARM                        0xffff00ff
+#define SCLP_HAS_CPU_INFO                       0x0C00000000000000ULL
+#define SCLP_CMDW_READ_CPU_INFO                 0x00010001
+#define SCLP_CMDW_CONFIGURE_CPU                 0x00110001
+#define SCLP_CMDW_DECONFIGURE_CPU               0x00100001
+#define SCLP_CMDW_CPU_CMD_PARM                  0xff00
+
 /* SCLP response codes */
 #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
 #define SCLP_RC_NORMAL_COMPLETION               0x0020
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/8] [PATCH RFC v2] s390-qemu: cpu hotplug - SCLP CPU Info

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Implement the CPU data in SCLP "Read SCP Info".  And implement "Read CPU Info"
SCLP command. This data will be used by the guest to get information about hot
plugged cpus.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/sclp.c         |   38 ++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/sclp.h |   30 ++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index cb53d7e..20b718f 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -15,6 +15,7 @@
 #include "cpu.h"
 #include "sysemu/kvm.h"
 #include "exec/memory.h"
+#include "sysemu/sysemu.h"
 
 #include "hw/s390x/sclp.h"
 
@@ -32,6 +33,19 @@ static void read_SCP_info(SCCB *sccb)
 {
     ReadInfo *read_info = (ReadInfo *) sccb;
     int shift = 0;
+    int i = 0;
+
+    /* CPU information */
+    read_info->entries_cpu = cpu_to_be16(smp_cpus);
+    read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
+    read_info->highest_cpu = cpu_to_be16(max_cpus);
+
+    for (i = 0; i < smp_cpus; i++) {
+        read_info->entries[i].address = i;
+        read_info->entries[i].type = 0;
+    }
+
+    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO);
 
     while ((ram_size >> (20 + shift)) > 65535) {
         shift++;
@@ -41,6 +55,27 @@ static void read_SCP_info(SCCB *sccb)
     sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
 }
 
+static void sclp_read_cpu_info(SCCB *sccb)
+{
+    ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
+    int i = 0;
+
+    cpu_info->nr_configured = cpu_to_be16(smp_cpus);
+    cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
+    cpu_info->nr_standby = cpu_to_be16(0);
+
+    /* The standby offset is 16-byte for each CPU */
+    cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
+        + cpu_info->nr_configured*sizeof(CpuEntry));
+
+    for (i = 0; i < smp_cpus; i++) {
+        cpu_info->entries[i].address = i;
+        cpu_info->entries[i].type = 0;
+    }
+
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
+}
+
 static void sclp_execute(SCCB *sccb, uint64_t code)
 {
     S390SCLPDevice *sdev = get_event_facility();
@@ -50,6 +85,9 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
     case SCLP_CMDW_READ_SCP_INFO_FORCED:
         read_SCP_info(sccb);
         break;
+    case SCLP_CMDW_READ_CPU_INFO:
+        sclp_read_cpu_info(sccb);
+        break;
     default:
         sdev->sclp_command_handler(sdev->ef, sccb, code);
         break;
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 174097d..6783d40 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -79,12 +79,42 @@ typedef struct SCCBHeader {
 
 #define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
 
+/* CPU information */
+typedef struct CpuEntry {
+    uint8_t address;
+    uint8_t reserved0[13];
+    uint8_t type;
+    uint8_t reserved1;
+} QEMU_PACKED CpuEntry;
+
 typedef struct ReadInfo {
     SCCBHeader h;
     uint16_t rnmax;
     uint8_t rnsize;
+    uint8_t  _reserved1[16 - 11];       /* 11-15 */
+    uint16_t entries_cpu;               /* 16-17 */
+    uint16_t offset_cpu;                /* 18-19 */
+    uint8_t  _reserved2[48 - 20];       /* 20-47 */
+    uint64_t facilities;                /* 48-55 */
+    uint8_t  _reserved3[100 - 56];
+    uint32_t rnsize2;
+    uint64_t rnmax2;
+    uint8_t  _reserved4[120-112];       /* 112-119 */
+    uint16_t highest_cpu;
+    uint8_t  _reserved5[128 - 122];     /* 122-127 */
+    struct CpuEntry entries[0];
 } QEMU_PACKED ReadInfo;
 
+typedef struct ReadCpuInfo {
+    SCCBHeader h;
+    uint16_t nr_configured;         /* 8-9 */
+    uint16_t offset_configured;     /* 10-11 */
+    uint16_t nr_standby;            /* 12-13 */
+    uint16_t offset_standby;        /* 14-15 */
+    uint8_t reserved0[24-16];       /* 16-23 */
+    struct CpuEntry entries[0];
+} QEMU_PACKED ReadCpuInfo;
+
 typedef struct SCCB {
     SCCBHeader h;
     char data[SCCB_DATA_LEN];
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/8] [PATCH RFC v2] s390-qemu: cpu hotplug - SCLP Event integration

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Add an sclp event for "cpu was hot plugged".  This allows Qemu to deliver an
SCLP interrupt to the guest stating that the requested cpu hotplug was
completed.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/Makefile.objs            |    2 +-
 hw/s390x/event-facility.c         |    7 +++
 hw/s390x/sclpcpu.c                |  119 +++++++++++++++++++++++++++++++++++++
 include/hw/s390x/event-facility.h |    3 +
 include/hw/s390x/sclp.h           |    1 +
 5 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 hw/s390x/sclpcpu.c

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 77e1218..856b65d 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -2,7 +2,7 @@ obj-y = s390-virtio-bus.o s390-virtio.o
 obj-y += s390-virtio-hcall.o
 obj-y += sclp.o
 obj-y += event-facility.o
-obj-y += sclpquiesce.o
+obj-y += sclpquiesce.o  sclpcpu.o
 obj-y += ipl.o
 obj-y += css.o
 obj-y += s390-virtio-ccw.o
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 0faade0..aec35cb 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -317,6 +317,7 @@ static int init_event_facility(S390SCLPDevice *sdev)
 {
     SCLPEventFacility *event_facility;
     DeviceState *quiesce;
+    DeviceState *cpu_hotplug;
 
     event_facility = g_malloc0(sizeof(SCLPEventFacility));
     sdev->ef = event_facility;
@@ -335,6 +336,12 @@ static int init_event_facility(S390SCLPDevice *sdev)
     }
     qdev_init_nofail(quiesce);
 
+    cpu_hotplug = qdev_create(&event_facility->sbus.qbus, "sclpcpuhotplug");
+    if (!cpu_hotplug) {
+        return -1;
+    }
+    qdev_init_nofail(cpu_hotplug);
+
     return 0;
 }
 
diff --git a/hw/s390x/sclpcpu.c b/hw/s390x/sclpcpu.c
new file mode 100644
index 0000000..227c8d0
--- /dev/null
+++ b/hw/s390x/sclpcpu.c
@@ -0,0 +1,119 @@
+/*
+ * SCLP event type
+ *    Signal CPU - Trigger SCLP interrupt for system CPU configure or
+ *    de-configure
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Thang Pham <thang.pham@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at your
+ * option) any later version.  See the COPYING file in the top-level directory.
+ *
+ */
+#include <hw/qdev.h>
+#include "sysemu/sysemu.h"
+#include "hw/s390x/sclp.h"
+#include "hw/s390x/event-facility.h"
+#include "cpu.h"
+#include "sysemu/cpus.h"
+#include "sysemu/kvm.h"
+
+typedef struct ConfigMgtData {
+    EventBufferHeader ebh;
+    uint8_t reserved;
+    uint8_t event_qualifier;
+} QEMU_PACKED ConfigMgtData;
+
+static qemu_irq irq_cpu_hotplug; /* Only used in this file */
+
+#define EVENT_QUAL_CPU_CHANGE  1
+
+void raise_irq_cpu_hotplug(void)
+{
+    qemu_irq_raise(irq_cpu_hotplug);
+}
+
+static int event_type(void)
+{
+    return SCLP_EVENT_CONFIG_MGT_DATA;
+}
+
+static unsigned int send_mask(void)
+{
+    return SCLP_EVENT_MASK_CONFIG_MGT_DATA;
+}
+
+static unsigned int receive_mask(void)
+{
+    return 0;
+}
+
+static int read_event_data(SCLPEvent *event, EventBufferHeader *evt_buf_hdr,
+                           int *slen)
+{
+    ConfigMgtData *cdata = (ConfigMgtData *) evt_buf_hdr;
+    if (*slen < sizeof(ConfigMgtData)) {
+        return 0;
+    }
+
+    /* Event is no longer pending */
+    if (!event->event_pending) {
+        return 0;
+    }
+    event->event_pending = false;
+
+    /* Event header data */
+    cdata->ebh.length = cpu_to_be16(sizeof(ConfigMgtData));
+    cdata->ebh.type = SCLP_EVENT_CONFIG_MGT_DATA;
+    cdata->ebh.flags |= SCLP_EVENT_BUFFER_ACCEPTED;
+
+    /* Trigger a rescan of CPUs by setting event qualifier */
+    cdata->event_qualifier = EVENT_QUAL_CPU_CHANGE;
+    *slen -= sizeof(ConfigMgtData);
+
+    return 1;
+}
+
+static void trigger_signal(void *opaque, int n, int level)
+{
+    SCLPEvent *event = opaque;
+    event->event_pending = true;
+
+    /* Trigger SCLP read operation */
+    sclp_service_interrupt(0);
+}
+
+static int irq_cpu_hotplug_init(SCLPEvent *event)
+{
+    irq_cpu_hotplug = *qemu_allocate_irqs(trigger_signal, event, 1);
+    return 0;
+}
+
+static void cpu_class_init(ObjectClass *klass, void *data)
+{
+    SCLPEventClass *k = SCLP_EVENT_CLASS(klass);
+
+    k->init = irq_cpu_hotplug_init;
+    k->get_send_mask = send_mask;
+    k->get_receive_mask = receive_mask;
+    k->event_type = event_type;
+    k->read_event_data = read_event_data;
+    k->write_event_data = NULL;
+}
+
+static TypeInfo sclp_cpu_info = {
+    .name          = "sclpcpuhotplug",
+    .parent        = TYPE_SCLP_EVENT,
+    .instance_size = sizeof(SCLPEvent),
+    .class_init    = cpu_class_init,
+    .class_size    = sizeof(SCLPEventClass),
+};
+
+static void register_types(void)
+{
+    type_register_static(&sclp_cpu_info);
+}
+
+type_init(register_types)
diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
index 791ab2a..d63969f 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -17,14 +17,17 @@
 
 #include <hw/qdev.h>
 #include "qemu/thread.h"
+#include "hw/s390x/sclp.h"
 
 /* SCLP event types */
+#define SCLP_EVENT_CONFIG_MGT_DATA              0x04
 #define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
 #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
 
 /* SCLP event masks */
 #define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
 #define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
+#define SCLP_EVENT_MASK_CONFIG_MGT_DATA         0x10000000
 
 #define SCLP_UNCONDITIONAL_READ                 0x00
 #define SCLP_SELECTIVE_READ                     0x01
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 6783d40..283c586 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -152,5 +152,6 @@ typedef struct S390SCLPDeviceClass {
 
 void s390_sclp_init(void);
 void sclp_service_interrupt(uint32_t sccb);
+void raise_irq_cpu_hotplug(void);
 
 #endif
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/8] [PATCH RFC v2] s390-qemu: cpu hotplug - ipi_states enhancements

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Modify s390_cpu_addr2state to allow fetching state information for cpu addresses
above the number of online cpus.  Hotplug requires this capability.

Also add s390_cpu_set_state function to allow modification of ipi_state entries
during hotplug.

Finally, add function to find lowest unused ipi_state.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio.c |   19 ++++++++++++++++---
 target-s390x/cpu.h     |    2 ++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 30d1118..ef4f1ae 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -54,13 +54,26 @@
 static VirtIOS390Bus *s390_bus;
 static S390CPU **ipi_states;
 
+void s390_cpu_set_state(uint16_t cpu_addr, S390CPU *state)
+{
+    ipi_states[cpu_addr] = state;
+}
+
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
-    if (cpu_addr >= smp_cpus) {
-        return NULL;
+    return ipi_states[cpu_addr];
+}
+
+uint16_t s390_cpu_get_free_state_idx(void)
+{
+    int i;
+    for (i = 0; i < max_cpus; i++)
+        if (ipi_states[i] == NULL) {
+            return i;
     }
 
-    return ipi_states[cpu_addr];
+    assert(0); /* BUG! */
+    return -1;
 }
 
 static int s390_virtio_hcall_notify(const uint64_t *args)
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 6304c4d..029d0c5 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -388,6 +388,8 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
 }
 #endif
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
+void s390_cpu_set_state(uint16_t cpu_addr, S390CPU *state);
+uint16_t s390_cpu_get_free_state_idx(void);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 5/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Introduce post-cpu-init function

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

In preparation for treating cpus as devices we need to separate machine
initialization into two stages:
1. Initialization that needs to be done before cpu devices can be created.
2. Initialization that requires cpu devices to already be created.

This is accomplished by creating an optional post-cpu initialization function
for QEMUMachine.

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

diff --git a/include/hw/boards.h b/include/hw/boards.h
index fb7c6f1..ed427a1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -19,7 +19,7 @@ typedef struct QEMUMachineInitArgs {
 } QEMUMachineInitArgs;
 
 typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
-
+typedef void QEMUMachineInitPostCpusFunc(void);
 typedef void QEMUMachineResetFunc(void);
 
 typedef void QEMUMachineHotAddCPUFunc(const int64_t id, Error **errp);
@@ -29,6 +29,7 @@ typedef struct QEMUMachine {
     const char *alias;
     const char *desc;
     QEMUMachineInitFunc *init;
+    QEMUMachineInitPostCpusFunc *post_cpu_init;
     QEMUMachineResetFunc *reset;
     QEMUMachineHotAddCPUFunc *hot_add_cpu;
     BlockInterfaceType block_default_type;
diff --git a/vl.c b/vl.c
index 47ab45d..71e1e6d 100644
--- a/vl.c
+++ b/vl.c
@@ -4305,6 +4305,10 @@ int main(int argc, char **argv, char **envp)
                                  .cpu_model = cpu_model };
     machine->init(&args);
 
+    if (machine->post_cpu_init) {
+        machine->post_cpu_init();
+    }
+
     audio_init();
 
     cpu_synchronize_all_post_init();
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 5/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Introduce post-cpu-init function

[Andreas Färber]

Am 07.06.2013 19:28, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> In preparation for treating cpus as devices

CPUs *are* devices since multiple releases now, so this is badly put.

> we need to separate machine
> initialization into two stages:
> 1. Initialization that needs to be done before cpu devices can be created.
> 2. Initialization that requires cpu devices to already be created.
> 
> This is accomplished by creating an optional post-cpu initialization function
> for QEMUMachine.

Whatever you are using it for, this sounds wrong to me.

Machine init is supposed to use less code and more QOM infrastructure,
with a future goal of replacing most code with a config file
instantiating and wiring up devices.

And please don't forget to CC me on the next CPU series.

Regards,
Andreas

> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  include/hw/boards.h |    3 ++-
>  vl.c                |    4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index fb7c6f1..ed427a1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -19,7 +19,7 @@ typedef struct QEMUMachineInitArgs {
>  } QEMUMachineInitArgs;
>  
>  typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
> -
> +typedef void QEMUMachineInitPostCpusFunc(void);
>  typedef void QEMUMachineResetFunc(void);
>  
>  typedef void QEMUMachineHotAddCPUFunc(const int64_t id, Error **errp);
> @@ -29,6 +29,7 @@ typedef struct QEMUMachine {
>      const char *alias;
>      const char *desc;
>      QEMUMachineInitFunc *init;
> +    QEMUMachineInitPostCpusFunc *post_cpu_init;
>      QEMUMachineResetFunc *reset;
>      QEMUMachineHotAddCPUFunc *hot_add_cpu;
>      BlockInterfaceType block_default_type;
> diff --git a/vl.c b/vl.c
> index 47ab45d..71e1e6d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4305,6 +4305,10 @@ int main(int argc, char **argv, char **envp)
>                                   .cpu_model = cpu_model };
>      machine->init(&args);
>  
> +    if (machine->post_cpu_init) {
> +        machine->post_cpu_init();
> +    }
> +
>      audio_init();
>  
>      cpu_synchronize_all_post_init();

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 5/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Introduce post-cpu-init function

[Jason J. Herne]

On 06/08/2013 06:10 PM, Andreas Färber wrote:
> Am 07.06.2013 19:28, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> In preparation for treating cpus as devices
>
> CPUs *are* devices since multiple releases now, so this is badly put.
>
>> we need to separate machine
>> initialization into two stages:
>> 1. Initialization that needs to be done before cpu devices can be created.
>> 2. Initialization that requires cpu devices to already be created.
>>
>> This is accomplished by creating an optional post-cpu initialization function
>> for QEMUMachine.
>
> Whatever you are using it for, this sounds wrong to me.
>

The QEMUMachine->init() function (at least for S390) currently handles 
several tasks.  One of those tasks is the creation of cpus.  If we are 
switching to a new paradigm where QOM cpu devices are parsed and created 
in main() then QEMUMachine->init() will happen either before or after 
cpus are created.  This change is meant to split QEMUMachine->init() 
into two parts

1. Stuff that does not depend on cpu creation. Specifically, stuff that 
might be a dependency of cpu create, like allocating ipi_states.

2. Stuff that does depend on cpu creation.  Like 
vm_s390_enable_css_support() which requires CPU 0 to exist.


> Machine init is supposed to use less code and more QOM infrastructure,
> with a future goal of replacing most code with a config file
> instantiating and wiring up devices.
>

Duly noted.  I can have another look at the code.  Perhaps there is an 
easy place I can move the ipi_state initialization.  Also, perhaps there 
is a way to remove the cpu-0 dependency from 
vm_s390_enable_css_support().  Both of these changes would remove the 
need for the post_cpu_init function.

> And please don't forget to CC me on the next CPU series.
>

Sorry.  I had meant to CC you originally.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

[Qemu-devel] [PATCH 6/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Storage key Global Access

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

In preparation for treating cpus as devices we refactor the code such that cpu
initialization no longer relies on cpu 0 to obtain a pointer to the storage key
data.  This patch introduces global access to that data.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c |    5 ++---
 hw/s390x/s390-virtio.c     |   21 ++++++++++++++++-----
 hw/s390x/s390-virtio.h     |    2 +-
 target-s390x/cpu.h         |    3 +++
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index eb774d9..70bd858 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -65,7 +65,6 @@ static void ccw_init(QEMUMachineInitArgs *args)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     int shift = 0;
-    uint8_t *storage_keys;
     int ret;
     VirtualCssBus *css_bus;
 
@@ -94,10 +93,10 @@ static void ccw_init(QEMUMachineInitArgs *args)
     memory_region_add_subregion(sysmem, 0, ram);
 
     /* allocate storage keys */
-    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
+    s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
 
     /* init CPUs */
-    s390_init_cpus(args->cpu_model, storage_keys);
+    s390_init_cpus(args->cpu_model);
 
     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 ef4f1ae..4af2d86 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -136,6 +136,18 @@ static void s390_virtio_register_hcalls(void)
                                    s390_virtio_hcall_set_status);
 }
 
+static uint8_t *storage_keys;
+
+uint8_t *s390_get_storage_keys_p(void)
+{
+    return storage_keys;
+}
+
+void s390_set_storage_keys_p(uint8_t *keys_p)
+{
+    storage_keys = keys_p;
+}
+
 /*
  * The number of running CPUs. On s390 a shutdown is the state of all CPUs
  * being either stopped or disabled (for interrupts) waiting. We have to
@@ -189,7 +201,7 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
+void s390_init_cpus(const char *cpu_model)
 {
     int i;
 
@@ -209,7 +221,7 @@ void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
         ipi_states[i] = cpu;
         cs->halted = 1;
         cpu->env.exception_index = EXCP_HLT;
-        cpu->env.storage_keys = storage_keys;
+        cpu->env.storage_keys = s390_get_storage_keys_p();
     }
 }
 
@@ -244,7 +256,6 @@ static void s390_init(QEMUMachineInitArgs *args)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     int shift = 0;
-    uint8_t *storage_keys;
     void *virtio_region;
     hwaddr virtio_region_len;
     hwaddr virtio_region_start;
@@ -283,10 +294,10 @@ static void s390_init(QEMUMachineInitArgs *args)
                               virtio_region_len);
 
     /* allocate storage keys */
-    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
+    s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
 
     /* init CPUs */
-    s390_init_cpus(args->cpu_model, storage_keys);
+    s390_init_cpus(args->cpu_model);
 
     /* Create VirtIO network adapters */
     s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index 5c405e7..c1cb042 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -20,7 +20,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, uint8_t *storage_keys);
+void s390_init_cpus(const char *cpu_model);
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
                        const char *initrd_filename,
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 029d0c5..a4b87bf 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -393,6 +393,9 @@ uint16_t s390_cpu_get_free_state_idx(void);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
 
+uint8_t *s390_get_storage_keys_p(void);
+void s390_set_storage_keys_p(uint8_t *keys_p);
+
 /* service interrupts are floating therefore we must not pass an cpustate */
 void s390_sclp_extint(uint32_t parm);
 
-- 
1.7.10.4

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

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

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

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

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

Re: [Qemu-devel] [PATCH 7/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Infrastructure for Cpu Devices

[Andreas Färber]

Hi,

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

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

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

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

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

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

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

2) QEMUMachine::max_cpus

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

3) vCPU initialization hooks

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

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 7/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Infrastructure for Cpu Devices

[Jason J. Herne]

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

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

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

I agree. This patch has the desired outcome here.

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

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

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

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

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

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

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

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

>
> Regards,
> Andreas
>


-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH 7/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Infrastructure for Cpu Devices

[Igor Mammedov]

On Fri,  7 Jun 2013 13:28:06 -0400
"Jason J. Herne" <jjherne@us.ibm.com> wrote:

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

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

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

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

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

[Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Modify cpu initialization and QOM routines associated with s390-cpu such that
all cpus on S390 are now created via the QOM device creation code path.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c |   15 ++++++++++-----
 hw/s390x/s390-virtio.c     |   25 +++++--------------------
 hw/s390x/s390-virtio.h     |    2 +-
 include/qapi/qmp/qerror.h  |    3 +++
 qdev-monitor.c             |   17 +++++++++++++++++
 target-s390x/cpu.c         |   24 ++++++++++++++++++++++--
 6 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 70bd858..141adce 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
     /* allocate storage keys */
     s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
 
-    /* init CPUs */
-    s390_init_cpus(args->cpu_model);
+    s390_init_ipi_states();
 
-    if (kvm_enabled()) {
-        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
-    }
     /*
      * Create virtual css and set it as default so that non mcss-e
      * enabled guests only see virtio devices.
@@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
     s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw");
 }
 
+static void ccw_post_cpu_init(void)
+{
+    if (kvm_enabled()) {
+        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
+    }
+}
+
 static QEMUMachine ccw_machine = {
     .name = "s390-ccw-virtio",
     .alias = "s390-ccw",
     .desc = "VirtIO-ccw based S390 machine",
+    .cpu_device_str = "s390-cpu",
     .init = ccw_init,
+    .post_cpu_init = ccw_post_cpu_init,
     .block_default_type = IF_VIRTIO,
     .no_cdrom = 1,
     .no_floppy = 1,
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 4af2d86..069a187 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(const char *cpu_model)
+void s390_init_ipi_states(void)
 {
     int i;
 
-    if (cpu_model == NULL) {
-        cpu_model = "host";
-    }
-
-    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
-
-    for (i = 0; i < smp_cpus; i++) {
-        S390CPU *cpu;
-        CPUState *cs;
+    ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
 
-        cpu = cpu_s390x_init(cpu_model);
-        cs = CPU(cpu);
-
-        ipi_states[i] = cpu;
-        cs->halted = 1;
-        cpu->env.exception_index = EXCP_HLT;
-        cpu->env.storage_keys = s390_get_storage_keys_p();
+    for (i = 0; i < max_cpus; i++) {
+        ipi_states[i] = NULL;
     }
 }
 
-
 void s390_create_virtio_net(BusState *bus, const char *name)
 {
     int i;
@@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
     /* allocate storage keys */
     s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
 
-    /* init CPUs */
-    s390_init_cpus(args->cpu_model);
+    s390_init_ipi_states();
 
     /* Create VirtIO network adapters */
     s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index c1cb042..7b1ef9f 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -20,7 +20,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_ipi_states(void);
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
                        const char *initrd_filename,
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 6c0a18d..6627dc4 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -162,6 +162,9 @@ void assert_no_error(Error *err);
 #define QERR_KVM_MISSING_CAP \
     ERROR_CLASS_K_V_M_MISSING_CAP, "Using KVM without %s, %s unavailable"
 
+#define QERR_MAX_CPUS \
+    ERROR_CLASS_GENERIC_ERROR, "The maximum number of cpus has already been created for this guest"
+
 #define QERR_MIGRATION_ACTIVE \
     ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
 
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e54dbc2..a4adeb8 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -23,6 +23,9 @@
 #include "monitor/qdev.h"
 #include "qmp-commands.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/sysemu.h"
+#include "hw/boards.h"
+#include "sysemu/cpus.h"
 #include "qemu/config-file.h"
 
 /*
@@ -442,6 +445,14 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         return NULL;
     }
 
+    if (driver && current_machine &&
+        strcmp(driver, current_machine->cpu_device_str) == 0) {
+        if (smp_cpus == max_cpus) {
+            qerror_report(QERR_MAX_CPUS);
+            return NULL;
+        }
+    }
+
     k = DEVICE_CLASS(obj);
 
     /* find bus */
@@ -498,6 +509,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         qerror_report(QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
     }
+
+    if (driver && current_machine &&
+        strcmp(driver, current_machine->cpu_device_str) == 0) {
+        resume_all_vcpus();
+    }
+
     qdev->opts = opts;
     return qdev;
 }
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 23fe51f..8b92c9c 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -29,6 +29,8 @@
 #include "hw/hw.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
+#include "sysemu/sysemu.h"
+#include "hw/s390x/sclp.h"
 #endif
 
 #define CR0_RESET       0xE0UL
@@ -106,6 +108,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(CPU(cpu));
 
     scc->parent_realize(dev, errp);
+
+    cpu_synchronize_post_init(CPU(dev));
+    raise_irq_cpu_hotplug();
 }
 
 static void s390_cpu_initfn(Object *obj)
@@ -113,8 +118,9 @@ static void s390_cpu_initfn(Object *obj)
     CPUState *cs = CPU(obj);
     S390CPU *cpu = S390_CPU(obj);
     CPUS390XState *env = &cpu->env;
+    int cpu_num = s390_cpu_get_free_state_idx();
     static bool inited;
-    static int cpu_num = 0;
+
 #if !defined(CONFIG_USER_ONLY)
     struct tm tm;
 #endif
@@ -134,13 +140,20 @@ static void s390_cpu_initfn(Object *obj)
      * initial ipl */
     cs->halted = 1;
 #endif
-    env->cpu_num = cpu_num++;
+    s390_cpu_set_state(cpu_num, cpu);
+    cs->cpu_index = cpu_num;
+    env->cpu_num = cpu_num;
     env->ext_index = -1;
+    env->cpu_model_str = "host";
+    cpu->env.exception_index = EXCP_HLT;
+    cpu->env.storage_keys = s390_get_storage_keys_p();
 
     if (tcg_enabled() && !inited) {
         inited = true;
         s390x_translate_init();
     }
+
+    smp_cpus += 1;
 }
 
 static void s390_cpu_finalize(Object *obj)
@@ -152,6 +165,12 @@ static void s390_cpu_finalize(Object *obj)
 #endif
 }
 
+static int s390_cpu_unplug(DeviceState *dev)
+{
+    fprintf(stderr, "Removal of CPU devices is not supported.\n");
+    return -1;
+}
+
 static const VMStateDescription vmstate_s390_cpu = {
     .name = "cpu",
     .unmigratable = 1,
@@ -165,6 +184,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
 
     scc->parent_realize = dc->realize;
     dc->realize = s390_cpu_realizefn;
+    dc->unplug = s390_cpu_unplug;
 
     scc->parent_reset = cc->reset;
     cc->reset = s390_cpu_reset;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Andreas Färber]

Am 07.06.2013 19:28, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Modify cpu initialization and QOM routines associated with s390-cpu such that
> all cpus on S390 are now created via the QOM device creation code path.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c |   15 ++++++++++-----
>  hw/s390x/s390-virtio.c     |   25 +++++--------------------
>  hw/s390x/s390-virtio.h     |    2 +-
>  include/qapi/qmp/qerror.h  |    3 +++
>  qdev-monitor.c             |   17 +++++++++++++++++
>  target-s390x/cpu.c         |   24 ++++++++++++++++++++++--
>  6 files changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 70bd858..141adce 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
>      /* allocate storage keys */
>      s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
>  
> -    /* init CPUs */
> -    s390_init_cpus(args->cpu_model);
> +    s390_init_ipi_states();
>  
> -    if (kvm_enabled()) {
> -        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> -    }
>      /*
>       * Create virtual css and set it as default so that non mcss-e
>       * enabled guests only see virtio devices.
> @@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
>      s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw");
>  }
>  
> +static void ccw_post_cpu_init(void)
> +{
> +    if (kvm_enabled()) {
> +        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> +    }
> +}

Am I understanding correctly that all this is about differentiating one
call between the ccw and legacy machines?

Isn't there a machine-init-done Notifier that the ccw machine init could
register for?

What if CPU 0 were hot-unplugged? Would the capability need to be
re-enabled or will this remain a one-time task?

> +
>  static QEMUMachine ccw_machine = {
>      .name = "s390-ccw-virtio",
>      .alias = "s390-ccw",
>      .desc = "VirtIO-ccw based S390 machine",
> +    .cpu_device_str = "s390-cpu",

TYPE_S390_CPU would be safer than hardcoding, if we need to do this.

>      .init = ccw_init,
> +    .post_cpu_init = ccw_post_cpu_init,
>      .block_default_type = IF_VIRTIO,
>      .no_cdrom = 1,
>      .no_floppy = 1,
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 4af2d86..069a187 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
>      qdev_init_nofail(dev);
>  }
>  
> -void s390_init_cpus(const char *cpu_model)
> +void s390_init_ipi_states(void)
>  {
>      int i;
>  
> -    if (cpu_model == NULL) {
> -        cpu_model = "host";
> -    }
> -
> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> -
> -    for (i = 0; i < smp_cpus; i++) {
> -        S390CPU *cpu;
> -        CPUState *cs;
> +    ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
>  
> -        cpu = cpu_s390x_init(cpu_model);
> -        cs = CPU(cpu);
> -
> -        ipi_states[i] = cpu;
> -        cs->halted = 1;
> -        cpu->env.exception_index = EXCP_HLT;
> -        cpu->env.storage_keys = s390_get_storage_keys_p();
> +    for (i = 0; i < max_cpus; i++) {
> +        ipi_states[i] = NULL;
>      }
>  }
>  
> -

Whitespace change intentional?

>  void s390_create_virtio_net(BusState *bus, const char *name)
>  {
>      int i;
> @@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>      /* allocate storage keys */
>      s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
>  
> -    /* init CPUs */
> -    s390_init_cpus(args->cpu_model);
> +    s390_init_ipi_states();
>  
>      /* Create VirtIO network adapters */
>      s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");

So effectively you're ripping out support for -cpu arguments and
assuming that s390-cpu will stay the only available type - when we were
actually just waiting for you guys to sort out how you want your models
to be named, which I believe you wanted to coordinate with Linux?

I still don't understand why you want to deviate from all other
architectures here. -smp N is supposed to create N times -cpu, not N
times QEMUMachine::cpu_device_str.

> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index c1cb042..7b1ef9f 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -20,7 +20,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_ipi_states(void);
>  void s390_init_ipl_dev(const char *kernel_filename,
>                         const char *kernel_cmdline,
>                         const char *initrd_filename,
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 6c0a18d..6627dc4 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -162,6 +162,9 @@ void assert_no_error(Error *err);
>  #define QERR_KVM_MISSING_CAP \
>      ERROR_CLASS_K_V_M_MISSING_CAP, "Using KVM without %s, %s unavailable"
>  
> +#define QERR_MAX_CPUS \
> +    ERROR_CLASS_GENERIC_ERROR, "The maximum number of cpus has already been created for this guest"

Don't add QERR_* constants, use error_setg().

> +
>  #define QERR_MIGRATION_ACTIVE \
>      ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
>  
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index e54dbc2..a4adeb8 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -23,6 +23,9 @@
>  #include "monitor/qdev.h"
>  #include "qmp-commands.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/boards.h"
> +#include "sysemu/cpus.h"
>  #include "qemu/config-file.h"
>  
>  /*
> @@ -442,6 +445,14 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          return NULL;
>      }
>  
> +    if (driver && current_machine &&
> +        strcmp(driver, current_machine->cpu_device_str) == 0) {
> +        if (smp_cpus == max_cpus) {
> +            qerror_report(QERR_MAX_CPUS);

As mentioned on 7/8, this should best be handled on QOM realize level.
That way we get the check consistently and can pass on the error.

Also this hunk seems misplaced in this patch, not being s390-only code.

> +            return NULL;
> +        }
> +    }
> +
>      k = DEVICE_CLASS(obj);
>  
>      /* find bus */
> @@ -498,6 +509,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>          return NULL;
>      }
> +
> +    if (driver && current_machine &&
> +        strcmp(driver, current_machine->cpu_device_str) == 0) {
> +        resume_all_vcpus();
> +    }

Ditto, generic change.

Hasn't this been obsoleted? Hot-added CPUs get resumed in
qom/cpu.c:cpu_common_realizefn() now. And CPUs added with -device should
be resumed together with machine-created CPUs from what I recall.
If something doesn't work as expected, please explicitly say so, then we
can fix it in its own patch and optionally backport it.

> +
>      qdev->opts = opts;
>      return qdev;
>  }
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 23fe51f..8b92c9c 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -29,6 +29,8 @@
>  #include "hw/hw.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/s390x/sclp.h"
>  #endif
>  
>  #define CR0_RESET       0xE0UL
> @@ -106,6 +108,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      cpu_reset(CPU(cpu));
>  
>      scc->parent_realize(dev, errp);
> +
> +    cpu_synchronize_post_init(CPU(dev));

This is already done as part of parent_realize above, please drop.

> +    raise_irq_cpu_hotplug();

[FTR: introduced in 3/8]

Shouldn't this be conditional on DeviceState::hotplugged, just like
cpu_synchronize_post_init() in qom/cpu.c?

>  }
>  
>  static void s390_cpu_initfn(Object *obj)
> @@ -113,8 +118,9 @@ static void s390_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      S390CPU *cpu = S390_CPU(obj);
>      CPUS390XState *env = &cpu->env;
> +    int cpu_num = s390_cpu_get_free_state_idx();
>      static bool inited;
> -    static int cpu_num = 0;
> +
>  #if !defined(CONFIG_USER_ONLY)
>      struct tm tm;
>  #endif
> @@ -134,13 +140,20 @@ static void s390_cpu_initfn(Object *obj)
>       * initial ipl */
>      cs->halted = 1;
>  #endif
> -    env->cpu_num = cpu_num++;
> +    s390_cpu_set_state(cpu_num, cpu);

This function name is rather confusing here, can you find a more precise
name?

In fact, can't we replace the ipi_states[] array with QOM link<S390CPU>
properties? All we would be doing here is adding or setting a link from
/machine/cpu[cpu_num] (or so) to this instance.

> +    cs->cpu_index = cpu_num;

This is after cpu_exec_init(), so fiddling with cpu_index should be OK.

But perhaps you should override CPUClass::get_arch_id to return cpu_num
directly, for cpu_exists() / cpu-add? If this is about hot-remove then
we may need a more generic solution. At least I don't see you changing
any cpu_index-dependent code here.

> +    env->cpu_num = cpu_num;
>      env->ext_index = -1;

> +    env->cpu_model_str = "host";

This is unneeded, cpu_model_str is only used for linux-user, where your
-smp twiddling does not apply.

> +    cpu->env.exception_index = EXCP_HLT;
> +    cpu->env.storage_keys = s390_get_storage_keys_p();

Moved from s390_init_cpus(), fine.

>  
>      if (tcg_enabled() && !inited) {
>          inited = true;
>          s390x_translate_init();
>      }
> +
> +    smp_cpus += 1;

Won't we need some form of locking?

If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
not just in s390x code.

>  }
>  
>  static void s390_cpu_finalize(Object *obj)
> @@ -152,6 +165,12 @@ static void s390_cpu_finalize(Object *obj)
>  #endif
>  }
>  
> +static int s390_cpu_unplug(DeviceState *dev)
> +{
> +    fprintf(stderr, "Removal of CPU devices is not supported.\n");
> +    return -1;
> +}
> +
>  static const VMStateDescription vmstate_s390_cpu = {
>      .name = "cpu",
>      .unmigratable = 1,
> @@ -165,6 +184,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>  
>      scc->parent_realize = dc->realize;
>      dc->realize = s390_cpu_realizefn;
> +    dc->unplug = s390_cpu_unplug;
>  
>      scc->parent_reset = cc->reset;
>      cc->reset = s390_cpu_reset;

That we should do in generic code, too.

Plus I count 6x dc->unplug plus 4x k->unplug, so we should probably
enhance the function signature with an Error** to properly return the
error message, since printing it to stderr means it won't show up on the
monitor console. I'll prepare a patch.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Cornelia Huck]

On Sun, 09 Jun 2013 03:11:35 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 07.06.2013 19:28, schrieb Jason J. Herne:
> > From: "Jason J. Herne" <jjherne@us.ibm.com>
> > 
> > Modify cpu initialization and QOM routines associated with s390-cpu such that
> > all cpus on S390 are now created via the QOM device creation code path.
> > 
> > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> > ---
> >  hw/s390x/s390-virtio-ccw.c |   15 ++++++++++-----
> >  hw/s390x/s390-virtio.c     |   25 +++++--------------------
> >  hw/s390x/s390-virtio.h     |    2 +-
> >  include/qapi/qmp/qerror.h  |    3 +++
> >  qdev-monitor.c             |   17 +++++++++++++++++
> >  target-s390x/cpu.c         |   24 ++++++++++++++++++++++--
> >  6 files changed, 58 insertions(+), 28 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 70bd858..141adce 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
> >      /* allocate storage keys */
> >      s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
> >  
> > -    /* init CPUs */
> > -    s390_init_cpus(args->cpu_model);
> > +    s390_init_ipi_states();
> >  
> > -    if (kvm_enabled()) {
> > -        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> > -    }
> >      /*
> >       * Create virtual css and set it as default so that non mcss-e
> >       * enabled guests only see virtio devices.
> > @@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
> >      s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw");
> >  }
> >  
> > +static void ccw_post_cpu_init(void)
> > +{
> > +    if (kvm_enabled()) {
> > +        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> > +    }
> > +}
> 
> Am I understanding correctly that all this is about differentiating one
> call between the ccw and legacy machines?
> 
> Isn't there a machine-init-done Notifier that the ccw machine init could
> register for?

I wasn't aware of that, but it looks worth a try.

> 
> What if CPU 0 were hot-unplugged? Would the capability need to be
> re-enabled or will this remain a one-time task?

KVM_ENABLE_CAP is a vcpu ioctl, but we use it to enable a machine-wide
capability (which will stay enabled during the lifetime of the
machine). (It probably should be "any cpu" instead of "cpu 0", but
that's probably not the only place doing that.)

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Jason J. Herne]

On 06/08/2013 09:11 PM, Andreas Färber wrote:
> Am 07.06.2013 19:28, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Modify cpu initialization and QOM routines associated with s390-cpu such that
>> all cpus on S390 are now created via the QOM device creation code path.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c |   15 ++++++++++-----
>>   hw/s390x/s390-virtio.c     |   25 +++++--------------------
>>   hw/s390x/s390-virtio.h     |    2 +-
>>   include/qapi/qmp/qerror.h  |    3 +++
>>   qdev-monitor.c             |   17 +++++++++++++++++
>>   target-s390x/cpu.c         |   24 ++++++++++++++++++++++--
>>   6 files changed, 58 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 70bd858..141adce 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
>>       /* allocate storage keys */
>>       s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
>>
>> -    /* init CPUs */
>> -    s390_init_cpus(args->cpu_model);
>> +    s390_init_ipi_states();
>>
>> -    if (kvm_enabled()) {
>> -        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
>> -    }
>>       /*
>>        * Create virtual css and set it as default so that non mcss-e
>>        * enabled guests only see virtio devices.
>> @@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
>>       s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw");
>>   }
>>
>> +static void ccw_post_cpu_init(void)
>> +{
>> +    if (kvm_enabled()) {
>> +        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
>> +    }
>> +}
>
> Am I understanding correctly that all this is about differentiating one
> call between the ccw and legacy machines?
>

If you are referring to the post_cpu_init hook that I've added in a 
previous patch, then yes :). I get into more detail in a reply to the 
relevant patch but essentially kvm_s390_enable_css_support() currently 
depends on cpus being initialized.  My patch series moves cpu 
initialization out of machine->init() and places it in main().  This is 
done to allow cpus to be treated more like QOM devices which are all 
created in main().

> Isn't there a machine-init-done Notifier that the ccw machine init could
> register for?
>

If there is I am not aware of it.  I will investigate.

> What if CPU 0 were hot-unplugged? Would the capability need to be
> re-enabled or will this remain a one-time task?
>

Several places in S390 code refer to cpu 0 specifically.  When unplug is 
implemented we will need to check for and fail an attempt to unplug cpu 
0.  Else: We'll need to remove all dependencies on cpu-0.

>> +
>>   static QEMUMachine ccw_machine = {
>>       .name = "s390-ccw-virtio",
>>       .alias = "s390-ccw",
>>       .desc = "VirtIO-ccw based S390 machine",
>> +    .cpu_device_str = "s390-cpu",
>
> TYPE_S390_CPU would be safer than hardcoding, if we need to do this.
>
>>       .init = ccw_init,
>> +    .post_cpu_init = ccw_post_cpu_init,
>>       .block_default_type = IF_VIRTIO,
>>       .no_cdrom = 1,
>>       .no_floppy = 1,
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index 4af2d86..069a187 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
>>       qdev_init_nofail(dev);
>>   }
>>
>> -void s390_init_cpus(const char *cpu_model)
>> +void s390_init_ipi_states(void)
>>   {
>>       int i;
>>
>> -    if (cpu_model == NULL) {
>> -        cpu_model = "host";
>> -    }
>> -
>> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
>> -
>> -    for (i = 0; i < smp_cpus; i++) {
>> -        S390CPU *cpu;
>> -        CPUState *cs;
>> +    ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
>>
>> -        cpu = cpu_s390x_init(cpu_model);
>> -        cs = CPU(cpu);
>> -
>> -        ipi_states[i] = cpu;
>> -        cs->halted = 1;
>> -        cpu->env.exception_index = EXCP_HLT;
>> -        cpu->env.storage_keys = s390_get_storage_keys_p();
>> +    for (i = 0; i < max_cpus; i++) {
>> +        ipi_states[i] = NULL;
>>       }
>>   }
>>
>> -
>
> Whitespace change intentional?
>
>>   void s390_create_virtio_net(BusState *bus, const char *name)
>>   {
>>       int i;
>> @@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>>       /* allocate storage keys */
>>       s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
>>
>> -    /* init CPUs */
>> -    s390_init_cpus(args->cpu_model);
>> +    s390_init_ipi_states();
>>
>>       /* Create VirtIO network adapters */
>>       s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
>
> So effectively you're ripping out support for -cpu arguments and
> assuming that s390-cpu will stay the only available type - when we were
> actually just waiting for you guys to sort out how you want your models
> to be named, which I believe you wanted to coordinate with Linux?

Removing model support was not my intention.  I'll discuss this with my 
team and ensure we're all on the same page and headed down the right 
path with respect to models.

>
> I still don't understand why you want to deviate from all other
> architectures here. -smp N is supposed to create N times -cpu, not N
> times QEMUMachine::cpu_device_str.
>

It is not my intention to deviate from other architectures. 
cpu_device_str is used so we can detect when the user has given us a cpu 
device on the command line.  We convert -smp to -device statements using 
cpu_device_str so we can handle cpus a single way instead of two ways. 
Plus it greatly simplifies counting.  I'll investigate this situation 
further.


>> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
>> index c1cb042..7b1ef9f 100644
>> --- a/hw/s390x/s390-virtio.h
>> +++ b/hw/s390x/s390-virtio.h
>> @@ -20,7 +20,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_ipi_states(void);
>>   void s390_init_ipl_dev(const char *kernel_filename,
>>                          const char *kernel_cmdline,
>>                          const char *initrd_filename,
>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>> index 6c0a18d..6627dc4 100644
>> --- a/include/qapi/qmp/qerror.h
>> +++ b/include/qapi/qmp/qerror.h
>> @@ -162,6 +162,9 @@ void assert_no_error(Error *err);
>>   #define QERR_KVM_MISSING_CAP \
>>       ERROR_CLASS_K_V_M_MISSING_CAP, "Using KVM without %s, %s unavailable"
>>
>> +#define QERR_MAX_CPUS \
>> +    ERROR_CLASS_GENERIC_ERROR, "The maximum number of cpus has already been created for this guest"
>
> Don't add QERR_* constants, use error_setg().
>

Will change.

>> +
>>   #define QERR_MIGRATION_ACTIVE \
>>       ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index e54dbc2..a4adeb8 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -23,6 +23,9 @@
>>   #include "monitor/qdev.h"
>>   #include "qmp-commands.h"
>>   #include "sysemu/arch_init.h"
>> +#include "sysemu/sysemu.h"
>> +#include "hw/boards.h"
>> +#include "sysemu/cpus.h"
>>   #include "qemu/config-file.h"
>>
>>   /*
>> @@ -442,6 +445,14 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>           return NULL;
>>       }
>>
>> +    if (driver && current_machine &&
>> +        strcmp(driver, current_machine->cpu_device_str) == 0) {
>> +        if (smp_cpus == max_cpus) {
>> +            qerror_report(QERR_MAX_CPUS);
>
> As mentioned on 7/8, this should best be handled on QOM realize level.
> That way we get the check consistently and can pass on the error.
>
Yep.

> Also this hunk seems misplaced in this patch, not being s390-only code.
>
Agree.  I'll do a better job organizing the code for next submission.

>> +            return NULL;
>> +        }
>> +    }
>> +
>>       k = DEVICE_CLASS(obj);
>>
>>       /* find bus */
>> @@ -498,6 +509,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>           qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>>           return NULL;
>>       }
>> +
>> +    if (driver && current_machine &&
>> +        strcmp(driver, current_machine->cpu_device_str) == 0) {
>> +        resume_all_vcpus();
>> +    }
>
> Ditto, generic change.
>
> Hasn't this been obsoleted? Hot-added CPUs get resumed in
> qom/cpu.c:cpu_common_realizefn() now. And CPUs added with -device should
> be resumed together with machine-created CPUs from what I recall.
> If something doesn't work as expected, please explicitly say so, then we
> can fix it in its own patch and optionally backport it.
>

With this call removed, the guest hangs on a hotplug via device_add. 
That is about all I was able to determine about the situation.  Perhaps 
there is another way to solve the problem other than calling 
resume_all_vcpus()?

>> +
>>       qdev->opts = opts;
>>       return qdev;
>>   }
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 23fe51f..8b92c9c 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -29,6 +29,8 @@
>>   #include "hw/hw.h"
>>   #ifndef CONFIG_USER_ONLY
>>   #include "sysemu/arch_init.h"
>> +#include "sysemu/sysemu.h"
>> +#include "hw/s390x/sclp.h"
>>   #endif
>>
>>   #define CR0_RESET       0xE0UL
>> @@ -106,6 +108,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>>       cpu_reset(CPU(cpu));
>>
>>       scc->parent_realize(dev, errp);
>> +
>> +    cpu_synchronize_post_init(CPU(dev));
>
> This is already done as part of parent_realize above, please drop.
>

Ok.

>> +    raise_irq_cpu_hotplug();
>
> [FTR: introduced in 3/8]
>
> Shouldn't this be conditional on DeviceState::hotplugged, just like
> cpu_synchronize_post_init() in qom/cpu.c?
>

Yes! Thanks for pointing that out. I had not noticed that "qdev_hotplug" 
existed.  This can be used where ever the hotplug case must differ from 
the startup case.  Specifically here.

>>   }
>>
>>   static void s390_cpu_initfn(Object *obj)
>> @@ -113,8 +118,9 @@ static void s390_cpu_initfn(Object *obj)
>>       CPUState *cs = CPU(obj);
>>       S390CPU *cpu = S390_CPU(obj);
>>       CPUS390XState *env = &cpu->env;
>> +    int cpu_num = s390_cpu_get_free_state_idx();
>>       static bool inited;
>> -    static int cpu_num = 0;
>> +
>>   #if !defined(CONFIG_USER_ONLY)
>>       struct tm tm;
>>   #endif
>> @@ -134,13 +140,20 @@ static void s390_cpu_initfn(Object *obj)
>>        * initial ipl */
>>       cs->halted = 1;
>>   #endif
>> -    env->cpu_num = cpu_num++;
>> +    s390_cpu_set_state(cpu_num, cpu);
>
> This function name is rather confusing here, can you find a more precise
> name?
>
> In fact, can't we replace the ipi_states[] array with QOM link<S390CPU>
> properties? All we would be doing here is adding or setting a link from
> /machine/cpu[cpu_num] (or so) to this instance.
>

I'm not sure what is meant by "QOM link<S390CPU> properties?".  I assume 
you mean add a poperty to the s390-cpu QOM object to hold the S390CPU 
pointer?  If so, I completely agree.  That would make life easier.

>> +    cs->cpu_index = cpu_num;
>
> This is after cpu_exec_init(), so fiddling with cpu_index should be OK.
>
> But perhaps you should override CPUClass::get_arch_id to return cpu_num
> directly, for cpu_exists() / cpu-add? If this is about hot-remove then
> we may need a more generic solution. At least I don't see you changing
> any cpu_index-dependent code here.
>

I'm not 100% sure what cpu_index/cpu_num is used for or how they differ. 
  Could you elaborate on the change you are requesting?

>> +    env->cpu_num = cpu_num;
>>       env->ext_index = -1;
>
>> +    env->cpu_model_str = "host";
>
> This is unneeded, cpu_model_str is only used for linux-user, where your
> -smp twiddling does not apply.
>
I'm not sure I understand.  i386 architecture sets this in 
target-i386:cpu.c:cpu_x86_create().  Why would s390 not need to set it here?

>> +    cpu->env.exception_index = EXCP_HLT;
>> +    cpu->env.storage_keys = s390_get_storage_keys_p();
>
> Moved from s390_init_cpus(), fine.
>
>>
>>       if (tcg_enabled() && !inited) {
>>           inited = true;
>>           s390x_translate_init();
>>       }
>> +
>> +    smp_cpus += 1;
>
> Won't we need some form of locking?
>

Monitor commands are processed by the main qemu thread, right? if so, 
aren't they processed in a serial fashion?  Thereby eliminating the 
possibility of concurrent updates?

Hmmmm, I'm just noticing that x86 does not seem to update smp_cpus when 
processing a hot plug.  Perhaps updating smp_cpus is not required? 
Thoughts?

> If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
> not just in s390x code.
>
>>   }
>>
>>   static void s390_cpu_finalize(Object *obj)
>> @@ -152,6 +165,12 @@ static void s390_cpu_finalize(Object *obj)
>>   #endif
>>   }
>>
>> +static int s390_cpu_unplug(DeviceState *dev)
>> +{
>> +    fprintf(stderr, "Removal of CPU devices is not supported.\n");
>> +    return -1;
>> +}
>> +
>>   static const VMStateDescription vmstate_s390_cpu = {
>>       .name = "cpu",
>>       .unmigratable = 1,
>> @@ -165,6 +184,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>
>>       scc->parent_realize = dc->realize;
>>       dc->realize = s390_cpu_realizefn;
>> +    dc->unplug = s390_cpu_unplug;
>>
>>       scc->parent_reset = cc->reset;
>>       cc->reset = s390_cpu_reset;
>
> That we should do in generic code, too.
>
> Plus I count 6x dc->unplug plus 4x k->unplug, so we should probably
> enhance the function signature with an Error** to properly return the
> error message, since printing it to stderr means it won't show up on the
> monitor console. I'll prepare a patch.

Sounds good.  Andreas, thanks very much for your thorough review.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Jason J. Herne]

On 06/08/2013 09:11 PM, Andreas Färber wrote:
>>   if (tcg_enabled() && !inited) {
>> >          inited = true;
>> >          s390x_translate_init();
>> >      }
>> >+
>> >+    smp_cpus += 1;
> Won't we need some form of locking?
>
> If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
> not just in s390x code.
>

I've redesigned a lot of this to make it simpler and less intrusive. 
I'm almost ready to post the next revision but I'm hung up on this one 
thing.

I moved the smp_cpu increment to qom/cpu.c : cpu_common_realizefn. 
However this seems to break the user mode target because smp_cpus does 
not exist.  I tried wrapping the increment in a #ifndef CONFIG_USER_ONLY 
statement but it seems to have no effect.  I think the reason for that 
is because CONFIG_USER_ONLY is added to config-target.h which is not 
actually generated until after we compile qom/cpu.c.

...
   CC    qom/object.o
   CC    qom/container.o
   CC    qom/qom-qobject.o
   CC    qom/cpu.o
   CC    hw/core/qdev.o
   CC    hw/core/qdev-properties.o
   CC    hw/core/irq.o
   GEN   s390x-linux-user/config-target.h
   CC    s390x-linux-user/exec.o
...

Is there another place I should put the increment?


-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Igor Mammedov]

On Mon, 29 Jul 2013 15:41:57 -0400
"Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:

> On 06/08/2013 09:11 PM, Andreas Färber wrote:
> >>   if (tcg_enabled() && !inited) {
> >> >          inited = true;
> >> >          s390x_translate_init();
> >> >      }
> >> >+
> >> >+    smp_cpus += 1;
> > Won't we need some form of locking?
> >
> > If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
> > not just in s390x code.
> >
> 
> I've redesigned a lot of this to make it simpler and less intrusive. 
> I'm almost ready to post the next revision but I'm hung up on this one 
> thing.
> 
> I moved the smp_cpu increment to qom/cpu.c : cpu_common_realizefn. 
> However this seems to break the user mode target because smp_cpus does 
> not exist.  I tried wrapping the increment in a #ifndef CONFIG_USER_ONLY 
> statement but it seems to have no effect.  I think the reason for that 
> is because CONFIG_USER_ONLY is added to config-target.h which is not 
> actually generated until after we compile qom/cpu.c.
> 
> ...
>    CC    qom/object.o
>    CC    qom/container.o
>    CC    qom/qom-qobject.o
>    CC    qom/cpu.o
>    CC    hw/core/qdev.o
>    CC    hw/core/qdev-properties.o
>    CC    hw/core/irq.o
>    GEN   s390x-linux-user/config-target.h
>    CC    s390x-linux-user/exec.o
> ...
> 
> Is there another place I should put the increment?

Could you just use current number of cpus instead of smp_cpus increment?

 

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Jason J. Herne]

On 07/30/2013 03:24 AM, Igor Mammedov wrote:
> On Mon, 29 Jul 2013 15:41:57 -0400
> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
>
>> On 06/08/2013 09:11 PM, Andreas Färber wrote:
>>>>    if (tcg_enabled() && !inited) {
>>>>>           inited = true;
>>>>>           s390x_translate_init();
>>>>>       }
>>>>> +
>>>>> +    smp_cpus += 1;
>>> Won't we need some form of locking?
>>>
>>> If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
>>> not just in s390x code.
>>>
>>
>> I've redesigned a lot of this to make it simpler and less intrusive.
>> I'm almost ready to post the next revision but I'm hung up on this one
>> thing.
>>
>> I moved the smp_cpu increment to qom/cpu.c : cpu_common_realizefn.
>> However this seems to break the user mode target because smp_cpus does
>> not exist.  I tried wrapping the increment in a #ifndef CONFIG_USER_ONLY
>> statement but it seems to have no effect.  I think the reason for that
>> is because CONFIG_USER_ONLY is added to config-target.h which is not
>> actually generated until after we compile qom/cpu.c.
>>
>> ...
>>     CC    qom/object.o
>>     CC    qom/container.o
>>     CC    qom/qom-qobject.o
>>     CC    qom/cpu.o
>>     CC    hw/core/qdev.o
>>     CC    hw/core/qdev-properties.o
>>     CC    hw/core/irq.o
>>     GEN   s390x-linux-user/config-target.h
>>     CC    s390x-linux-user/exec.o
>> ...
>>
>> Is there another place I should put the increment?
>
> Could you just use current number of cpus instead of smp_cpus increment?
>

Is there an easier way of getting the count besides this?

int cpu_count = 0;
for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
     cpu_count++;
}




-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Igor Mammedov]

On Tue, 30 Jul 2013 10:27:26 -0400
"Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:

> On 07/30/2013 03:24 AM, Igor Mammedov wrote:
> > On Mon, 29 Jul 2013 15:41:57 -0400
> > "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
> >
> >> On 06/08/2013 09:11 PM, Andreas Färber wrote:
> >>>>    if (tcg_enabled() && !inited) {
> >>>>>           inited = true;
> >>>>>           s390x_translate_init();
> >>>>>       }
> >>>>> +
> >>>>> +    smp_cpus += 1;
> >>> Won't we need some form of locking?
> >>>
> >>> If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
> >>> not just in s390x code.
> >>>
> >>
> >> I've redesigned a lot of this to make it simpler and less intrusive.
> >> I'm almost ready to post the next revision but I'm hung up on this one
> >> thing.
> >>
> >> I moved the smp_cpu increment to qom/cpu.c : cpu_common_realizefn.
> >> However this seems to break the user mode target because smp_cpus does
> >> not exist.  I tried wrapping the increment in a #ifndef CONFIG_USER_ONLY
> >> statement but it seems to have no effect.  I think the reason for that
> >> is because CONFIG_USER_ONLY is added to config-target.h which is not
> >> actually generated until after we compile qom/cpu.c.
> >>
> >> ...
> >>     CC    qom/object.o
> >>     CC    qom/container.o
> >>     CC    qom/qom-qobject.o
> >>     CC    qom/cpu.o
> >>     CC    hw/core/qdev.o
> >>     CC    hw/core/qdev-properties.o
> >>     CC    hw/core/irq.o
> >>     GEN   s390x-linux-user/config-target.h
> >>     CC    s390x-linux-user/exec.o
> >> ...
> >>
> >> Is there another place I should put the increment?
> >
> > Could you just use current number of cpus instead of smp_cpus increment?
> >
> 
> Is there an easier way of getting the count besides this?
> 
> int cpu_count = 0;
> for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
>      cpu_count++;
> }

maybe qemu_for_each_cpu(), direct access to first_cpu & co is not encouraged.
 

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Andreas Färber]

Am 30.07.2013 16:50, schrieb Igor Mammedov:
> On Tue, 30 Jul 2013 10:27:26 -0400
> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
>> On 07/30/2013 03:24 AM, Igor Mammedov wrote:
>> Is there an easier way of getting the count besides this?
>>
>> int cpu_count = 0;
>> for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
>>      cpu_count++;
>> }
> 
> maybe qemu_for_each_cpu(), direct access to first_cpu & co is not encouraged.

Negative: qemu_for_each_cpu() is discouraged, first_cpu and next_cpu are
okay. I need to send out my patch introducing CPU_FOREACH() macro based
on QTAILQ_FOREACH(), preview on qom-cpu-12 branch.

Markus wanted to rip out qemu_for_each_cpu() - also on that branch - but
mst wanted both to coexist.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Igor Mammedov]

On Sun, 09 Jun 2013 03:11:35 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 07.06.2013 19:28, schrieb Jason J. Herne:
> > From: "Jason J. Herne" <jjherne@us.ibm.com>
> > 
> > Modify cpu initialization and QOM routines associated with s390-cpu such that
> > all cpus on S390 are now created via the QOM device creation code path.
> > 
> > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> > ---
> >  hw/s390x/s390-virtio-ccw.c |   15 ++++++++++-----
> >  hw/s390x/s390-virtio.c     |   25 +++++--------------------
> >  hw/s390x/s390-virtio.h     |    2 +-
> >  include/qapi/qmp/qerror.h  |    3 +++
> >  qdev-monitor.c             |   17 +++++++++++++++++
> >  target-s390x/cpu.c         |   24 ++++++++++++++++++++++--
> >  6 files changed, 58 insertions(+), 28 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 70bd858..141adce 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
> >      /* allocate storage keys */
> >      s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
> >  
> > -    /* init CPUs */
> > -    s390_init_cpus(args->cpu_model);
> > +    s390_init_ipi_states();
> >  
> > -    if (kvm_enabled()) {
> > -        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> > -    }
> >      /*
> >       * Create virtual css and set it as default so that non mcss-e
> >       * enabled guests only see virtio devices.
> > @@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
> >      s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw");
> >  }
> >  
> > +static void ccw_post_cpu_init(void)
> > +{
> > +    if (kvm_enabled()) {
> > +        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> > +    }
> > +}
> 
> Am I understanding correctly that all this is about differentiating one
> call between the ccw and legacy machines?
> 
> Isn't there a machine-init-done Notifier that the ccw machine init could
> register for?
> 
> What if CPU 0 were hot-unplugged? Would the capability need to be
> re-enabled or will this remain a one-time task?
> 
> > +
> >  static QEMUMachine ccw_machine = {
> >      .name = "s390-ccw-virtio",
> >      .alias = "s390-ccw",
> >      .desc = "VirtIO-ccw based S390 machine",
> > +    .cpu_device_str = "s390-cpu",
> 
> TYPE_S390_CPU would be safer than hardcoding, if we need to do this.
Similar change was rejected before for i386 target and as temporary solution
target specific cpu_add hook was added. But do it needed for s390 at all?

if s390 doesn't support legacy -cpu [+-]foo-feature format then
CPU subclasses + properties should do the job, at least it's where we
heading with i386 target.

> 
> >      .init = ccw_init,
> > +    .post_cpu_init = ccw_post_cpu_init,
> >      .block_default_type = IF_VIRTIO,
> >      .no_cdrom = 1,
> >      .no_floppy = 1,
> > diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> > index 4af2d86..069a187 100644
> > --- a/hw/s390x/s390-virtio.c
> > +++ b/hw/s390x/s390-virtio.c
> > @@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
> >      qdev_init_nofail(dev);
> >  }
> >  
> > -void s390_init_cpus(const char *cpu_model)
> > +void s390_init_ipi_states(void)
> >  {
> >      int i;
> >  
> > -    if (cpu_model == NULL) {
> > -        cpu_model = "host";
> > -    }
> > -
> > -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> > -
> > -    for (i = 0; i < smp_cpus; i++) {
> > -        S390CPU *cpu;
> > -        CPUState *cs;
> > +    ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
> >  
> > -        cpu = cpu_s390x_init(cpu_model);
> > -        cs = CPU(cpu);
> > -
> > -        ipi_states[i] = cpu;
> > -        cs->halted = 1;
> > -        cpu->env.exception_index = EXCP_HLT;
> > -        cpu->env.storage_keys = s390_get_storage_keys_p();
> > +    for (i = 0; i < max_cpus; i++) {
> > +        ipi_states[i] = NULL;
> >      }
> >  }
> >  
> > -
> 
> Whitespace change intentional?
> 
> >  void s390_create_virtio_net(BusState *bus, const char *name)
> >  {
> >      int i;
> > @@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
> >      /* allocate storage keys */
> >      s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
> >  
> > -    /* init CPUs */
> > -    s390_init_cpus(args->cpu_model);
> > +    s390_init_ipi_states();
> >  
> >      /* Create VirtIO network adapters */
> >      s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
> 
> So effectively you're ripping out support for -cpu arguments and
> assuming that s390-cpu will stay the only available type - when we were
> actually just waiting for you guys to sort out how you want your models
> to be named, which I believe you wanted to coordinate with Linux?
> 
> I still don't understand why you want to deviate from all other
> architectures here. -smp N is supposed to create N times -cpu, not N
> times QEMUMachine::cpu_device_str.
> 
> > diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> > index c1cb042..7b1ef9f 100644
> > --- a/hw/s390x/s390-virtio.h
> > +++ b/hw/s390x/s390-virtio.h
> > @@ -20,7 +20,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_ipi_states(void);
> >  void s390_init_ipl_dev(const char *kernel_filename,
> >                         const char *kernel_cmdline,
> >                         const char *initrd_filename,
> > diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> > index 6c0a18d..6627dc4 100644
> > --- a/include/qapi/qmp/qerror.h
> > +++ b/include/qapi/qmp/qerror.h
> > @@ -162,6 +162,9 @@ void assert_no_error(Error *err);
> >  #define QERR_KVM_MISSING_CAP \
> >      ERROR_CLASS_K_V_M_MISSING_CAP, "Using KVM without %s, %s unavailable"
> >  
> > +#define QERR_MAX_CPUS \
> > +    ERROR_CLASS_GENERIC_ERROR, "The maximum number of cpus has already been created for this guest"
> 
> Don't add QERR_* constants, use error_setg().
> 
> > +
> >  #define QERR_MIGRATION_ACTIVE \
> >      ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
> >  
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index e54dbc2..a4adeb8 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -23,6 +23,9 @@
> >  #include "monitor/qdev.h"
> >  #include "qmp-commands.h"
> >  #include "sysemu/arch_init.h"
> > +#include "sysemu/sysemu.h"
> > +#include "hw/boards.h"
> > +#include "sysemu/cpus.h"
> >  #include "qemu/config-file.h"
> >  
> >  /*
> > @@ -442,6 +445,14 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >          return NULL;
> >      }
> >  
> > +    if (driver && current_machine &&
> > +        strcmp(driver, current_machine->cpu_device_str) == 0) {
> > +        if (smp_cpus == max_cpus) {
> > +            qerror_report(QERR_MAX_CPUS);
> 
> As mentioned on 7/8, this should best be handled on QOM realize level.
> That way we get the check consistently and can pass on the error.
> 
> Also this hunk seems misplaced in this patch, not being s390-only code.
> 
> > +            return NULL;
> > +        }
> > +    }
> > +
> >      k = DEVICE_CLASS(obj);
> >  
> >      /* find bus */
> > @@ -498,6 +509,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
> >          return NULL;
> >      }
> > +
> > +    if (driver && current_machine &&
> > +        strcmp(driver, current_machine->cpu_device_str) == 0) {
> > +        resume_all_vcpus();
> > +    }
> 
> Ditto, generic change.
> 
> Hasn't this been obsoleted? Hot-added CPUs get resumed in
> qom/cpu.c:cpu_common_realizefn() now. And CPUs added with -device should
> be resumed together with machine-created CPUs from what I recall.
> If something doesn't work as expected, please explicitly say so, then we
> can fix it in its own patch and optionally backport it.
> 
> > +
> >      qdev->opts = opts;
> >      return qdev;
> >  }
> > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> > index 23fe51f..8b92c9c 100644
> > --- a/target-s390x/cpu.c
> > +++ b/target-s390x/cpu.c
> > @@ -29,6 +29,8 @@
> >  #include "hw/hw.h"
> >  #ifndef CONFIG_USER_ONLY
> >  #include "sysemu/arch_init.h"
> > +#include "sysemu/sysemu.h"
> > +#include "hw/s390x/sclp.h"
> >  #endif
> >  
> >  #define CR0_RESET       0xE0UL
> > @@ -106,6 +108,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
> >      cpu_reset(CPU(cpu));
> >  
> >      scc->parent_realize(dev, errp);
> > +
> > +    cpu_synchronize_post_init(CPU(dev));
> 
> This is already done as part of parent_realize above, please drop.
> 
> > +    raise_irq_cpu_hotplug();
> 
> [FTR: introduced in 3/8]
> 
> Shouldn't this be conditional on DeviceState::hotplugged, just like
> cpu_synchronize_post_init() in qom/cpu.c?
> 
> >  }
> >  
> >  static void s390_cpu_initfn(Object *obj)
> > @@ -113,8 +118,9 @@ static void s390_cpu_initfn(Object *obj)
> >      CPUState *cs = CPU(obj);
> >      S390CPU *cpu = S390_CPU(obj);
> >      CPUS390XState *env = &cpu->env;
> > +    int cpu_num = s390_cpu_get_free_state_idx();
> >      static bool inited;
> > -    static int cpu_num = 0;
> > +
> >  #if !defined(CONFIG_USER_ONLY)
> >      struct tm tm;
> >  #endif
> > @@ -134,13 +140,20 @@ static void s390_cpu_initfn(Object *obj)
> >       * initial ipl */
> >      cs->halted = 1;
> >  #endif
> > -    env->cpu_num = cpu_num++;
> > +    s390_cpu_set_state(cpu_num, cpu);
> 
> This function name is rather confusing here, can you find a more precise
> name?
> 
> In fact, can't we replace the ipi_states[] array with QOM link<S390CPU>
> properties? All we would be doing here is adding or setting a link from
> /machine/cpu[cpu_num] (or so) to this instance.
> 
> > +    cs->cpu_index = cpu_num;
> 
> This is after cpu_exec_init(), so fiddling with cpu_index should be OK.
cpu_index in cpu_exec_init() is used for registering CPU's vmstate.
vmstate from cpu_exec_init() should be moved to realize time, probably to
common CPU class.

> 
> But perhaps you should override CPUClass::get_arch_id to return cpu_num
> directly, for cpu_exists() / cpu-add? If this is about hot-remove then
> we may need a more generic solution. At least I don't see you changing
> any cpu_index-dependent code here.
> 
> > +    env->cpu_num = cpu_num;
> >      env->ext_index = -1;
> 
> > +    env->cpu_model_str = "host";
> 
> This is unneeded, cpu_model_str is only used for linux-user, where your
> -smp twiddling does not apply.
> 
> > +    cpu->env.exception_index = EXCP_HLT;
> > +    cpu->env.storage_keys = s390_get_storage_keys_p();
> 
> Moved from s390_init_cpus(), fine.
> 
> >  
> >      if (tcg_enabled() && !inited) {
> >          inited = true;
> >          s390x_translate_init();
> >      }
> > +
> > +    smp_cpus += 1;
> 
> Won't we need some form of locking?
> 
> If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
> not just in s390x code.
> 
> >  }
> >  
> >  static void s390_cpu_finalize(Object *obj)
> > @@ -152,6 +165,12 @@ static void s390_cpu_finalize(Object *obj)
> >  #endif
> >  }
> >  
> > +static int s390_cpu_unplug(DeviceState *dev)
> > +{
> > +    fprintf(stderr, "Removal of CPU devices is not supported.\n");
> > +    return -1;
> > +}
> > +
> >  static const VMStateDescription vmstate_s390_cpu = {
> >      .name = "cpu",
> >      .unmigratable = 1,
> > @@ -165,6 +184,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
> >  
> >      scc->parent_realize = dc->realize;
> >      dc->realize = s390_cpu_realizefn;
> > +    dc->unplug = s390_cpu_unplug;
> >  
> >      scc->parent_reset = cc->reset;
> >      cc->reset = s390_cpu_reset;
> 
> That we should do in generic code, too.
> 
> Plus I count 6x dc->unplug plus 4x k->unplug, so we should probably
> enhance the function signature with an Error** to properly return the
> error message, since printing it to stderr means it won't show up on the
> monitor console. I'll prepare a patch.
> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] [PATCH 0/8] [PATCH RFC v2] s390-qemu: cpu hotplug

[Christian Borntraeger]

On 07/06/13 19:27, Jason J. Herne wrote:
> Latest code for cpu Hotplug designed to be controled via the QOM infrastructure.
> cpu on S390 are treated as devices via a new platform independent
> infrastructure I designed to allow this "new way" to exist with the "old way"
> of representing cpus.
> 
> The Qemu command line now allows "-device s390-cpu" which will instantiate a 
> cpu device. This is additive to anything that might be specified on the -smp
> parameter.
> 
> Devices can be hot plugged via the monitor command "device_add s390-cpu".
> Hotplugged cpus are created in the configured state and can be used by the
> guest after the guest onlines the cpu by: 
> "echo 1 > /sys/bus/cpu/devices/cpuN/online"
> 
> Hot unplugging is currently not implemented by this code. 

Adding Viktor, Andreas.

Andreas, since we are the first ones going forward with cpu is a device (on a command level)
we still have to provide the old cpu_add  and query commands to make libvirt work regarding
cpu hotplugging.
Are you ok with having the x86 commands as wrappers around the new ones?

Christian