[Qemu-devel] [PATCH 0/1] s390-cpu: qom interface for S390 cpu states array

[Qemu-devel] [PATCH 0/1] s390-cpu: qom interface for S390 cpu states array

[Jason J. Herne]

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

This patch is a result of changes requested by Andreas Färber during the
S390 cpu hotplug code review.

Andreas, 

I am unsure if you were asking for the complete removal of the
ipi_states array, or the removal of the set/get functions used to encapsulate
access to the array, or if you are ok with the current scheme and you just
wanted the data accesible within the qom tree as well. 

The approach I took with this patch was to leave the ipi_states array in place,
although it has been renamed for clarity. Because object_property_add_link()
expects Object** when pasing the link target, we must have a lasting reference
to our S390CPU objects. I'm not sure where we would store these pointers, if
not in the currently existing aray.

I decided to encapsulate the details in set/get functions instead of
(eventually) repeating all of the link related details when we allow the
creation of cpus via hotplug.

If you are unhappy with how I did this, please assist me in understanding what
you would like changed. I am happy to change it as needed.

Jason J. Herne (1):
  s390-cpu: qom interface for S390 cpu states array

 hw/s390x/s390-virtio.c | 32 +++++++++++++++++++++++---------
 target-s390x/cpu.h     |  1 +
 2 files changed, 24 insertions(+), 9 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/1] s390-cpu: qom interface for S390 cpu states array

[Jason J. Herne]

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

Rename the S390 ipi_states array to cpu_states to better reflect its contents.

Create machine/cpu[cpu_addr] links within the qom tree when creating a new cpu.

Encapsulate the qom tree linking process and the management of the cpu_states
array into helper functions.

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

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 9eeda97..82411e7 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -52,15 +52,33 @@
 #define ZIPL_FILENAME                   "s390-zipl.rom"
 
 static VirtIOS390Bus *s390_bus;
-static S390CPU **ipi_states;
+static S390CPU **cpu_states;
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
+    gchar *name;
+    Object *cpu;
+
     if (cpu_addr >= smp_cpus) {
         return NULL;
     }
 
-    return ipi_states[cpu_addr];
+    name = g_strdup_printf("cpu[%i]", cpu_addr);
+    cpu = object_property_get_link(qdev_get_machine(), name, NULL);
+
+    g_free(name);
+    return S390_CPU(cpu);
+}
+
+void s390_cpu_set_cpustate(uint16_t cpu_addr, S390CPU *state)
+{
+    gchar *name;
+
+    cpu_states[cpu_addr] = state;
+    name = g_strdup_printf("cpu[%i]", cpu_addr);
+    object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
+                            (Object **) &cpu_states[cpu_addr], NULL);
+    g_free(name);
 }
 
 static int s390_virtio_hcall_notify(const uint64_t *args)
@@ -184,16 +202,16 @@ void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
         cpu_model = "host";
     }
 
-    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
+    cpu_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
 
     for (i = 0; i < smp_cpus; i++) {
-        S390CPU *cpu;
+        S390CPU* cpu;
         CPUState *cs;
 
         cpu = cpu_s390x_init(cpu_model);
-        cs = CPU(cpu);
+        s390_cpu_set_cpustate(i, cpu);
 
-        ipi_states[i] = cpu;
+        cs = CPU(cpu_states[i]);
         cs->halted = 1;
         cpu->env.exception_index = EXCP_HLT;
         cpu->env.storage_keys = storage_keys;
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 96c2b4a..6ce3b64 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -370,6 +370,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
 }
 #endif
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
+void s390_cpu_set_cpustate(uint16_t cpu_addr, S390CPU *state);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
 
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 1/1] s390-cpu: qom interface for S390 cpu states array

[Paolo Bonzini]

Il 07/03/2014 17:29, Jason J. Herne ha scritto:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
>
> Rename the S390 ipi_states array to cpu_states to better reflect its contents.
>
> Create machine/cpu[cpu_addr] links within the qom tree when creating a new cpu.
>
> Encapsulate the qom tree linking process and the management of the cpu_states
> array into helper functions.
>
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  hw/s390x/s390-virtio.c | 30 ++++++++++++++++++++++++------
>  target-s390x/cpu.h     |  1 +
>  2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 9eeda97..82411e7 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -52,15 +52,33 @@
>  #define ZIPL_FILENAME                   "s390-zipl.rom"
>
>  static VirtIOS390Bus *s390_bus;
> -static S390CPU **ipi_states;
> +static S390CPU **cpu_states;
>
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> +    gchar *name;
> +    Object *cpu;
> +
>      if (cpu_addr >= smp_cpus) {
>          return NULL;
>      }
>
> -    return ipi_states[cpu_addr];
> +    name = g_strdup_printf("cpu[%i]", cpu_addr);
> +    cpu = object_property_get_link(qdev_get_machine(), name, NULL);
> +
> +    g_free(name);
> +    return S390_CPU(cpu);
> +}

QOM is too slow to be used in the data path.

I don't think you want a malloc + a linear scan of an array in 
css_inject_io_interrupt, so you should keep using cpu_states here.

Paolo

> +void s390_cpu_set_cpustate(uint16_t cpu_addr, S390CPU *state)
> +{
> +    gchar *name;
> +
> +    cpu_states[cpu_addr] = state;
> +    name = g_strdup_printf("cpu[%i]", cpu_addr);
> +    object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
> +                            (Object **) &cpu_states[cpu_addr], NULL);
> +    g_free(name);
>  }
>
>  static int s390_virtio_hcall_notify(const uint64_t *args)
> @@ -184,16 +202,16 @@ void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
>          cpu_model = "host";
>      }
>
> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> +    cpu_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
>
>      for (i = 0; i < smp_cpus; i++) {
> -        S390CPU *cpu;
> +        S390CPU* cpu;
>          CPUState *cs;
>
>          cpu = cpu_s390x_init(cpu_model);
> -        cs = CPU(cpu);
> +        s390_cpu_set_cpustate(i, cpu);
>
> -        ipi_states[i] = cpu;
> +        cs = CPU(cpu_states[i]);
>          cs->halted = 1;
>          cpu->env.exception_index = EXCP_HLT;
>          cpu->env.storage_keys = storage_keys;
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 96c2b4a..6ce3b64 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -370,6 +370,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
>  }
>  #endif
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> +void s390_cpu_set_cpustate(uint16_t cpu_addr, S390CPU *state);
>  void s390_add_running_cpu(S390CPU *cpu);
>  unsigned s390_del_running_cpu(S390CPU *cpu);
>
>

Re: [Qemu-devel] [PATCH 1/1] s390-cpu: qom interface for S390 cpu states array

[Jason J. Herne]

On 03/07/2014 12:03 PM, Paolo Bonzini wrote:
> Il 07/03/2014 17:29, Jason J. Herne ha scritto:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Rename the S390 ipi_states array to cpu_states to better reflect its
>> contents.
>>
>> Create machine/cpu[cpu_addr] links within the qom tree when creating a
>> new cpu.
>>
>> Encapsulate the qom tree linking process and the management of the
>> cpu_states
>> array into helper functions.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>> ---
>>  hw/s390x/s390-virtio.c | 30 ++++++++++++++++++++++++------
>>  target-s390x/cpu.h     |  1 +
>>  2 files changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index 9eeda97..82411e7 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -52,15 +52,33 @@
>>  #define ZIPL_FILENAME                   "s390-zipl.rom"
>>
>>  static VirtIOS390Bus *s390_bus;
>> -static S390CPU **ipi_states;
>> +static S390CPU **cpu_states;
>>
>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>  {
>> +    gchar *name;
>> +    Object *cpu;
>> +
>>      if (cpu_addr >= smp_cpus) {
>>          return NULL;
>>      }
>>
>> -    return ipi_states[cpu_addr];
>> +    name = g_strdup_printf("cpu[%i]", cpu_addr);
>> +    cpu = object_property_get_link(qdev_get_machine(), name, NULL);
>> +
>> +    g_free(name);
>> +    return S390_CPU(cpu);
>> +}
>
> QOM is too slow to be used in the data path.
>
> I don't think you want a malloc + a linear scan of an array in
> css_inject_io_interrupt, so you should keep using cpu_states here.
>

Hi Paolo, I agree. Provided Andreas also agrees I'll include this change 
in my next patch. Thanks.

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