[Qemu-devel] [PATCH 0/8] [PATCH RFC v3] s390 cpu hotplug

[Qemu-devel] [PATCH 0/8] [PATCH RFC v3] s390 cpu hotplug

[Jason J. Herne]

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

Latest code for cpu Hotplug on S390 architecture.   This one is vastly simpler
than v2 as we have decided to avoid the command line specification 
of -device s390-cpu.

The last version can be found here:
http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg01183.html

There is also a patch in this series to add cpu-add to the Qemu monitor
interface.

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. 

Jason J. Herne (8):
  s390-qemu: cpu hotplug - Define New SCLP Codes
  s390-qemu: cpu hotplug - SCLP CPU Info
  s390-qemu: cpu hotplug - SCLP Event integration
  s390-qemu: cpu hotplug - Storage key global access
  s390-qemu: cpu hotplug - ipi_states enhancements
  s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug
  s390-qemu: cpu hotplug - Implement hot_add_cpu hook
  qemu-monitor: HMP cpu-add wrapper

 hmp-commands.hx                   |   13 ++++
 hmp.c                             |   10 ++++
 hmp.h                             |    1 +
 hw/s390x/Makefile.objs            |    2 +-
 hw/s390x/event-facility.c         |    7 +++
 hw/s390x/s390-virtio-ccw.c        |    8 ++-
 hw/s390x/s390-virtio.c            |   47 +++++++++------
 hw/s390x/s390-virtio.h            |    2 +-
 hw/s390x/sclp.c                   |   53 +++++++++++++++-
 hw/s390x/sclpcpu.c                |  120 +++++++++++++++++++++++++++++++++++++
 include/hw/s390x/event-facility.h |    3 +
 include/hw/s390x/sclp.h           |   41 +++++++++++++
 target-s390x/cpu.c                |   36 ++++++++++-
 target-s390x/cpu.h                |    7 +++
 target-s390x/helper.c             |   12 ++++
 15 files changed, 336 insertions(+), 26 deletions(-)
 create mode 100644 hw/s390x/sclpcpu.c

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/8] [PATCH RFC v3] 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

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

[Alexander Graf]

On 01.08.2013, at 16:12, Jason J. Herne wrote:

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

switch (code & ~SCLP_CMD_PARM)

Or are the upper bits parm as well? In fact, what about the upper 32 bits?


Alex

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

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

[Christian Borntraeger]

On 05/09/13 13:25, Alexander Graf wrote:
> 
> On 01.08.2013, at 16:12, Jason J. Herne wrote:
> 
>> 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) {
> 
> switch (code & ~SCLP_CMD_PARM)
> 
> Or are the upper bits parm as well? In fact, what about the upper 32 bits?

As of now those are ignored by the sclp. So (code & SCLP_NO_CMD_PARM) seems
better to me.

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

[Jason J. Herne]

On 09/16/2013 09:53 AM, Christian Borntraeger wrote:
> On 05/09/13 13:25, Alexander Graf wrote:
>>
>> On 01.08.2013, at 16:12, Jason J. Herne wrote:
>>
>>> 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) {
>>
>> switch (code & ~SCLP_CMD_PARM)
>>
>> Or are the upper bits parm as well? In fact, what about the upper 32 bits?
>
> As of now those are ignored by the sclp. So (code & SCLP_NO_CMD_PARM) seems
> better to me.
>
>
>
>

What if I rename it to SCLP_CMD_CODE_MASK?  This removes the negative 
from the name and keeps the same semantics.

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

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

[Alexander Graf]

Am 16.09.2013 um 09:29 schrieb "Jason J. Herne" <jjherne@linux.vnet.ibm.com>:

> On 09/16/2013 09:53 AM, Christian Borntraeger wrote:
>> On 05/09/13 13:25, Alexander Graf wrote:
>>> 
>>> On 01.08.2013, at 16:12, Jason J. Herne wrote:
>>> 
>>>> 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) {
>>> 
>>> switch (code & ~SCLP_CMD_PARM)
>>> 
>>> Or are the upper bits parm as well? In fact, what about the upper 32 bits?
>> 
>> As of now those are ignored by the sclp. So (code & SCLP_NO_CMD_PARM) seems
>> better to me.
> 
> What if I rename it to SCLP_CMD_CODE_MASK?  This removes the negative from the name and keeps the same semantics.

Does the mask include payload information as well or only the cmd code?

Alex

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

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

[Jason J. Herne]

On 09/16/2013 10:43 AM, Alexander Graf wrote:
>
>
> Am 16.09.2013 um 09:29 schrieb "Jason J. Herne" <jjherne@linux.vnet.ibm.com>:
>
>> On 09/16/2013 09:53 AM, Christian Borntraeger wrote:
>>> On 05/09/13 13:25, Alexander Graf wrote:
>>>>
>>>> On 01.08.2013, at 16:12, Jason J. Herne wrote:
>>>>
>>>>> 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) {
>>>>
>>>> switch (code & ~SCLP_CMD_PARM)
>>>>
>>>> Or are the upper bits parm as well? In fact, what about the upper 32 bits?
>>>
>>> As of now those are ignored by the sclp. So (code & SCLP_NO_CMD_PARM) seems
>>> better to me.
>>
>> What if I rename it to SCLP_CMD_CODE_MASK?  This removes the negative from the name and keeps the same semantics.
>
> Does the mask include payload information as well or only the cmd code?
>

It contains all elements of the command code and nothing more.

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


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

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

[Andreas Färber]

Am 01.08.2013 16:12, schrieb 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>

"s390-qemu:" is really bad. For one, all QEMU patches are somehow about
QEMU, so that's redundant. For another, "sclp:" would be much more
telling to me which patches to look at than text disappearing at the end
of the subject line in the mail client. :)

Andreas

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

[Qemu-devel] [PATCH 2/8] [PATCH RFC v3] 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         |   51 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/sclp.h |   32 +++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index cb53d7e..da8cf7a 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"
 
@@ -31,7 +32,26 @@ static inline S390SCLPDevice *get_event_facility(void)
 static void read_SCP_info(SCCB *sccb)
 {
     ReadInfo *read_info = (ReadInfo *) sccb;
+    CPUState *cpu;
     int shift = 0;
+    int cpu_count = 0;
+    int i = 0;
+
+    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
+        cpu_count++;
+    }
+
+    /* CPU information */
+    read_info->entries_cpu = cpu_to_be16(cpu_count);
+    read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
+    read_info->highest_cpu = cpu_to_be16(max_cpus);
+
+    for (i = 0; i < cpu_count; 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 +61,34 @@ static void read_SCP_info(SCCB *sccb)
     sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
 }
 
+/* Provide information about the CPU */
+static void sclp_read_cpu_info(SCCB *sccb)
+{
+    ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
+    CPUState *cpu;
+    int cpu_count = 0;
+    int i = 0;
+
+    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
+        cpu_count++;
+    }
+
+    cpu_info->nr_configured = cpu_to_be16(cpu_count);
+    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 < cpu_count; 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 +98,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..89ae7d1 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -79,12 +79,44 @@ 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[24 - 20];       /* 20-23 */
+    uint8_t  loadparm[8];               /* 24-31 */
+    uint8_t  _reserved3[48 - 32];       /* 32-47 */
+    uint64_t facilities;                /* 48-55 */
+    uint8_t  _reserved0[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

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

[Andreas Färber]

Am 01.08.2013 16:12, schrieb 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         |   51 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/sclp.h |   32 +++++++++++++++++++++++++++++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index cb53d7e..da8cf7a 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"
>  
> @@ -31,7 +32,26 @@ static inline S390SCLPDevice *get_event_facility(void)
>  static void read_SCP_info(SCCB *sccb)
>  {
>      ReadInfo *read_info = (ReadInfo *) sccb;
> +    CPUState *cpu;
>      int shift = 0;
> +    int cpu_count = 0;
> +    int i = 0;
> +
> +    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
> +        cpu_count++;
> +    }
> +
> +    /* CPU information */
> +    read_info->entries_cpu = cpu_to_be16(cpu_count);
> +    read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
> +    read_info->highest_cpu = cpu_to_be16(max_cpus);
> +
> +    for (i = 0; i < cpu_count; 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 +61,34 @@ static void read_SCP_info(SCCB *sccb)
>      sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
>  }
>  
> +/* Provide information about the CPU */
> +static void sclp_read_cpu_info(SCCB *sccb)
> +{
> +    ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
> +    CPUState *cpu;
> +    int cpu_count = 0;
> +    int i = 0;
> +
> +    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {

This becomes CPU_FOREACH(cpu) { now.

> +        cpu_count++;
> +    }
> +
> +    cpu_info->nr_configured = cpu_to_be16(cpu_count);
> +    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 < cpu_count; 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 +98,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..89ae7d1 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -79,12 +79,44 @@ 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;

Feel free to use CPUEntry capitalization if this is not copied from a
Linux struct of that name - your choice.

Andreas

> +
>  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[24 - 20];       /* 20-23 */
> +    uint8_t  loadparm[8];               /* 24-31 */
> +    uint8_t  _reserved3[48 - 32];       /* 32-47 */
> +    uint64_t facilities;                /* 48-55 */
> +    uint8_t  _reserved0[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];
> 


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

[Qemu-devel] [PATCH 3/8] [PATCH RFC v3] 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                |  120 +++++++++++++++++++++++++++++++++++++
 include/hw/s390x/event-facility.h |    3 +
 include/hw/s390x/sclp.h           |    1 +
 5 files changed, 132 insertions(+), 1 deletion(-)
 create mode 100644 hw/s390x/sclpcpu.c

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 77e1218..104ae8e 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..5b4139e
--- /dev/null
+++ b/hw/s390x/sclpcpu.c
@@ -0,0 +1,120 @@
+/*
+ * 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 89ae7d1..7728ad8 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -154,5 +154,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

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

[Andreas Färber]

Am 01.08.2013 16:12, schrieb 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                |  120 +++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/event-facility.h |    3 +
>  include/hw/s390x/sclp.h           |    1 +
>  5 files changed, 132 insertions(+), 1 deletion(-)
>  create mode 100644 hw/s390x/sclpcpu.c
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 77e1218..104ae8e 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

On a line of its own for consistency and to avoid '-' line?

>  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");

Please don't create devices in such an init function. Also don't access
.qbus please.

Instead, please use object_initialize() followed by
qdev_set_parent_bus() in an instance_init function.

Conversion of the initfn to a realizefn will be a bit more involved so I
won't ask that for this series, but if there were volunteers among your
colleagues that would be appreciated. The effect would be to propagate
errors to the caller of the realizefn rather than asserting here.

> +    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..5b4139e
> --- /dev/null
> +++ b/hw/s390x/sclpcpu.c
> @@ -0,0 +1,120 @@
> +/*
> + * 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>

"hw/qdev.h", but I'm guessing that's redundant with either sclp.h or
event-facility.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 = {

static const

> +    .name          = "sclpcpuhotplug",

Please use dashes rather than concatenation.

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

Trailing white line.

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

Ah, copy&paste and answer to the above. ;)

>  #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 89ae7d1..7728ad8 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -154,5 +154,6 @@ typedef struct S390SCLPDeviceClass {
>  
>  void s390_sclp_init(void);
>  void sclp_service_interrupt(uint32_t sccb);
> +void raise_irq_cpu_hotplug(void);
>  
>  #endif
> 

Andreas

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

[Qemu-devel] [PATCH 4/8] [PATCH RFC v3] s390-qemu: cpu hotplug - Storage key global access

[Jason J. Herne]

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

Introduces global access to storage key data so we can set it for each cpu in
the S390 cpu initialization routine.

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         |    4 ++++
 4 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index aebbbf1..b469960 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_alloc_storage_keys(my_ram_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 439d732..21e9124 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -123,6 +123,18 @@ static void s390_virtio_register_hcalls(void)
                                    s390_virtio_hcall_set_status);
 }
 
+static uint8_t *storage_keys;
+
+uint8_t *s390_get_storage_keys(void)
+{
+    return storage_keys;
+}
+
+void s390_alloc_storage_keys(ram_addr_t ram_size)
+{
+    storage_keys = g_malloc0(ram_size / TARGET_PAGE_SIZE);
+}
+
 /*
  * 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
@@ -176,7 +188,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;
 
@@ -196,7 +208,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();
     }
 }
 
@@ -231,7 +243,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;
@@ -270,10 +281,10 @@ static void s390_init(QEMUMachineInitArgs *args)
                               virtio_region_len);
 
     /* allocate storage keys */
-    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
+    s390_alloc_storage_keys(my_ram_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 65bef86..877eac7 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -374,6 +374,10 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
 {
 }
 #endif
+
+uint8_t *s390_get_storage_keys(void);
+void s390_alloc_storage_keys(ram_addr_t ram_size);
+
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/8] [PATCH RFC v3] s390-qemu: cpu hotplug - Storage key global access

[Andreas Färber]

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Introduces global access to storage key data so we can set it for each cpu in
> the S390 cpu initialization routine.
> 
> 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         |    4 ++++
>  4 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index aebbbf1..b469960 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_alloc_storage_keys(my_ram_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 439d732..21e9124 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -123,6 +123,18 @@ static void s390_virtio_register_hcalls(void)
>                                     s390_virtio_hcall_set_status);
>  }
>  
> +static uint8_t *storage_keys;
> +
> +uint8_t *s390_get_storage_keys(void)
> +{
> +    return storage_keys;
> +}
> +
> +void s390_alloc_storage_keys(ram_addr_t ram_size)
> +{
> +    storage_keys = g_malloc0(ram_size / TARGET_PAGE_SIZE);
> +}
> +
>  /*
>   * 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
> @@ -176,7 +188,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;
>  
> @@ -196,7 +208,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();

Why not go this from the CPU initfn? Is there any ccw- vs. non-ccw
difference? Thinking about -device s390-cpu here. I believe it's safe to
assume that machine init and thus allocation has run before the CPU is
instantiated - possibly assert that.

Andreas

>      }
>  }
>  
> @@ -231,7 +243,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;
> @@ -270,10 +281,10 @@ static void s390_init(QEMUMachineInitArgs *args)
>                                virtio_region_len);
>  
>      /* allocate storage keys */
> -    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
> +    s390_alloc_storage_keys(my_ram_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 65bef86..877eac7 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -374,6 +374,10 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
>  {
>  }
>  #endif
> +
> +uint8_t *s390_get_storage_keys(void);
> +void s390_alloc_storage_keys(ram_addr_t ram_size);
> +
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  void s390_add_running_cpu(S390CPU *cpu);
>  unsigned s390_del_running_cpu(S390CPU *cpu);
> 


-- 
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 4/8] [PATCH RFC v3] s390-qemu: cpu hotplug - Storage key global access

[Jason J. Herne]

On 09/05/2013 07:46 AM, Andreas Färber wrote:
> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index 439d732..21e9124 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -123,6 +123,18 @@ static void s390_virtio_register_hcalls(void)
>>                                      s390_virtio_hcall_set_status);
>>   }
>>
>> +static uint8_t *storage_keys;
>> +
>> +uint8_t *s390_get_storage_keys(void)
>> +{
>> +    return storage_keys;
>> +}
>> +
>> +void s390_alloc_storage_keys(ram_addr_t ram_size)
>> +{
>> +    storage_keys = g_malloc0(ram_size / TARGET_PAGE_SIZE);
>> +}
>> +
>>   /*
>>    * 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
>> @@ -176,7 +188,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;
>>
>> @@ -196,7 +208,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();
>
> Why not go this from the CPU initfn? Is there any ccw- vs. non-ccw
> difference? Thinking about -device s390-cpu here. I believe it's safe to
> assume that machine init and thus allocation has run before the CPU is
> instantiated - possibly assert that.
>
>

Patch 6/8 does exactly that.

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

Re: [Qemu-devel] [PATCH 4/8] [PATCH RFC v3] s390-qemu: cpu hotplug - Storage key global access

[Alexander Graf]

On 01.08.2013, at 16:12, Jason J. Herne wrote:

> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Introduces global access to storage key data so we can set it for each cpu in
> the S390 cpu initialization routine.
> 
> 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         |    4 ++++
> 4 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index aebbbf1..b469960 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_alloc_storage_keys(my_ram_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 439d732..21e9124 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -123,6 +123,18 @@ static void s390_virtio_register_hcalls(void)
>                                    s390_virtio_hcall_set_status);
> }
> 
> +static uint8_t *storage_keys;
> +
> +uint8_t *s390_get_storage_keys(void)
> +{
> +    return storage_keys;

This is basically the same as having a global in my point of view. I would personally just make it a global and call it a day.

However, it might make sense to make storage keys be a device. That way you get direct access for free (you can ask for the device by-path) and you have somewhere to hook migration off of.


Alex

> +}
> +
> +void s390_alloc_storage_keys(ram_addr_t ram_size)
> +{
> +    storage_keys = g_malloc0(ram_size / TARGET_PAGE_SIZE);
> +}
> +
> /*
>  * 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
> @@ -176,7 +188,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;
> 
> @@ -196,7 +208,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();
>     }
> }
> 
> @@ -231,7 +243,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;
> @@ -270,10 +281,10 @@ static void s390_init(QEMUMachineInitArgs *args)
>                               virtio_region_len);
> 
>     /* allocate storage keys */
> -    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
> +    s390_alloc_storage_keys(my_ram_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 65bef86..877eac7 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -374,6 +374,10 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
> {
> }
> #endif
> +
> +uint8_t *s390_get_storage_keys(void);
> +void s390_alloc_storage_keys(ram_addr_t ram_size);
> +
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> 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 v3] 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 smp_cpus.  Hotplug requires this capability.

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

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

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 21e9124..5ad9cf3 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -54,12 +54,13 @@
 static VirtIOS390Bus *s390_bus;
 static S390CPU **ipi_states;
 
-S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
 {
-    if (cpu_addr >= smp_cpus) {
-        return NULL;
-    }
+    ipi_states[cpu_addr] = state;
+}
 
+S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
+{
     return ipi_states[cpu_addr];
 }
 
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 877eac7..62eb810 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -377,7 +377,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
 
 uint8_t *s390_get_storage_keys(void);
 void s390_alloc_storage_keys(ram_addr_t ram_size);
-
+void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state);
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 5/8] [PATCH RFC v3] s390-qemu: cpu hotplug - ipi_states enhancements

[Andreas Färber]

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Modify s390_cpu_addr2state to allow fetching state information for cpu addresses
> above smp_cpus.  Hotplug requires this capability.
> 
> Also add s390_cpu_set_state function to allow modification of ipi_state entries
> during hotplug.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  hw/s390x/s390-virtio.c |    9 +++++----
>  target-s390x/cpu.h     |    2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 21e9124..5ad9cf3 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -54,12 +54,13 @@
>  static VirtIOS390Bus *s390_bus;
>  static S390CPU **ipi_states;
>  
> -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
>  {
> -    if (cpu_addr >= smp_cpus) {
> -        return NULL;
> -    }
> +    ipi_states[cpu_addr] = state;
> +}
>  
> +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> +{
>      return ipi_states[cpu_addr];
>  }
>  

This is what got us into the link<> discussion last time. If we do

for (i = 0; i < ARRAY_SIZE(ipi_states); i++) {
    name = g_strdup_printf("cpu[%i]", i);
    object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
                             &ipi_states[i], &err);
}

then we get said /machine/cpu[n] link<> properties, at a QMP level
either returning nothing or the canonical path to the CPU object.

On IRC I didn't get an answer of whether it was being done the above way
because there is infrastructure missing, and a look at object.h now
confirms that suspicion. CC'ing Anthony and Paolo.

Since object_property_add_link() uses a NULL opaque, my idea would be to
add a single setter hook argument passed through as opaque to
object_set_link_property(), which would call it with the old and the new
value.

The purpose would be to avoid growing our own internal setter API, which
is disjoint from the QMP qom-set we are targetting at.

Regards,
Andreas

> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 877eac7..62eb810 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -377,7 +377,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
>  
>  uint8_t *s390_get_storage_keys(void);
>  void s390_alloc_storage_keys(ram_addr_t ram_size);
> -
> +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state);
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  void s390_add_running_cpu(S390CPU *cpu);
>  unsigned s390_del_running_cpu(S390CPU *cpu);
> 


-- 
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 v3] s390-qemu: cpu hotplug - ipi_states enhancements

[Jason J. Herne]

On 09/05/2013 08:01 AM, Andreas Färber wrote:
> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Modify s390_cpu_addr2state to allow fetching state information for cpu addresses
>> above smp_cpus.  Hotplug requires this capability.
>>
>> Also add s390_cpu_set_state function to allow modification of ipi_state entries
>> during hotplug.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>> ---
>>   hw/s390x/s390-virtio.c |    9 +++++----
>>   target-s390x/cpu.h     |    2 +-
>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index 21e9124..5ad9cf3 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -54,12 +54,13 @@
>>   static VirtIOS390Bus *s390_bus;
>>   static S390CPU **ipi_states;
>>
>> -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
>>   {
>> -    if (cpu_addr >= smp_cpus) {
>> -        return NULL;
>> -    }
>> +    ipi_states[cpu_addr] = state;
>> +}
>>
>> +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> +{
>>       return ipi_states[cpu_addr];
>>   }
>>
>
> This is what got us into the link<> discussion last time. If we do
>
> for (i = 0; i < ARRAY_SIZE(ipi_states); i++) {
>      name = g_strdup_printf("cpu[%i]", i);
>      object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
>                               &ipi_states[i], &err);
> }
>
> then we get said /machine/cpu[n] link<> properties, at a QMP level
> either returning nothing or the canonical path to the CPU object.
>
> On IRC I didn't get an answer of whether it was being done the above way
> because there is infrastructure missing, and a look at object.h now
> confirms that suspicion. CC'ing Anthony and Paolo.
>
> Since object_property_add_link() uses a NULL opaque, my idea would be to
> add a single setter hook argument passed through as opaque to
> object_set_link_property(), which would call it with the old and the new
> value.
>
> The purpose would be to avoid growing our own internal setter API, which
> is disjoint from the QMP qom-set we are targetting at.
>

Ok, you lost me :).  I must admit my understanding of QOM is still 
limited. Sorry for not keeping up.  Why do we need this new hook?  The 
link would contain a pointer to the correct ipi_states entry right? 
What are we using opaque for?

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

Re: [Qemu-devel] [PATCH 5/8] [PATCH RFC v3] s390-qemu: cpu hotplug - ipi_states enhancements

[Jason J. Herne]

On 09/05/2013 08:01 AM, Andreas Färber wrote:
> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
...
>
> This is what got us into the link<> discussion last time. If we do
>
> for (i = 0; i < ARRAY_SIZE(ipi_states); i++) {
>      name = g_strdup_printf("cpu[%i]", i);
>      object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
>                               &ipi_states[i], &err);
> }
>
> then we get said /machine/cpu[n] link<> properties, at a QMP level
> either returning nothing or the canonical path to the CPU object.
>
> On IRC I didn't get an answer of whether it was being done the above way
> because there is infrastructure missing, and a look at object.h now
> confirms that suspicion. CC'ing Anthony and Paolo.
>
> Since object_property_add_link() uses a NULL opaque, my idea would be to
> add a single setter hook argument passed through as opaque to
> object_set_link_property(), which would call it with the old and the new
> value.
>
> The purpose would be to avoid growing our own internal setter API, which
> is disjoint from the QMP qom-set we are targetting at.

I wrote the code, very close to how you suggested and it appears to be 
working when tested with qom-list.  I'm still not certain why we need 
the ability to set the opaque of object_set_link_property()?

For reference here is what I did:

void s390_init_cpus(const char *cpu_model)
{
     int i;
     char* name;

     if (cpu_model == NULL) {
         cpu_model = "host";
     }

     ipi_states = g_malloc0(sizeof(S390CPU *) * max_cpus);

     for (i = 0; i < max_cpus; i++) {
         name = g_strdup_printf("cpu[%i]", i);
         object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
                                  (Object **)&ipi_states[i], NULL);
     }

     for (i = 0; i < smp_cpus; i++) {
         cpu_s390x_init(cpu_model);
     }
}

Yep, I know cpu_model is going away ;).

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

Re: [Qemu-devel] [PATCH 5/8] [PATCH RFC v3] s390-qemu: cpu hotplug - ipi_states enhancements

[Michael Mueller]

On Thu, 19 Sep 2013 16:19:46 -0400
"Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:

> On 09/05/2013 08:01 AM, Andreas Färber wrote:
> > Am 01.08.2013 16:12, schrieb Jason J. Herne:
> >> From: "Jason J. Herne" <jjherne@us.ibm.com>
> >>
> ...
> >
> > This is what got us into the link<> discussion last time. If we do
> >
> > for (i = 0; i < ARRAY_SIZE(ipi_states); i++) {
> >      name = g_strdup_printf("cpu[%i]", i);
> >      object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
> >                               &ipi_states[i], &err);
> > }
> >
> > then we get said /machine/cpu[n] link<> properties, at a QMP level
> > either returning nothing or the canonical path to the CPU object.
> >
> > On IRC I didn't get an answer of whether it was being done the above way
> > because there is infrastructure missing, and a look at object.h now
> > confirms that suspicion. CC'ing Anthony and Paolo.
> >
> > Since object_property_add_link() uses a NULL opaque, my idea would be to
> > add a single setter hook argument passed through as opaque to
> > object_set_link_property(), which would call it with the old and the new
> > value.
> >
> > The purpose would be to avoid growing our own internal setter API, which
> > is disjoint from the QMP qom-set we are targetting at.
> 
> I wrote the code, very close to how you suggested and it appears to be 
> working when tested with qom-list.  I'm still not certain why we need 
> the ability to set the opaque of object_set_link_property()?
> 
> For reference here is what I did:
> 
> void s390_init_cpus(const char *cpu_model)
> {
>      int i;
>      char* name;
> 
>      if (cpu_model == NULL) {
>          cpu_model = "host";
>      }
> 
>      ipi_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
> 
>      for (i = 0; i < max_cpus; i++) {
>          name = g_strdup_printf("cpu[%i]", i);
>          object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
>                                   (Object **)&ipi_states[i], NULL);
>      }
> 
>      for (i = 0; i < smp_cpus; i++) {
>          cpu_s390x_init(cpu_model);
>      }
> }
> 
> Yep, I know cpu_model is going away ;).

Jason, do you have more information how cpu modeling or at least the parameterization will be
managed in future? I'm close to finishing a patch that introduces S390 cpu models and need to
know all this more precisely. Is there any document or discussion you can point me to. Thanks a
lot.

Michael 

> 

Re: [Qemu-devel] [PATCH 5/8] [PATCH RFC v3] s390-qemu: cpu hotplug - ipi_states enhancements

[Jason J. Herne]

On 09/19/2013 04:19 PM, Jason J. Herne wrote:
> On 09/05/2013 08:01 AM, Andreas Färber wrote:
>> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>>
> ...
>>
>> This is what got us into the link<> discussion last time. If we do
>>
>> for (i = 0; i < ARRAY_SIZE(ipi_states); i++) {
>>      name = g_strdup_printf("cpu[%i]", i);
>>      object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
>>                               &ipi_states[i], &err);
>> }
>>
>> then we get said /machine/cpu[n] link<> properties, at a QMP level
>> either returning nothing or the canonical path to the CPU object.
>>
>> On IRC I didn't get an answer of whether it was being done the above way
>> because there is infrastructure missing, and a look at object.h now
>> confirms that suspicion. CC'ing Anthony and Paolo.
>>
>> Since object_property_add_link() uses a NULL opaque, my idea would be to
>> add a single setter hook argument passed through as opaque to
>> object_set_link_property(), which would call it with the old and the new
>> value.
>>
>> The purpose would be to avoid growing our own internal setter API, which
>> is disjoint from the QMP qom-set we are targetting at.
>
> I wrote the code, very close to how you suggested and it appears to be
> working when tested with qom-list.  I'm still not certain why we need
> the ability to set the opaque of object_set_link_property()?
>
> For reference here is what I did:
>
> void s390_init_cpus(const char *cpu_model)
> {
>      int i;
>      char* name;
>
>      if (cpu_model == NULL) {
>          cpu_model = "host";
>      }
>
>      ipi_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
>
>      for (i = 0; i < max_cpus; i++) {
>          name = g_strdup_printf("cpu[%i]", i);
>          object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
>                                   (Object **)&ipi_states[i], NULL);
>      }
>
>      for (i = 0; i < smp_cpus; i++) {
>          cpu_s390x_init(cpu_model);
>      }
> }
>
> Yep, I know cpu_model is going away ;).
>

Ping? :)

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

Re: [Qemu-devel] [PATCH 5/8] [PATCH RFC v3] s390-qemu: cpu hotplug - ipi_states enhancements

[Alexander Graf]

On 01.08.2013, at 16:12, Jason J. Herne wrote:

> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Modify s390_cpu_addr2state to allow fetching state information for cpu addresses
> above smp_cpus.  Hotplug requires this capability.
> 
> Also add s390_cpu_set_state function to allow modification of ipi_state entries
> during hotplug.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
> hw/s390x/s390-virtio.c |    9 +++++----
> target-s390x/cpu.h     |    2 +-
> 2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 21e9124..5ad9cf3 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -54,12 +54,13 @@
> static VirtIOS390Bus *s390_bus;
> static S390CPU **ipi_states;
> 
> -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
> {
> -    if (cpu_addr >= smp_cpus) {
> -        return NULL;
> -    }
> +    ipi_states[cpu_addr] = state;

We should still ensure that we don't access anything beyond the size of the array, no?


Alex

> +}
> 
> +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> +{
>     return ipi_states[cpu_addr];
> }
> 
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 877eac7..62eb810 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -377,7 +377,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
> 
> uint8_t *s390_get_storage_keys(void);
> void s390_alloc_storage_keys(ram_addr_t ram_size);
> -
> +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state);
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> void s390_add_running_cpu(S390CPU *cpu);
> unsigned s390_del_running_cpu(S390CPU *cpu);
> -- 
> 1.7.10.4
> 

[Qemu-devel] [PATCH 6/8] [PATCH RFC v3] s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug

[Jason J. Herne]

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

    s390_new_cpu is created to encapsulate the creation of a new QOM S390CPU
    object given a cpuid and a model string.

    All actual cpu initialization code is moved from boot time specific functions to
    s390_cpu_initfn (qom init routine) or to s390_new_cpu. This is done to allow us
    to use the same basic code path for a cpu created at boot time and one created
    during a hotplug operation.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio.c |   25 ++++++++++++-------------
 target-s390x/cpu.c     |    4 ++--
 target-s390x/cpu.h     |    1 +
 target-s390x/helper.c  |   12 ++++++++++++
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 5ad9cf3..103f32e 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -56,11 +56,16 @@ static S390CPU **ipi_states;
 
 void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
 {
-    ipi_states[cpu_addr] = state;
+    if (cpu_addr < max_cpus) {
+        ipi_states[cpu_addr] = state;
+    }
 }
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
+    if (cpu_addr >= max_cpus) {
+        return NULL;
+    }
     return ipi_states[cpu_addr];
 }
 
@@ -197,19 +202,13 @@ void s390_init_cpus(const char *cpu_model)
         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();
+    for (i = 0; i < max_cpus; i++) {
+        ipi_states[i] = NULL;
+        if (i < smp_cpus) {
+            s390_new_cpu(cpu_model, i);
+        }
     }
 }
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 6be6c08..c90a91c 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -116,7 +116,6 @@ static void s390_cpu_initfn(Object *obj)
     S390CPU *cpu = S390_CPU(obj);
     CPUS390XState *env = &cpu->env;
     static bool inited;
-    static int cpu_num = 0;
 #if !defined(CONFIG_USER_ONLY)
     struct tm tm;
 #endif
@@ -135,8 +134,9 @@ static void s390_cpu_initfn(Object *obj)
      * cpu counter in s390_cpu_reset to a negative number at
      * initial ipl */
     cs->halted = 1;
+    cpu->env.exception_index = EXCP_HLT;
+    env->storage_keys = s390_get_storage_keys();
 #endif
-    env->cpu_num = cpu_num++;
     env->ext_index = -1;
 
     if (tcg_enabled() && !inited) {
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 62eb810..0f68dd0 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -315,6 +315,7 @@ static inline int get_ilen(uint8_t opc)
 #endif
 
 S390CPU *cpu_s390x_init(const char *cpu_model);
+S390CPU *s390_new_cpu(const char *cpu_model, int64_t cpuid);
 void s390x_translate_init(void);
 int cpu_s390x_exec(CPUS390XState *s);
 
diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 61abfd7..a39b148 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -70,6 +70,18 @@ void s390x_cpu_timer(void *opaque)
 }
 #endif
 
+S390CPU *s390_new_cpu(const char *cpu_model, int64_t cpuid)
+{
+    S390CPU *cpu;
+
+    cpu = cpu_s390x_init(cpu_model);
+    cpu->env.cpu_num = cpuid;
+#if !defined(CONFIG_USER_ONLY)
+    s390_cpu_set_ipistate(cpuid, cpu);
+#endif
+    return cpu;
+}
+
 S390CPU *cpu_s390x_init(const char *cpu_model)
 {
     S390CPU *cpu;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 6/8] [PATCH RFC v3] s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug

[Andreas Färber]

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
>     s390_new_cpu is created to encapsulate the creation of a new QOM S390CPU
>     object given a cpuid and a model string.
> 
>     All actual cpu initialization code is moved from boot time specific functions to
>     s390_cpu_initfn (qom init routine) or to s390_new_cpu. This is done to allow us
>     to use the same basic code path for a cpu created at boot time and one created
>     during a hotplug operation.

Intentionally indented?

> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  hw/s390x/s390-virtio.c |   25 ++++++++++++-------------
>  target-s390x/cpu.c     |    4 ++--
>  target-s390x/cpu.h     |    1 +
>  target-s390x/helper.c  |   12 ++++++++++++
>  4 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 5ad9cf3..103f32e 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -56,11 +56,16 @@ static S390CPU **ipi_states;
>  
>  void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
>  {
> -    ipi_states[cpu_addr] = state;
> +    if (cpu_addr < max_cpus) {
> +        ipi_states[cpu_addr] = state;
> +    }
>  }
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> +    if (cpu_addr >= max_cpus) {
> +        return NULL;
> +    }
>      return ipi_states[cpu_addr];
>  }
>  
> @@ -197,19 +202,13 @@ void s390_init_cpus(const char *cpu_model)
>          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();
> +    for (i = 0; i < max_cpus; i++) {
> +        ipi_states[i] = NULL;

Using g_malloc0() above would hopefully be more efficient and would
allow to leave the loop untouched for easier review.

> +        if (i < smp_cpus) {
> +            s390_new_cpu(cpu_model, i);
> +        }
>      }
>  }
>  
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 6be6c08..c90a91c 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -116,7 +116,6 @@ static void s390_cpu_initfn(Object *obj)
>      S390CPU *cpu = S390_CPU(obj);
>      CPUS390XState *env = &cpu->env;
>      static bool inited;
> -    static int cpu_num = 0;
>  #if !defined(CONFIG_USER_ONLY)
>      struct tm tm;
>  #endif
> @@ -135,8 +134,9 @@ static void s390_cpu_initfn(Object *obj)
>       * cpu counter in s390_cpu_reset to a negative number at
>       * initial ipl */
>      cs->halted = 1;
> +    cpu->env.exception_index = EXCP_HLT;
> +    env->storage_keys = s390_get_storage_keys();

4/8?

>  #endif
> -    env->cpu_num = cpu_num++;
>      env->ext_index = -1;
>  
>      if (tcg_enabled() && !inited) {
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 62eb810..0f68dd0 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -315,6 +315,7 @@ static inline int get_ilen(uint8_t opc)
>  #endif
>  
>  S390CPU *cpu_s390x_init(const char *cpu_model);
> +S390CPU *s390_new_cpu(const char *cpu_model, int64_t cpuid);
>  void s390x_translate_init(void);
>  int cpu_s390x_exec(CPUS390XState *s);
>  
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index 61abfd7..a39b148 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -70,6 +70,18 @@ void s390x_cpu_timer(void *opaque)
>  }
>  #endif
>  
> +S390CPU *s390_new_cpu(const char *cpu_model, int64_t cpuid)

Like I said on IRC, I'm not so fond of copying x86 workarounds here...
x86 does not have a fully QOM'ified CPU, s390x does.

> +{
> +    S390CPU *cpu;
> +
> +    cpu = cpu_s390x_init(cpu_model);
> +    cpu->env.cpu_num = cpuid;

linux-user never calls s390_new_cpu(), so it will change behavior in
always having cpu_num of 0. I guess we can live with that but such a
change needs to be mentioned in the commit message at least.

Why is this moved to after CPU init? Can't we just override the field if
need be? Either Jens or Christian said that we would not want to fill up
holes in ipi_tables to have the CPU address be always unique; which
would mean that it would always be counting as before. if we need to
tweak it, we should add a property to be able to set it from command
line and QMP.

This affects migration btw: We would need to migrate the current or next
CPU address since the last CPU might've been hot-unplugged so that next
CPU address != last non-NULL ipi_states[] slot plus one.

> +#if !defined(CONFIG_USER_ONLY)
> +    s390_cpu_set_ipistate(cpuid, cpu);
> +#endif

...leaving only this then. Why not do this from the CPU realizefn so
that errors actually can be propagated? If cpuid >= max_cpus the above
will silently do nothing.

In that case we don't need this function any longer.

> +    return cpu;
> +}
> +
>  S390CPU *cpu_s390x_init(const char *cpu_model)
>  {
>      S390CPU *cpu;

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 6/8] [PATCH RFC v3] s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug

[Jason J. Herne]

On 09/05/2013 08:28 AM, Andreas Färber wrote:
> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>>      s390_new_cpu is created to encapsulate the creation of a new QOM S390CPU
>>      object given a cpuid and a model string.
>>
>>      All actual cpu initialization code is moved from boot time specific functions to
>>      s390_cpu_initfn (qom init routine) or to s390_new_cpu. This is done to allow us
>>      to use the same basic code path for a cpu created at boot time and one created
>>      during a hotplug operation.
>
> Intentionally indented?
>
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>> ---
>>   hw/s390x/s390-virtio.c |   25 ++++++++++++-------------
>>   target-s390x/cpu.c     |    4 ++--
>>   target-s390x/cpu.h     |    1 +
>>   target-s390x/helper.c  |   12 ++++++++++++
>>   4 files changed, 27 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index 5ad9cf3..103f32e 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
...
>> @@ -197,19 +202,13 @@ void s390_init_cpus(const char *cpu_model)
>>           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();
>> +    for (i = 0; i < max_cpus; i++) {
>> +        ipi_states[i] = NULL;
>
> Using g_malloc0() above would hopefully be more efficient and would
> allow to leave the loop untouched for easier review.
>

I don't follow. I'm completely changing this loop.  I do not believe we 
can obtain the same functionality implemented here while not touching 
the loop.

>> +        if (i < smp_cpus) {
>> +            s390_new_cpu(cpu_model, i);
>> +        }
>>       }
>>   }
>>
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 6be6c08..c90a91c 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -116,7 +116,6 @@ static void s390_cpu_initfn(Object *obj)
>>       S390CPU *cpu = S390_CPU(obj);
>>       CPUS390XState *env = &cpu->env;
>>       static bool inited;
>> -    static int cpu_num = 0;
>>   #if !defined(CONFIG_USER_ONLY)
>>       struct tm tm;
>>   #endif
>> @@ -135,8 +134,9 @@ static void s390_cpu_initfn(Object *obj)
>>        * cpu counter in s390_cpu_reset to a negative number at
>>        * initial ipl */
>>       cs->halted = 1;
>> +    cpu->env.exception_index = EXCP_HLT;
>> +    env->storage_keys = s390_get_storage_keys();
>
> 4/8?
>

Are you asking if this belongs in patch #4? if so, I would say no.  It 
does deal with storage keys, yes. But we're not changing storage key 
semantics here (as we are in patch 4), we're just moving where the 
storage key ptr gets set.  This is in support of re-organizing how cpus 
are initialized as per patch title/description.

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

Re: [Qemu-devel] [PATCH 6/8] [PATCH RFC v3] s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug

[Jason J. Herne]

On 09/13/2013 11:24 AM, Jason J. Herne wrote:
> On 09/05/2013 08:28 AM, Andreas Färber wrote:
>> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>>
>>>      s390_new_cpu is created to encapsulate the creation of a new QOM
>>> S390CPU
>>>      object given a cpuid and a model string.
>>>
>>>      All actual cpu initialization code is moved from boot time
>>> specific functions to
>>>      s390_cpu_initfn (qom init routine) or to s390_new_cpu. This is
>>> done to allow us
>>>      to use the same basic code path for a cpu created at boot time
>>> and one created
>>>      during a hotplug operation.
>>
>> Intentionally indented?
>>
>>>
>>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>>> ---
>>>   hw/s390x/s390-virtio.c |   25 ++++++++++++-------------
>>>   target-s390x/cpu.c     |    4 ++--
>>>   target-s390x/cpu.h     |    1 +
>>>   target-s390x/helper.c  |   12 ++++++++++++
>>>   4 files changed, 27 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>>> index 5ad9cf3..103f32e 100644
>>> --- a/hw/s390x/s390-virtio.c
>>> +++ b/hw/s390x/s390-virtio.c
> ...
>>> @@ -197,19 +202,13 @@ void s390_init_cpus(const char *cpu_model)
>>>           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();
>>> +    for (i = 0; i < max_cpus; i++) {
>>> +        ipi_states[i] = NULL;
>>
>> Using g_malloc0() above would hopefully be more efficient and would
>> allow to leave the loop untouched for easier review.
>>
>
> I don't follow. I'm completely changing this loop.  I do not believe we
> can obtain the same functionality implemented here while not touching
> the loop.
>
>>> +        if (i < smp_cpus) {
>>> +            s390_new_cpu(cpu_model, i);
>>> +        }
>>>       }
>>>   }
>>>
>>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>>> index 6be6c08..c90a91c 100644
>>> --- a/target-s390x/cpu.c
>>> +++ b/target-s390x/cpu.c
>>> @@ -116,7 +116,6 @@ static void s390_cpu_initfn(Object *obj)
>>>       S390CPU *cpu = S390_CPU(obj);
>>>       CPUS390XState *env = &cpu->env;
>>>       static bool inited;
>>> -    static int cpu_num = 0;
>>>   #if !defined(CONFIG_USER_ONLY)
>>>       struct tm tm;
>>>   #endif
>>> @@ -135,8 +134,9 @@ static void s390_cpu_initfn(Object *obj)
>>>        * cpu counter in s390_cpu_reset to a negative number at
>>>        * initial ipl */
>>>       cs->halted = 1;
>>> +    cpu->env.exception_index = EXCP_HLT;
>>> +    env->storage_keys = s390_get_storage_keys();
>>
>> 4/8?
>>
>
> Are you asking if this belongs in patch #4? if so, I would say no.  It
> does deal with storage keys, yes. But we're not changing storage key
> semantics here (as we are in patch 4), we're just moving where the
> storage key ptr gets set.  This is in support of re-organizing how cpus
> are initialized as per patch title/description.
>

Ping? :)

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

Re: [Qemu-devel] [PATCH 6/8] [PATCH RFC v3] s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug

[Alexander Graf]

On 01.08.2013, at 16:12, Jason J. Herne wrote:

> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
>    s390_new_cpu is created to encapsulate the creation of a new QOM S390CPU
>    object given a cpuid and a model string.
> 
>    All actual cpu initialization code is moved from boot time specific functions to
>    s390_cpu_initfn (qom init routine) or to s390_new_cpu. This is done to allow us
>    to use the same basic code path for a cpu created at boot time and one created
>    during a hotplug operation.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
> hw/s390x/s390-virtio.c |   25 ++++++++++++-------------
> target-s390x/cpu.c     |    4 ++--
> target-s390x/cpu.h     |    1 +
> target-s390x/helper.c  |   12 ++++++++++++
> 4 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 5ad9cf3..103f32e 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -56,11 +56,16 @@ static S390CPU **ipi_states;
> 
> void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
> {
> -    ipi_states[cpu_addr] = state;
> +    if (cpu_addr < max_cpus) {

Ah, here you add the checks back in. Works for me.

> +        ipi_states[cpu_addr] = state;
> +    }
> }
> 
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> {
> +    if (cpu_addr >= max_cpus) {
> +        return NULL;
> +    }
>     return ipi_states[cpu_addr];
> }
> 
> @@ -197,19 +202,13 @@ void s390_init_cpus(const char *cpu_model)
>         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);

g_new is easier :).


Alex

[Qemu-devel] [PATCH 7/8] [PATCH RFC v3] s390-qemu: cpu hotplug - Implement hot_add_cpu hook

[Jason J. Herne]

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

Implement hot_add_cpu for S390 to allow hot plugging of cpus.

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b469960..30b6a48 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -117,6 +117,9 @@ static QEMUMachine ccw_machine = {
     .alias = "s390-ccw",
     .desc = "VirtIO-ccw based S390 machine",
     .init = ccw_init,
+#if !defined(CONFIG_USER_ONLY)
+    .hot_add_cpu = ccw_hot_add_cpu,
+#endif
     .block_default_type = IF_VIRTIO,
     .no_cdrom = 1,
     .no_floppy = 1,
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index c90a91c..60029d9 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -27,6 +27,8 @@
 #include "qemu-common.h"
 #include "qemu/timer.h"
 #include "hw/hw.h"
+#include "hw/s390x/sclp.h"
+#include "sysemu/sysemu.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
@@ -154,6 +156,36 @@ static void s390_cpu_finalize(Object *obj)
 #endif
 }
 
+#if !defined(CONFIG_USER_ONLY)
+void ccw_hot_add_cpu(const int64_t id, Error **errp)
+{
+    S390CPU *new_cpu;
+    CPUState *cpu;
+    const char *model_str;
+    int cpu_count = 0;
+
+    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
+        cpu_count++;
+    }
+
+    if (cpu_count == max_cpus) {
+        error_setg(errp, "Maximum number of cpus already defined");
+        return;
+    }
+
+    if (id != cpu_count) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", The next available id is %d", id, cpu_count);
+        return;
+    }
+
+    model_str = s390_cpu_addr2state(0)->env.cpu_model_str;
+    new_cpu = s390_new_cpu(model_str, id);
+    object_property_set_bool(OBJECT(new_cpu), true, "realized", NULL);
+    raise_irq_cpu_hotplug();
+}
+#endif
+
 static const VMStateDescription vmstate_s390_cpu = {
     .name = "cpu",
     .unmigratable = 1,
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 0f68dd0..711aad4 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -383,6 +383,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
 
+void ccw_hot_add_cpu(const int64_t id, Error **errp);
+
 /* service interrupts are floating therefore we must not pass an cpustate */
 void s390_sclp_extint(uint32_t parm);
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 7/8] [PATCH RFC v3] s390-qemu: cpu hotplug - Implement hot_add_cpu hook

[Andreas Färber]

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Implement hot_add_cpu for S390 to allow hot plugging of cpus.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c |    3 +++
>  target-s390x/cpu.c         |   32 ++++++++++++++++++++++++++++++++
>  target-s390x/cpu.h         |    2 ++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index b469960..30b6a48 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -117,6 +117,9 @@ static QEMUMachine ccw_machine = {
>      .alias = "s390-ccw",
>      .desc = "VirtIO-ccw based S390 machine",
>      .init = ccw_init,
> +#if !defined(CONFIG_USER_ONLY)
> +    .hot_add_cpu = ccw_hot_add_cpu,
> +#endif

I doubt this #ifdeffery is necessary here?

>      .block_default_type = IF_VIRTIO,
>      .no_cdrom = 1,
>      .no_floppy = 1,
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index c90a91c..60029d9 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -27,6 +27,8 @@
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  #include "hw/hw.h"
> +#include "hw/s390x/sclp.h"
> +#include "sysemu/sysemu.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
>  #endif
> @@ -154,6 +156,36 @@ static void s390_cpu_finalize(Object *obj)
>  #endif
>  }
>  
> +#if !defined(CONFIG_USER_ONLY)
> +void ccw_hot_add_cpu(const int64_t id, Error **errp)
> +{
> +    S390CPU *new_cpu;
> +    CPUState *cpu;
> +    const char *model_str;
> +    int cpu_count = 0;
> +
> +    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {

CPU_FOREACH(cpu) {

> +        cpu_count++;
> +    }
> +
> +    if (cpu_count == max_cpus) {
> +        error_setg(errp, "Maximum number of cpus already defined");
> +        return;
> +    }
> +
> +    if (id != cpu_count) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", The next available id is %d", id, cpu_count);
> +        return;
> +    }

This logic seems wrong according to your colleagues. It should be
checking against the static cpu_num counter or not checking at all if we
want to allow explicit device_add s390-cpu,cpu-num=42.

> +
> +    model_str = s390_cpu_addr2state(0)->env.cpu_model_str;
> +    new_cpu = s390_new_cpu(model_str, id);

As announced, a patch in my large series finally sent out removes
cpu_model_str field. Since we don't have any for s390x, I suggest that
you use the QOM constructs so that device_add works as well, i.e.
new_cpu = object_new(TYPE_S390_CPU).

> +    object_property_set_bool(OBJECT(new_cpu), true, "realized", NULL);
> +    raise_irq_cpu_hotplug();

This would mean moving this line into the realizefn, conditional on
dev->hotplugged (and probably #ifndef CONFIG_USER_ONLY).

Regards,
Andreas

> +}
> +#endif
> +
>  static const VMStateDescription vmstate_s390_cpu = {
>      .name = "cpu",
>      .unmigratable = 1,
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 0f68dd0..711aad4 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -383,6 +383,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  void s390_add_running_cpu(S390CPU *cpu);
>  unsigned s390_del_running_cpu(S390CPU *cpu);
>  
> +void ccw_hot_add_cpu(const int64_t id, Error **errp);
> +
>  /* service interrupts are floating therefore we must not pass an cpustate */
>  void s390_sclp_extint(uint32_t parm);
>  
> 


-- 
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 v3] s390-qemu: cpu hotplug - Implement hot_add_cpu hook

[Jason J. Herne]

On 09/05/2013 08:38 AM, Andreas Färber wrote:
> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Implement hot_add_cpu for S390 to allow hot plugging of cpus.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c |    3 +++
>>   target-s390x/cpu.c         |   32 ++++++++++++++++++++++++++++++++
>>   target-s390x/cpu.h         |    2 ++
>>   3 files changed, 37 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index b469960..30b6a48 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -117,6 +117,9 @@ static QEMUMachine ccw_machine = {
>>       .alias = "s390-ccw",
>>       .desc = "VirtIO-ccw based S390 machine",
>>       .init = ccw_init,
>> +#if !defined(CONFIG_USER_ONLY)
>> +    .hot_add_cpu = ccw_hot_add_cpu,
>> +#endif
>
> I doubt this #ifdeffery is necessary here?
>

This was needed because ccw_hot_add_cpu calls s390_cpu_addr2state which 
is wrapped in the very same ifdef.  However, the offending line is this:

model_str = s390_cpu_addr2state(0)->env.cpu_model_str;

Since we're doing away with that line anyway I can probably remove that 
ifdef. However, does it make sense to have a cpu-add command for the 
linux-user target?

Also, do you know when your patch to remove the model string will hit 
the master branch?

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

Re: [Qemu-devel] [PATCH 7/8] [PATCH RFC v3] s390-qemu: cpu hotplug - Implement hot_add_cpu hook

[Andreas Färber]

Am 13.09.2013 17:29, schrieb Jason J. Herne:
> On 09/05/2013 08:38 AM, Andreas Färber wrote:
>> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>>
>>> Implement hot_add_cpu for S390 to allow hot plugging of cpus.
>>>
>>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>>> ---
>>>   hw/s390x/s390-virtio-ccw.c |    3 +++
>>>   target-s390x/cpu.c         |   32 ++++++++++++++++++++++++++++++++
>>>   target-s390x/cpu.h         |    2 ++
>>>   3 files changed, 37 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index b469960..30b6a48 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -117,6 +117,9 @@ static QEMUMachine ccw_machine = {
>>>       .alias = "s390-ccw",
>>>       .desc = "VirtIO-ccw based S390 machine",
>>>       .init = ccw_init,
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +    .hot_add_cpu = ccw_hot_add_cpu,
>>> +#endif
>>
>> I doubt this #ifdeffery is necessary here?
>>
> 
> This was needed because ccw_hot_add_cpu calls s390_cpu_addr2state which
> is wrapped in the very same ifdef.

This whole file should never get compiled for CONFIG_USER_ONLY.

>  However, the offending line is this:
> 
> model_str = s390_cpu_addr2state(0)->env.cpu_model_str;
> 
> Since we're doing away with that line anyway I can probably remove that
> ifdef. However, does it make sense to have a cpu-add command for the
> linux-user target?
> 
> Also, do you know when your patch to remove the model string will hit
> the master branch?

I've pushed it to qom-cpu-next branch for now, suggest to rebase on
that. It all depends on when I get to review all the pending patches and
respin some interfering ones and then get someone to pull, which is
outside of my control.

Cheers,
Andreas

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

[Qemu-devel] [PATCH 8/8] [PATCH RFC v3] qemu-monitor: HMP cpu-add wrapper

[Jason J. Herne]

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

Add HMP cpu-add wrapper to allow cpu hot plugging via monitor.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hmp-commands.hx |   13 +++++++++++++
 hmp.c           |   10 ++++++++++
 hmp.h           |    1 +
 3 files changed, 24 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8c6b91a..cb8712b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1587,6 +1587,19 @@ Executes a qemu-io command on the given block device.
 ETEXI
 
     {
+        .name       = "cpu-add",
+        .args_type  = "id:i",
+        .params     = "id",
+        .help       = "add cpu",
+        .mhandler.cmd  = hmp_cpu_add,
+    },
+
+STEXI
+@item cpu-add @var{id}
+Add CPU with id @var{id}
+ETEXI
+
+    {
         .name       = "info",
         .args_type  = "item:s?",
         .params     = "[subcommand]",
diff --git a/hmp.c b/hmp.c
index c45514b..9465bd4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1475,6 +1475,16 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_cpu_add(Monitor *mon, const QDict *qdict)
+{
+    int cpuid;
+    Error *err = NULL;
+
+    cpuid = qdict_get_int(qdict, "id");
+    qmp_cpu_add(cpuid, &err);
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_chardev_add(Monitor *mon, const QDict *qdict)
 {
     const char *args = qdict_get_str(qdict, "args");
diff --git a/hmp.h b/hmp.h
index 6c3bdcd..9effca5 100644
--- a/hmp.h
+++ b/hmp.h
@@ -87,5 +87,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
 void hmp_chardev_add(Monitor *mon, const QDict *qdict);
 void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
 void hmp_qemu_io(Monitor *mon, const QDict *qdict);
+void hmp_cpu_add(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v3] qemu-monitor: HMP cpu-add wrapper

[Andreas Färber]

Luiz,

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Add HMP cpu-add wrapper to allow cpu hot plugging via monitor.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>

What are your thoughts on this?

Thanks,
Andreas

> ---
>  hmp-commands.hx |   13 +++++++++++++
>  hmp.c           |   10 ++++++++++
>  hmp.h           |    1 +
>  3 files changed, 24 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8c6b91a..cb8712b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1587,6 +1587,19 @@ Executes a qemu-io command on the given block device.
>  ETEXI
>  
>      {
> +        .name       = "cpu-add",
> +        .args_type  = "id:i",
> +        .params     = "id",
> +        .help       = "add cpu",
> +        .mhandler.cmd  = hmp_cpu_add,
> +    },
> +
> +STEXI
> +@item cpu-add @var{id}
> +Add CPU with id @var{id}
> +ETEXI
> +
> +    {
>          .name       = "info",
>          .args_type  = "item:s?",
>          .params     = "[subcommand]",
> diff --git a/hmp.c b/hmp.c
> index c45514b..9465bd4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1475,6 +1475,16 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &errp);
>  }
>  
> +void hmp_cpu_add(Monitor *mon, const QDict *qdict)
> +{
> +    int cpuid;
> +    Error *err = NULL;
> +
> +    cpuid = qdict_get_int(qdict, "id");
> +    qmp_cpu_add(cpuid, &err);
> +    hmp_handle_error(mon, &err);
> +}
> +
>  void hmp_chardev_add(Monitor *mon, const QDict *qdict)
>  {
>      const char *args = qdict_get_str(qdict, "args");
> diff --git a/hmp.h b/hmp.h
> index 6c3bdcd..9effca5 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -87,5 +87,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_add(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
>  void hmp_qemu_io(Monitor *mon, const QDict *qdict);
> +void hmp_cpu_add(Monitor *mon, const QDict *qdict);
>  
>  #endif
> 


-- 
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 v3] qemu-monitor: HMP cpu-add wrapper

[Luiz Capitulino]

On Thu, 01 Aug 2013 18:02:08 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Luiz,
> 
> Am 01.08.2013 16:12, schrieb Jason J. Herne:
> > From: "Jason J. Herne" <jjherne@us.ibm.com>
> > 
> > Add HMP cpu-add wrapper to allow cpu hot plugging via monitor.
> > 
> > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> 
> What are your thoughts on this?

Looks good:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> 
> Thanks,
> Andreas
> 
> > ---
> >  hmp-commands.hx |   13 +++++++++++++
> >  hmp.c           |   10 ++++++++++
> >  hmp.h           |    1 +
> >  3 files changed, 24 insertions(+)
> > 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 8c6b91a..cb8712b 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1587,6 +1587,19 @@ Executes a qemu-io command on the given block device.
> >  ETEXI
> >  
> >      {
> > +        .name       = "cpu-add",
> > +        .args_type  = "id:i",
> > +        .params     = "id",
> > +        .help       = "add cpu",
> > +        .mhandler.cmd  = hmp_cpu_add,
> > +    },
> > +
> > +STEXI
> > +@item cpu-add @var{id}
> > +Add CPU with id @var{id}
> > +ETEXI
> > +
> > +    {
> >          .name       = "info",
> >          .args_type  = "item:s?",
> >          .params     = "[subcommand]",
> > diff --git a/hmp.c b/hmp.c
> > index c45514b..9465bd4 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1475,6 +1475,16 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
> >      hmp_handle_error(mon, &errp);
> >  }
> >  
> > +void hmp_cpu_add(Monitor *mon, const QDict *qdict)
> > +{
> > +    int cpuid;
> > +    Error *err = NULL;
> > +
> > +    cpuid = qdict_get_int(qdict, "id");
> > +    qmp_cpu_add(cpuid, &err);
> > +    hmp_handle_error(mon, &err);
> > +}
> > +
> >  void hmp_chardev_add(Monitor *mon, const QDict *qdict)
> >  {
> >      const char *args = qdict_get_str(qdict, "args");
> > diff --git a/hmp.h b/hmp.h
> > index 6c3bdcd..9effca5 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -87,5 +87,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
> >  void hmp_chardev_add(Monitor *mon, const QDict *qdict);
> >  void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
> >  void hmp_qemu_io(Monitor *mon, const QDict *qdict);
> > +void hmp_cpu_add(Monitor *mon, const QDict *qdict);
> >  
> >  #endif
> > 
> 
> 

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

[Andreas Färber]

Hello,

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Latest code for cpu Hotplug on S390 architecture.   This one is vastly simpler
> than v2 as we have decided to avoid the command line specification 
> of -device s390-cpu.
> 
> The last version can be found here:
> http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg01183.html
> 
> There is also a patch in this series to add cpu-add to the Qemu monitor
> interface.
> 
> 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. 

We have been having several off-list discussions since then that I'll
try to briefly summarize here, please correct or extend as needed:

1) CPU topology for QOM

Physically a System z machine may have an MCM with, e.g., 6 CPUs with 6
cores each. But unlike x86, there is PR/SM, LPAR and possibly z/VM in
between Linux and hardware, so we do actually want to be able to
hot-plug in quantities of 1 and not by 6 on s390x for the foreseeable
future. We seem willing to set a QOM ABI in stone based on that assumption.

=> s390-cpu (or future subtypes) to be used with device_add.
=> Flat /machine/cpu[n] list in composition tree a possibility.

1a) CPU topology for guests

STSI instruction topology support not implemented yet.

=> Guest unaware of any emulated topology today.

hyptop tool requires hypfs implementation for KVM.

=> Guest unaware of sibling VMs today, unlike z/VM and LPAR.

2) CPU hot-unplug

Hotplug will always use a unique linear CPU address, even if hot-unplug
leads to a sparse address space.

=> cpu_num != cpu_index


With all that in mind, I'll now need to review the s390 patches again.

For the HMP patch I am waiting on feedback from Igor once he returns
from his vacation and, if there are no objections, would like to see
that patch go through Luiz' queue since unrelated to s390x.

Regards,
Andreas

> 
> Jason J. Herne (8):
>   s390-qemu: cpu hotplug - Define New SCLP Codes
>   s390-qemu: cpu hotplug - SCLP CPU Info
>   s390-qemu: cpu hotplug - SCLP Event integration
>   s390-qemu: cpu hotplug - Storage key global access
>   s390-qemu: cpu hotplug - ipi_states enhancements
>   s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug
>   s390-qemu: cpu hotplug - Implement hot_add_cpu hook
>   qemu-monitor: HMP cpu-add wrapper
> 
>  hmp-commands.hx                   |   13 ++++
>  hmp.c                             |   10 ++++
>  hmp.h                             |    1 +
>  hw/s390x/Makefile.objs            |    2 +-
>  hw/s390x/event-facility.c         |    7 +++
>  hw/s390x/s390-virtio-ccw.c        |    8 ++-
>  hw/s390x/s390-virtio.c            |   47 +++++++++------
>  hw/s390x/s390-virtio.h            |    2 +-
>  hw/s390x/sclp.c                   |   53 +++++++++++++++-
>  hw/s390x/sclpcpu.c                |  120 +++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/event-facility.h |    3 +
>  include/hw/s390x/sclp.h           |   41 +++++++++++++
>  target-s390x/cpu.c                |   36 ++++++++++-
>  target-s390x/cpu.h                |    7 +++
>  target-s390x/helper.c             |   12 ++++
>  15 files changed, 336 insertions(+), 26 deletions(-)
>  create mode 100644 hw/s390x/sclpcpu.c
> 


-- 
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 0/8] [PATCH RFC v3] s390 cpu hotplug

[Luiz Capitulino]

On Wed, 04 Sep 2013 14:45:44 +0200
Andreas Färber <afaerber@suse.de> wrote:

> => cpu_num != cpu_index
> 
> 
> With all that in mind, I'll now need to review the s390 patches again.
> 
> For the HMP patch I am waiting on feedback from Igor once he returns
> from his vacation and, if there are no objections, would like to see
> that patch go through Luiz' queue since unrelated to s390x.

I don't remember seeing that patch, I was CC'ed?

Anyway, if you point me to the patch I can review it and add my
Reviewed-by, which should be as good as having the patch merged
on master through my queue.

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

[Andreas Färber]

Am 04.09.2013 14:56, schrieb Luiz Capitulino:
> On Wed, 04 Sep 2013 14:45:44 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> For the HMP patch I am waiting on feedback from Igor once he returns
>> from his vacation and, if there are no objections, would like to see
>> that patch go through Luiz' queue since unrelated to s390x.
> 
> I don't remember seeing that patch, I was CC'ed?

Originally no, but you added a Reviewed-by after I CC'ed you. :)

Apart from me not being familiar with the HMP infrastructure, I was
wondering if there was a reason why this wasn't done from the start.

So we don't have some only-new-QMP-commands policy and people are
encouraged to implement an HMP command when they do a QMP command?

Cheers,
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 0/8] [PATCH RFC v3] s390 cpu hotplug

[Luiz Capitulino]

On Wed, 04 Sep 2013 15:04:17 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 04.09.2013 14:56, schrieb Luiz Capitulino:
> > On Wed, 04 Sep 2013 14:45:44 +0200
> > Andreas Färber <afaerber@suse.de> wrote:
> > 
> >> For the HMP patch I am waiting on feedback from Igor once he returns
> >> from his vacation and, if there are no objections, would like to see
> >> that patch go through Luiz' queue since unrelated to s390x.
> > 
> > I don't remember seeing that patch, I was CC'ed?

It should be good then :)

> 
> Originally no, but you added a Reviewed-by after I CC'ed you. :)
> 
> Apart from me not being familiar with the HMP infrastructure, I was
> wondering if there was a reason why this wasn't done from the start.
> 
> So we don't have some only-new-QMP-commands policy

Not sure what you meant, but what is usually not accepted are
new HMP-only commands.

> and people are
> encouraged to implement an HMP command when they do a QMP command?

If the new command makes sense in HMP, yes they're.

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

[Christian Borntraeger]

On 04/09/13 14:45, Andreas Färber wrote:
> Hello,
> 
> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Latest code for cpu Hotplug on S390 architecture.   This one is vastly simpler
>> than v2 as we have decided to avoid the command line specification 
>> of -device s390-cpu.
>>
>> The last version can be found here:
>> http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg01183.html
>>
>> There is also a patch in this series to add cpu-add to the Qemu monitor
>> interface.
>>
>> 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. 
> 
> We have been having several off-list discussions since then that I'll
> try to briefly summarize here, please correct or extend as needed:
> 
> 1) CPU topology for QOM
> 
> Physically a System z machine may have an MCM with, e.g., 6 CPUs with 6
> cores each. But unlike x86, there is PR/SM, LPAR and possibly z/VM in
> between Linux and hardware, so we do actually want to be able to
> hot-plug in quantities of 1 and not by 6 on s390x for the foreseeable
> future. We seem willing to set a QOM ABI in stone based on that assumption.

Just stepping in, Jason is on vacation this week.

To summarize my understanding:
You were thinking if CPU model needs topology (e.g. -device mcm,id=m1, -device cpu,mcm=m1)
and s390 was the only arch left, that you were not sure about if topology is needed? 
All other platforms dont need topology for cpu hotplug?

Yes, we want to be able to hotplug single cores (not chips, not MCMs). 
It is pretty hard to pin the vCPUs to a given real topology for KVM. You need to
pin on LPAR and KVM. Libvirt could  do some pinning of guest vCPUs to host CPUs and
LPAR can have dedicated CPUs. But pinning a full chip (6cores) would only make
sense in very rare cases.


> 
> => s390-cpu (or future subtypes) to be used with device_add.
> => Flat /machine/cpu[n] list in composition tree a possibility.
> 
> 1a) CPU topology for guests
> 
> STSI instruction topology support not implemented yet.

Right not implemented yet, but we certainly want to be able to define the guest
visible topology at some point in time (grouping of cores basically). 
But I guess this does not mean that we have to go away from the flat list of CPUs.


> 
> => Guest unaware of any emulated topology today.

An additional problem is, that for the normal case (linux scheduler, no pinning, also
no gang scheduling) the topology would change too fast. The guest would be busy rebuilding
the scheduler domains all the time.

> 
> hyptop tool requires hypfs implementation for KVM.
> 
> => Guest unaware of sibling VMs today, unlike z/VM and LPAR.

Right. But we will look into hyptop at some point in time. 

> 
> 2) CPU hot-unplug
> 
> Hotplug will always use a unique linear CPU address, even if hot-unplug
> leads to a sparse address space.
> 
> => cpu_num != cpu_index
> 
> 
> With all that in mind, I'll now need to review the s390 patches again.
> 
> For the HMP patch I am waiting on feedback from Igor once he returns
> from his vacation and, if there are no objections, would like to see
> that patch go through Luiz' queue since unrelated to s390x.

Yes, would be cool if the HMP patch could go upstream via that path. That would 
reduce the rework/patch burden of Jason.

> 
> Regards,
> Andreas
> 
>>
>> Jason J. Herne (8):
>>   s390-qemu: cpu hotplug - Define New SCLP Codes
>>   s390-qemu: cpu hotplug - SCLP CPU Info
>>   s390-qemu: cpu hotplug - SCLP Event integration
>>   s390-qemu: cpu hotplug - Storage key global access
>>   s390-qemu: cpu hotplug - ipi_states enhancements
>>   s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug
>>   s390-qemu: cpu hotplug - Implement hot_add_cpu hook
>>   qemu-monitor: HMP cpu-add wrapper
>>
>>  hmp-commands.hx                   |   13 ++++
>>  hmp.c                             |   10 ++++
>>  hmp.h                             |    1 +
>>  hw/s390x/Makefile.objs            |    2 +-
>>  hw/s390x/event-facility.c         |    7 +++
>>  hw/s390x/s390-virtio-ccw.c        |    8 ++-
>>  hw/s390x/s390-virtio.c            |   47 +++++++++------
>>  hw/s390x/s390-virtio.h            |    2 +-
>>  hw/s390x/sclp.c                   |   53 +++++++++++++++-
>>  hw/s390x/sclpcpu.c                |  120 +++++++++++++++++++++++++++++++++++++
>>  include/hw/s390x/event-facility.h |    3 +
>>  include/hw/s390x/sclp.h           |   41 +++++++++++++
>>  target-s390x/cpu.c                |   36 ++++++++++-
>>  target-s390x/cpu.h                |    7 +++
>>  target-s390x/helper.c             |   12 ++++
>>  15 files changed, 336 insertions(+), 26 deletions(-)
>>  create mode 100644 hw/s390x/sclpcpu.c
>>
> 
> 

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

[Andreas Färber]

Am 05.09.2013 12:40, schrieb Christian Borntraeger:
> On 04/09/13 14:45, Andreas Färber wrote:
>> Hello,
>>
>> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>>
>>> Latest code for cpu Hotplug on S390 architecture.   This one is vastly simpler
>>> than v2 as we have decided to avoid the command line specification 
>>> of -device s390-cpu.
>>>
>>> The last version can be found here:
>>> http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg01183.html
>>>
>>> There is also a patch in this series to add cpu-add to the Qemu monitor
>>> interface.
>>>
>>> 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. 
>>
>> We have been having several off-list discussions since then that I'll
>> try to briefly summarize here, please correct or extend as needed:
>>
>> 1) CPU topology for QOM
>>
>> Physically a System z machine may have an MCM with, e.g., 6 CPUs with 6
>> cores each. But unlike x86, there is PR/SM, LPAR and possibly z/VM in
>> between Linux and hardware, so we do actually want to be able to
>> hot-plug in quantities of 1 and not by 6 on s390x for the foreseeable
>> future. We seem willing to set a QOM ABI in stone based on that assumption.
> 
> Just stepping in, Jason is on vacation this week.

Everyone is welcome to comment. :)

> To summarize my understanding:
> You were thinking if CPU model needs topology (e.g. -device mcm,id=m1, -device cpu,mcm=m1)
> and s390 was the only arch left, that you were not sure about if topology is needed? 
> All other platforms dont need topology for cpu hotplug?

No, on the contrary: I don't want s390x to blindly copy x86 cpu-add,
because for x86 we know that what we have is a hack to make it work
today, but there we know we want to do device_add Xeon-X42-4242 instead,
which then hot-plugs the 6 cores x 2 threads at once that a physical
hot-plug would do and not hot-add individual threads.

So the question of topology is not about what is below KVM but about
what is inside QEMU, since x86 emulates i440fx/q35 based hardware.
The understanding I reached on IRC is that s390x (similar to sPAPR)
tries to emulate LPAR / z/VM layer rather than the hardware below them,
thus no applicable concept of "real" hardware and arbitrary quantities.

> Yes, we want to be able to hotplug single cores (not chips, not MCMs). 
> It is pretty hard to pin the vCPUs to a given real topology for KVM. You need to
> pin on LPAR and KVM. Libvirt could  do some pinning of guest vCPUs to host CPUs and
> LPAR can have dedicated CPUs. But pinning a full chip (6cores) would only make
> sense in very rare cases.

Last time I looked into this, the post-add hook was solely for overall
ccw initialization. So we can use device_add s390-cpu today, can't we?

The question that I still need to investigate is how the
always-incrementing CPU address interacts with maxcpus. Consider
maxcpus=6 and smp_cpus=2. 4x device_add should work. Now if we did 1x
device_del, then 1x device_add should work again IMO. cpu-add checks the
user-supplied id against maxcpus though iirc.

Therefore my saying in multiple contexts that we should get the QEMU and
KVM CPU count checks into the CPU realizefn so that we get the checks
irrespective of the call site with nice error reporting.

>> => s390-cpu (or future subtypes) to be used with device_add.
>> => Flat /machine/cpu[n] list in composition tree a possibility.
>>
>> 1a) CPU topology for guests
>>
>> STSI instruction topology support not implemented yet.
> 
> Right not implemented yet, but we certainly want to be able to define the guest
> visible topology at some point in time (grouping of cores basically). 
> But I guess this does not mean that we have to go away from the flat list of CPUs.

So STSI would show what real LPAR/CPU we are running on? But QEMU would
have /machine/cpu[0]? Or do we need /machine/cpugroup[0]/cpu[0]? The
latter is my concern here, to decide about child<> vs. link<> properties.

To cope with device_add s390-cpu adding the device to
/machine/peripheral/<id> or /machine/peripheral-anon/device[0] I *think*
we'll need link<>, which would then translate back to ipi_states array
as backend and the remaining question would be where to expose those
properties in the composition tree - i.e. /machine/cpu[n] or
/machine/ipi/cpu[n] or something - please suggest. Similarly if those
become link<> properties then the CPUs created by the machine via
smp_cpus need a canonical path as well; quite obviously both cannot be
the same.

Background is that long-term Anthony would like x86 CPU hot-plug to
become setting/unsetting some /machine/cpu-socket[n] link<> property of
the machine, and the ipi_states array seems a close equivalent on s390x.

>> => Guest unaware of any emulated topology today.
> 
> An additional problem is, that for the normal case (linux scheduler, no pinning, also
> no gang scheduling) the topology would change too fast. The guest would be busy rebuilding
> the scheduler domains all the time.
[snip]

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 0/8] [PATCH RFC v3] s390 cpu hotplug

[Jason J. Herne]

On 09/05/2013 07:25 AM, Andreas Färber wrote:
> Am 05.09.2013 12:40, schrieb Christian Borntraeger:
>> On 04/09/13 14:45, Andreas Färber wrote:
...
> To cope with device_add s390-cpu adding the device to
> /machine/peripheral/<id> or /machine/peripheral-anon/device[0] I *think*
> we'll need link<>, which would then translate back to ipi_states array
> as backend and the remaining question would be where to expose those
> properties in the composition tree - i.e. /machine/cpu[n] or
> /machine/ipi/cpu[n] or something - please suggest. Similarly if those
> become link<> properties then the CPUs created by the machine via
> smp_cpus need a canonical path as well; quite obviously both cannot be
> the same.
>

Ok, if I understand right then /machine/peripheral[-anon]/device[n] is 
the canonical path given to a a cpu that is created via qdev_device_add. 
  We're going to create a link to that cpu via path 
/machine/cpu[cpu_addr].

I'm not sure what you meant by "if those become link<> properties then 
the CPUs created by the machine via smp_cpus need a canonical path as 
well".  Wouldn't the canonical path just be 
/machine/peripheral[-anon]/device[n]?  Are you saying we really want 
something different for non-hotplugged cpus?

> Background is that long-term Anthony would like x86 CPU hot-plug to
> become setting/unsetting some /machine/cpu-socket[n] link<> property of
> the machine, and the ipi_states array seems a close equivalent on s390x.
>
>>> => Guest unaware of any emulated topology today.
>>
>> An additional problem is, that for the normal case (linux scheduler, no pinning, also
>> no gang scheduling) the topology would change too fast. The guest would be busy rebuilding
>> the scheduler domains all the time.
> [snip]
>
> Regards,
> Andreas
>


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

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

[Alexander Graf]

On 01.08.2013, at 16:12, Jason J. Herne wrote:

> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Latest code for cpu Hotplug on S390 architecture.   This one is vastly simpler
> than v2 as we have decided to avoid the command line specification 
> of -device s390-cpu.
> 
> The last version can be found here:
> http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg01183.html
> 
> There is also a patch in this series to add cpu-add to the Qemu monitor
> interface.
> 
> 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. 

Very simple and clean patch set. I don't think it deserves the RFC tag.

Apart from the minor comments I had consider it

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

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

[Andreas Färber]

Am 05.09.2013 14:54, schrieb Alexander Graf:
> 
> On 01.08.2013, at 16:12, Jason J. Herne wrote:
> 
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Latest code for cpu Hotplug on S390 architecture.   This one is vastly simpler
>> than v2 as we have decided to avoid the command line specification 
>> of -device s390-cpu.
>>
>> The last version can be found here:
>> http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg01183.html
>>
>> There is also a patch in this series to add cpu-add to the Qemu monitor
>> interface.
>>
>> 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. 
> 
> Very simple and clean patch set. I don't think it deserves the RFC tag.

Negative, see my review. If you want to fix up and queue patches 1-2
that's fine with me, but the others need a respin. No major blocker
though, just some more footwork mostly related to QOM and Jason's
shifted focus on cpu-add rather than device_add.

Open issues:
* Might ipi_states need to become a device due to migration?
* QOM properties considerations
* Device creation in qdev initfn
* Parent field access
* QOM-unfriendly creation and reliance upon helper function

Andreas

> 
> Apart from the minor comments I had consider it
> 
> Reviewed-by: Alexander Graf <agraf@suse.de>
> 
> 
> Alex
> 


-- 
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 0/8] [PATCH RFC v3] s390 cpu hotplug

[Alexander Graf]

On 05.09.2013, at 15:05, Andreas Färber wrote:

> Am 05.09.2013 14:54, schrieb Alexander Graf:
>> 
>> On 01.08.2013, at 16:12, Jason J. Herne wrote:
>> 
>>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>> 
>>> Latest code for cpu Hotplug on S390 architecture.   This one is vastly simpler
>>> than v2 as we have decided to avoid the command line specification 
>>> of -device s390-cpu.
>>> 
>>> The last version can be found here:
>>> http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg01183.html
>>> 
>>> There is also a patch in this series to add cpu-add to the Qemu monitor
>>> interface.
>>> 
>>> 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. 
>> 
>> Very simple and clean patch set. I don't think it deserves the RFC tag.
> 
> Negative, see my review. If you want to fix up and queue patches 1-2
> that's fine with me, but the others need a respin. No major blocker
> though, just some more footwork mostly related to QOM and Jason's
> shifted focus on cpu-add rather than device_add.

Yeah, that's what I'm referring to. I've seen a lot worse patch sets at v8 than this RFC :).

I don't think we should apply it as is, and I'm very happy to see your review and comment on the modeling bits :). But I try to never apply or cherry pick RFC patches - and this set looks like he sent it with the intent of getting it merged.


Alex

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

[Andreas Färber]

Am 05.09.2013 15:10, schrieb Alexander Graf:
> On 05.09.2013, at 15:05, Andreas Färber wrote:
>> Am 05.09.2013 14:54, schrieb Alexander Graf:
>>> Very simple and clean patch set. I don't think it deserves the RFC tag.
>>
>> Negative, see my review. If you want to fix up and queue patches 1-2
>> that's fine with me, but the others need a respin. No major blocker
>> though, just some more footwork mostly related to QOM and Jason's
>> shifted focus on cpu-add rather than device_add.
> 
> Yeah, that's what I'm referring to. I've seen a lot worse patch sets at v8 than this RFC :).
> 
> I don't think we should apply it as is, and I'm very happy to see your review and comment on the modeling bits :). But I try to never apply or cherry pick RFC patches - and this set looks like he sent it with the intent of getting it merged.

Agreed, we can continue with "PATCH v4". I was more upset about the
"very simple and clean" bit after I commented on a number of unclean
things to improve - mostly about doing things in different places.

If you could find some time to review my two model string patches then I
could supply Jason with a branch or even a pull to base on:

http://patchwork.ozlabs.org/patch/272511/
http://patchwork.ozlabs.org/patch/272509/

I would also volunteer to provide a base patch for the link<> issue if
there is agreement. Apart from the QOM API question this depends on the
contradictory modelling of whether we allow CPU addresses 0..max_cpus as
seen in this series or 0..somemax with <= max_cpus non-NULL as discussed
on #zkvm.
(child<s390-cpu> properties would allow to model the latter sparse
address space very well, but an object can only have one parent in the
hot-add case. We could of course add cpu[n] link<s390-cpu> properties as
CPUs get added, but that doesn't strike me as very clean. My underlying
thought is to offload the error handling to QOM so that we don't start
hardcoding s/smp_cpus/max_cpus/g (or some max_cpu_address) all around
ipi_states.)

Btw an unanswered question: ipi_states is just pointers to CPUs
currently, no further state. So what's "ipi" in the name? Will that
array need to carry state beyond S390CPU someday?

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 0/8] [PATCH RFC v3] s390 cpu hotplug

[Jason J. Herne]

On 09/05/2013 10:06 AM, Andreas Färber wrote:
> Am 05.09.2013 15:10, schrieb Alexander Graf:
>> On 05.09.2013, at 15:05, Andreas Färber wrote:
>>> Am 05.09.2013 14:54, schrieb Alexander Graf:
>>>> Very simple and clean patch set. I don't think it deserves the RFC tag.
>>>
>>> Negative, see my review. If you want to fix up and queue patches 1-2
>>> that's fine with me, but the others need a respin. No major blocker
>>> though, just some more footwork mostly related to QOM and Jason's
>>> shifted focus on cpu-add rather than device_add.
>>
>> Yeah, that's what I'm referring to. I've seen a lot worse patch sets at v8 than this RFC :).
>>
>> I don't think we should apply it as is, and I'm very happy to see your review and comment on the modeling bits :). But I try to never apply or cherry pick RFC patches - and this set looks like he sent it with the intent of getting it merged.
>
> Agreed, we can continue with "PATCH v4". I was more upset about the
> "very simple and clean" bit after I commented on a number of unclean
> things to improve - mostly about doing things in different places.
>
> If you could find some time to review my two model string patches then I
> could supply Jason with a branch or even a pull to base on:
>
> http://patchwork.ozlabs.org/patch/272511/
> http://patchwork.ozlabs.org/patch/272509/
>
> I would also volunteer to provide a base patch for the link<> issue if
> there is agreement. Apart from the QOM API question this depends on the
> contradictory modelling of whether we allow CPU addresses 0..max_cpus as
> seen in this series or 0..somemax with <= max_cpus non-NULL as discussed
> on #zkvm.

According to http://wiki.qemu.org/Features/CPUHotplug:

"adding CPUs should be done in successive order from lower to higher IDs 
in [0..max-cpus) range.
It's possible to add arbitrary CPUs in random order, however that would 
cause migration to fail on its target side."

Considering that, in a virtual environment, it rarely (if ever) makes 
sense to define out of order cpu ids maybe we should keep the patch as 
is and only allow consecutive cpu ids to be used.

By extension, hot-unplug would require that the highest id be unplugged. 
This is probably not acceptable in any type of "mixed cpu" environment 
because the greatest id may not be the cpu type you want to remove.  I'm 
not sure if S390 will implement mixed cpu types.

> (child<s390-cpu> properties would allow to model the latter sparse
> address space very well, but an object can only have one parent in the
> hot-add case. We could of course add cpu[n] link<s390-cpu> properties as
> CPUs get added, but that doesn't strike me as very clean. My underlying
> thought is to offload the error handling to QOM so that we don't start
> hardcoding s/smp_cpus/max_cpus/g (or some max_cpu_address) all around
> ipi_states.)
>

I'm not sure I understand. What is meant by: "an object can only have 
one parent in the hot-add case."

What is the difference between "child<s390-cpu>" and "cpu[n] 
link<s390-cpu>"?  And why do you feel the link case would be unclean?

> Btw an unanswered question: ipi_states is just pointers to CPUs
> currently, no further state. So what's "ipi" in the name? Will that
> array need to carry state beyond S390CPU someday?
>

Quoting Jens Freimann:
"
The ipi_states array holds all our S390CPU states. The position of a cpu
within this array equals its "cpu address". See section "CPU address 
identification"
in the Principles of Operation. This cpu address is used for
cpu signalling (inter-processor interrupts->ipi) via the sigp instruction.
The cpu address does not contain information about which book this cpu is
plugged into, nor does it hold any other topology information.

When we intercept a sigp instruction in qemu the cpu address is passed
to us in a field of the SIE control block. We then use the cpu address
as an index to the ipi_states field, get hold of the S390CPU and then
for example do an vcpu ioctl on the chosen cpu.
"

Based on this explanation I do not think this array will ever be 
anything more than what it is today.

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

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

[Andreas Färber]

Am 13.09.2013 17:01, schrieb Jason J. Herne:
> On 09/05/2013 10:06 AM, Andreas Färber wrote:
>> Am 05.09.2013 15:10, schrieb Alexander Graf:
>>> On 05.09.2013, at 15:05, Andreas Färber wrote:
>>>> Am 05.09.2013 14:54, schrieb Alexander Graf:
>>>>> Very simple and clean patch set. I don't think it deserves the RFC
>>>>> tag.
>>>>
>>>> Negative, see my review. If you want to fix up and queue patches 1-2
>>>> that's fine with me, but the others need a respin. No major blocker
>>>> though, just some more footwork mostly related to QOM and Jason's
>>>> shifted focus on cpu-add rather than device_add.
>>>
>>> Yeah, that's what I'm referring to. I've seen a lot worse patch sets
>>> at v8 than this RFC :).
>>>
>>> I don't think we should apply it as is, and I'm very happy to see
>>> your review and comment on the modeling bits :). But I try to never
>>> apply or cherry pick RFC patches - and this set looks like he sent it
>>> with the intent of getting it merged.
>>
>> Agreed, we can continue with "PATCH v4". I was more upset about the
>> "very simple and clean" bit after I commented on a number of unclean
>> things to improve - mostly about doing things in different places.
>>
>> If you could find some time to review my two model string patches then I
>> could supply Jason with a branch or even a pull to base on:
>>
>> http://patchwork.ozlabs.org/patch/272511/
>> http://patchwork.ozlabs.org/patch/272509/
>>
>> I would also volunteer to provide a base patch for the link<> issue if
>> there is agreement. Apart from the QOM API question this depends on the
>> contradictory modelling of whether we allow CPU addresses 0..max_cpus as
>> seen in this series or 0..somemax with <= max_cpus non-NULL as discussed
>> on #zkvm.
> 
> According to http://wiki.qemu.org/Features/CPUHotplug:
> 
> "adding CPUs should be done in successive order from lower to higher IDs
> in [0..max-cpus) range.
> It's possible to add arbitrary CPUs in random order, however that would
> cause migration to fail on its target side."
> 
> Considering that, in a virtual environment, it rarely (if ever) makes
> sense to define out of order cpu ids maybe we should keep the patch as
> is and only allow consecutive cpu ids to be used.

Your previous series tried to make -device work in place of -smp. This
series now only seems to focus on cpu-add, including you referencing its
current limitations above.

As I tried to explain, x86 needed cpu-add because unlike s390x its CPU
is not yet a fully initialize'able QOM object and thus can't use
device_add yet (and long-term we want to use containers instead of
today's *-x86_64-cpu).

So I would very much prefer to see s390x continuing to use -smp but
using device_add for CPU hot-add and in a way where we don't have to
change semantics and ABI again when we implement hot-unplug. I am fine
with you implementing a cpu-add wrapper, but on top of a working
implementation please rather than limiting your implementation by it
upfront.

> By extension, hot-unplug would require that the highest id be unplugged.
> This is probably not acceptable in any type of "mixed cpu" environment
> because the greatest id may not be the cpu type you want to remove.  I'm
> not sure if S390 will implement mixed cpu types.
> 
>> (child<s390-cpu> properties would allow to model the latter sparse
>> address space very well, but an object can only have one parent in the
>> hot-add case. We could of course add cpu[n] link<s390-cpu> properties as
>> CPUs get added, but that doesn't strike me as very clean. My underlying
>> thought is to offload the error handling to QOM so that we don't start
>> hardcoding s/smp_cpus/max_cpus/g (or some max_cpu_address) all around
>> ipi_states.)
>>
> 
> I'm not sure I understand. What is meant by: "an object can only have
> one parent in the hot-add case."
> 
> What is the difference between "child<s390-cpu>" and "cpu[n]
> link<s390-cpu>"?  And why do you feel the link case would be unclean?

child<> properties determine the canonical path of an object. Each
object only has one canonical path. When using link<> properties, the
linked-to objects still need a canonical path, which becomes the string
value of the property on QMP level (pointer on C level). device_add
assigns canonical paths. Realizing a device creates a canonical path in
/machine/unassigned as fallback, but that won't work long-term as the
composition tree will be our source to find devices to realize, so
either the machine code or the S390CPU code should take care of assuring
CPUs have canonical paths before manually realizing them.

> 
>> Btw an unanswered question: ipi_states is just pointers to CPUs
>> currently, no further state. So what's "ipi" in the name? Will that
>> array need to carry state beyond S390CPU someday?
>>
> 
> Quoting Jens Freimann:
> "
> The ipi_states array holds all our S390CPU states. The position of a cpu
> within this array equals its "cpu address". See section "CPU address
> identification"
> in the Principles of Operation. This cpu address is used for
> cpu signalling (inter-processor interrupts->ipi) via the sigp instruction.
> The cpu address does not contain information about which book this cpu is
> plugged into, nor does it hold any other topology information.
> 
> When we intercept a sigp instruction in qemu the cpu address is passed
> to us in a field of the SIE control block. We then use the cpu address
> as an index to the ipi_states field, get hold of the S390CPU and then
> for example do an vcpu ioctl on the chosen cpu.
> "
> 
> Based on this explanation I do not think this array will ever be
> anything more than what it is today.

Thanks, seems I overread that.

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 0/8] [PATCH RFC v3] s390 cpu hotplug

[Michael Mueller]

On Fri, 13 Sep 2013 11:01:57 -0400
"Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:

> On 09/05/2013 10:06 AM, Andreas Färber wrote:
> > Am 05.09.2013 15:10, schrieb Alexander Graf:
> >> On 05.09.2013, at 15:05, Andreas Färber wrote:
> >>> Am 05.09.2013 14:54, schrieb Alexander Graf:
> >>>> Very simple and clean patch set. I don't think it deserves the RFC tag.
> >>>
> >>> Negative, see my review. If you want to fix up and queue patches 1-2
> >>> that's fine with me, but the others need a respin. No major blocker
> >>> though, just some more footwork mostly related to QOM and Jason's
> >>> shifted focus on cpu-add rather than device_add.
> >>
> >> Yeah, that's what I'm referring to. I've seen a lot worse patch sets at v8 than this RFC :).
> >>
> >> I don't think we should apply it as is, and I'm very happy to see your review and comment on
> >> the modeling bits :). But I try to never apply or cherry pick RFC patches - and this set
> >> looks like he sent it with the intent of getting it merged.
> >
> > Agreed, we can continue with "PATCH v4". I was more upset about the
> > "very simple and clean" bit after I commented on a number of unclean
> > things to improve - mostly about doing things in different places.
> >
> > If you could find some time to review my two model string patches then I
> > could supply Jason with a branch or even a pull to base on:
> >
> > http://patchwork.ozlabs.org/patch/272511/
> > http://patchwork.ozlabs.org/patch/272509/
> >
> > I would also volunteer to provide a base patch for the link<> issue if
> > there is agreement. Apart from the QOM API question this depends on the
> > contradictory modelling of whether we allow CPU addresses 0..max_cpus as
> > seen in this series or 0..somemax with <= max_cpus non-NULL as discussed
> > on #zkvm.
> 
> According to http://wiki.qemu.org/Features/CPUHotplug:
> 
> "adding CPUs should be done in successive order from lower to higher IDs 
> in [0..max-cpus) range.
> It's possible to add arbitrary CPUs in random order, however that would 
> cause migration to fail on its target side."
> 
> Considering that, in a virtual environment, it rarely (if ever) makes 
> sense to define out of order cpu ids maybe we should keep the patch as 
> is and only allow consecutive cpu ids to be used.
> 
> By extension, hot-unplug would require that the highest id be unplugged. 
> This is probably not acceptable in any type of "mixed cpu" environment 
> because the greatest id may not be the cpu type you want to remove.  I'm 
> not sure if S390 will implement mixed cpu types.

As zArch implements only one CPU type in its CEC we also will allow only virtual CPUs of the same
type in one KVM.

> 
> > (child<s390-cpu> properties would allow to model the latter sparse
> > address space very well, but an object can only have one parent in the
> > hot-add case. We could of course add cpu[n] link<s390-cpu> properties as
> > CPUs get added, but that doesn't strike me as very clean. My underlying
> > thought is to offload the error handling to QOM so that we don't start
> > hardcoding s/smp_cpus/max_cpus/g (or some max_cpu_address) all around
> > ipi_states.)
> >
> 
> I'm not sure I understand. What is meant by: "an object can only have 
> one parent in the hot-add case."
> 
> What is the difference between "child<s390-cpu>" and "cpu[n] 
> link<s390-cpu>"?  And why do you feel the link case would be unclean?
> 
> > Btw an unanswered question: ipi_states is just pointers to CPUs
> > currently, no further state. So what's "ipi" in the name? Will that
> > array need to carry state beyond S390CPU someday?
> >
> 
> Quoting Jens Freimann:
> "
> The ipi_states array holds all our S390CPU states. The position of a cpu
> within this array equals its "cpu address". See section "CPU address 
> identification"
> in the Principles of Operation. This cpu address is used for
> cpu signalling (inter-processor interrupts->ipi) via the sigp instruction.
> The cpu address does not contain information about which book this cpu is
> plugged into, nor does it hold any other topology information.
> 
> When we intercept a sigp instruction in qemu the cpu address is passed
> to us in a field of the SIE control block. We then use the cpu address
> as an index to the ipi_states field, get hold of the S390CPU and then
> for example do an vcpu ioctl on the chosen cpu.
> "
> 
> Based on this explanation I do not think this array will ever be 
> anything more than what it is today.
>