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

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

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

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.