diff mbox

[v4,4/5] s390x/cpu: Add functions to register CPU state

Message ID 1455739955-28139-5-git-send-email-mjrosato@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Matthew Rosato Feb. 17, 2016, 8:12 p.m. UTC 
Introduce s390_register_cpustate, which will set the
machine/cpu[n] link with the current CPU state.  Additionally,
maintain an array of state pointers indexed by CPU id for fast lookup
during interrupt handling.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio.c | 45 ++++++++++++++++++++++++++++++++++++---------
 target-s390x/cpu.c     | 14 +++++++++++++-
 target-s390x/cpu.h     |  1 +
 3 files changed, 50 insertions(+), 10 deletions(-)

Comments

David Hildenbrand Feb. 18, 2016, 8:57 a.m. UTC | #1 
> Introduce s390_register_cpustate, which will set the
> machine/cpu[n] link with the current CPU state.  Additionally,
> maintain an array of state pointers indexed by CPU id for fast lookup
> during interrupt handling.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

Acked-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

David
Igor Mammedov Feb. 18, 2016, 9:36 a.m. UTC | #2 
On Wed, 17 Feb 2016 15:12:34 -0500
Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:

> Introduce s390_register_cpustate, which will set the
> machine/cpu[n] link with the current CPU state.  Additionally,
> maintain an array of state pointers indexed by CPU id for fast lookup
> during interrupt handling.
> 
cosmetic nit,
you call qdev_get_machine() several time withing single function
or when function already has machine argument. You probably shouldn't
do it or do it once and use return value throughout function.

> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio.c | 45 ++++++++++++++++++++++++++++++++++++---------
>  target-s390x/cpu.c     | 14 +++++++++++++-
>  target-s390x/cpu.h     |  1 +
>  3 files changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index b3707f4..6a1e3f6 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -60,15 +60,35 @@
>  #define S390_TOD_CLOCK_VALUE_MISSING    0x00
>  #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
>  
> -static S390CPU **ipi_states;
> +static S390CPU **cpu_states;
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> -    if (cpu_addr >= smp_cpus) {
> +    if (cpu_addr >= max_cpus) {
>          return NULL;
>      }
>  
> -    return ipi_states[cpu_addr];
> +    /* Fast lookup via CPU ID */
> +    return cpu_states[cpu_addr];
> +}
> +
> +int s390_register_cpustate(uint16_t cpu_addr, S390CPU *state)
isn't it the board responsibility to link CPU somewhere?
I'd suggest to use for it generic device hotplug hooks
see how get_hotplug_handler() and pc_machine_device_plug_cb() are used,
it's executed after CPU's realize is successfully completed.

> +{
> +    gchar *name;
> +    int r = 0;
> +
> +    name = g_strdup_printf("cpu[%i]", cpu_addr);
> +    if (object_property_get_link(qdev_get_machine(), name, NULL)) {
> +        r = -EEXIST;
> +        goto out;
> +    }
> +
> +    object_property_set_link(qdev_get_machine(), OBJECT(state), name,\
> +                             &error_abort);
> +
> +out:
> +    g_free(name);
> +    return r;
>  }
>  
>  void s390_init_ipl_dev(const char *kernel_filename,
> @@ -98,19 +118,26 @@ void s390_init_ipl_dev(const char *kernel_filename,
>  void s390_init_cpus(MachineState *machine)
>  {
>      int i;
> +    gchar *name;
>  
>      if (machine->cpu_model == NULL) {
>          machine->cpu_model = "host";
>      }
>  
> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> -
> -    for (i = 0; i < smp_cpus; i++) {
> -        S390CPU *cpu;
> +    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
>  
> -        cpu = cpu_s390x_init(machine->cpu_model);
> +    for (i = 0; i < max_cpus; i++) {
> +        name = g_strdup_printf("cpu[%i]", i);
> +        object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
> +                                 (Object **) &cpu_states[i],
> +                                 object_property_allow_set_link,
> +                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
> +                                 &error_abort);
> +        g_free(name);
> +    }
>  
> -        ipi_states[i] = cpu;
> +    for (i = 0; i < smp_cpus; i++) {
> +        cpu_s390x_init(machine->cpu_model);
>      }
>  }
>  
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 5f6411f..620b2e3 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -32,6 +32,7 @@
>  #include "trace.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #endif
>  
>  #define CR0_RESET       0xE0UL
> @@ -202,9 +203,20 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUS390XState *env = &cpu->env;
>  
>  #if !defined(CONFIG_USER_ONLY)
> +    if (s390_register_cpustate(next_cpu_id, cpu) < 0) {
> +        error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
> +        return;
> +    }
>      qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> -#endif
> +    env->cpu_num = next_cpu_id;
> +    while (next_cpu_id < max_cpus - 1) {
> +        if (!cpu_exists(++next_cpu_id)) {
> +            break;
> +        }
> +    }
maybe it's possible to reuse cpu_get_free_index() here?


> +#else
>      env->cpu_num = next_cpu_id++;
> +#endif
>      s390_cpu_gdb_init(cs);
>      qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 06ca60b..429bf90 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -531,6 +531,7 @@ static inline int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>  }
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> +int s390_register_cpustate(uint16_t cpu_addr, S390CPU *state);
>  unsigned int s390_cpu_halt(S390CPU *cpu);
>  void s390_cpu_unhalt(S390CPU *cpu);
>  unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
Matthew Rosato Feb. 18, 2016, 8:22 p.m. UTC | #3 
On 02/18/2016 04:36 AM, Igor Mammedov wrote:
> On Wed, 17 Feb 2016 15:12:34 -0500
> Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
> 
>> Introduce s390_register_cpustate, which will set the
>> machine/cpu[n] link with the current CPU state.  Additionally,
>> maintain an array of state pointers indexed by CPU id for fast lookup
>> during interrupt handling.
>>
> cosmetic nit,
> you call qdev_get_machine() several time withing single function
> or when function already has machine argument. You probably shouldn't
> do it or do it once and use return value throughout function.
> 

Oops, will fix that.

>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/s390-virtio.c | 45 ++++++++++++++++++++++++++++++++++++---------
>>  target-s390x/cpu.c     | 14 +++++++++++++-
>>  target-s390x/cpu.h     |  1 +
>>  3 files changed, 50 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index b3707f4..6a1e3f6 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -60,15 +60,35 @@
>>  #define S390_TOD_CLOCK_VALUE_MISSING    0x00
>>  #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
>>  
>> -static S390CPU **ipi_states;
>> +static S390CPU **cpu_states;
>>  
>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>  {
>> -    if (cpu_addr >= smp_cpus) {
>> +    if (cpu_addr >= max_cpus) {
>>          return NULL;
>>      }
>>  
>> -    return ipi_states[cpu_addr];
>> +    /* Fast lookup via CPU ID */
>> +    return cpu_states[cpu_addr];
>> +}
>> +
>> +int s390_register_cpustate(uint16_t cpu_addr, S390CPU *state)
> isn't it the board responsibility to link CPU somewhere?
> I'd suggest to use for it generic device hotplug hooks
> see how get_hotplug_handler() and pc_machine_device_plug_cb() are used,
> it's executed after CPU's realize is successfully completed.
> 

OK, I'll have a look at this.

>> +{
>> +    gchar *name;
>> +    int r = 0;
>> +
>> +    name = g_strdup_printf("cpu[%i]", cpu_addr);
>> +    if (object_property_get_link(qdev_get_machine(), name, NULL)) {
>> +        r = -EEXIST;
>> +        goto out;
>> +    }
>> +
>> +    object_property_set_link(qdev_get_machine(), OBJECT(state), name,\
>> +                             &error_abort);
>> +
>> +out:
>> +    g_free(name);
>> +    return r;
>>  }
>>  
>>  void s390_init_ipl_dev(const char *kernel_filename,
>> @@ -98,19 +118,26 @@ void s390_init_ipl_dev(const char *kernel_filename,
>>  void s390_init_cpus(MachineState *machine)
>>  {
>>      int i;
>> +    gchar *name;
>>  
>>      if (machine->cpu_model == NULL) {
>>          machine->cpu_model = "host";
>>      }
>>  
>> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
>> -
>> -    for (i = 0; i < smp_cpus; i++) {
>> -        S390CPU *cpu;
>> +    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
>>  
>> -        cpu = cpu_s390x_init(machine->cpu_model);
>> +    for (i = 0; i < max_cpus; i++) {
>> +        name = g_strdup_printf("cpu[%i]", i);
>> +        object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
>> +                                 (Object **) &cpu_states[i],
>> +                                 object_property_allow_set_link,
>> +                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
>> +                                 &error_abort);
>> +        g_free(name);
>> +    }
>>  
>> -        ipi_states[i] = cpu;
>> +    for (i = 0; i < smp_cpus; i++) {
>> +        cpu_s390x_init(machine->cpu_model);
>>      }
>>  }
>>  
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 5f6411f..620b2e3 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -32,6 +32,7 @@
>>  #include "trace.h"
>>  #ifndef CONFIG_USER_ONLY
>>  #include "sysemu/arch_init.h"
>> +#include "sysemu/sysemu.h"
>>  #endif
>>  
>>  #define CR0_RESET       0xE0UL
>> @@ -202,9 +203,20 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>>      CPUS390XState *env = &cpu->env;
>>  
>>  #if !defined(CONFIG_USER_ONLY)
>> +    if (s390_register_cpustate(next_cpu_id, cpu) < 0) {
>> +        error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
>> +        return;
>> +    }
>>      qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>> -#endif
>> +    env->cpu_num = next_cpu_id;
>> +    while (next_cpu_id < max_cpus - 1) {
>> +        if (!cpu_exists(++next_cpu_id)) {
>> +            break;
>> +        }
>> +    }
> maybe it's possible to reuse cpu_get_free_index() here?
> 

Hmm, but based on your other comment, there's no need to track
next_cpu_id in this fashion afterall.  To bring these 2 points together,
how about the following:

1) In Patch 4 (this patch), drop next_cpu_id.  Since we don't allow
unplug and only do sequential cpu adds, rely on env->cpu_num =
cs->cpu_index here in realizefn.

2) For hotplug in patch 5, check against QTAILQ_LAST(&cpus, CPUTailQ) as
you suggest.

I tried this out quick, seems to work fine.

Matt
diff mbox

Patch

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index b3707f4..6a1e3f6 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -60,15 +60,35 @@ 
 #define S390_TOD_CLOCK_VALUE_MISSING    0x00
 #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
 
-static S390CPU **ipi_states;
+static S390CPU **cpu_states;
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
-    if (cpu_addr >= smp_cpus) {
+    if (cpu_addr >= max_cpus) {
         return NULL;
     }
 
-    return ipi_states[cpu_addr];
+    /* Fast lookup via CPU ID */
+    return cpu_states[cpu_addr];
+}
+
+int s390_register_cpustate(uint16_t cpu_addr, S390CPU *state)
+{
+    gchar *name;
+    int r = 0;
+
+    name = g_strdup_printf("cpu[%i]", cpu_addr);
+    if (object_property_get_link(qdev_get_machine(), name, NULL)) {
+        r = -EEXIST;
+        goto out;
+    }
+
+    object_property_set_link(qdev_get_machine(), OBJECT(state), name,\
+                             &error_abort);
+
+out:
+    g_free(name);
+    return r;
 }
 
 void s390_init_ipl_dev(const char *kernel_filename,
@@ -98,19 +118,26 @@  void s390_init_ipl_dev(const char *kernel_filename,
 void s390_init_cpus(MachineState *machine)
 {
     int i;
+    gchar *name;
 
     if (machine->cpu_model == NULL) {
         machine->cpu_model = "host";
     }
 
-    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
-
-    for (i = 0; i < smp_cpus; i++) {
-        S390CPU *cpu;
+    cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
 
-        cpu = cpu_s390x_init(machine->cpu_model);
+    for (i = 0; i < max_cpus; i++) {
+        name = g_strdup_printf("cpu[%i]", i);
+        object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
+                                 (Object **) &cpu_states[i],
+                                 object_property_allow_set_link,
+                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                                 &error_abort);
+        g_free(name);
+    }
 
-        ipi_states[i] = cpu;
+    for (i = 0; i < smp_cpus; i++) {
+        cpu_s390x_init(machine->cpu_model);
     }
 }
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 5f6411f..620b2e3 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -32,6 +32,7 @@ 
 #include "trace.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
+#include "sysemu/sysemu.h"
 #endif
 
 #define CR0_RESET       0xE0UL
@@ -202,9 +203,20 @@  static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUS390XState *env = &cpu->env;
 
 #if !defined(CONFIG_USER_ONLY)
+    if (s390_register_cpustate(next_cpu_id, cpu) < 0) {
+        error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
+        return;
+    }
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
-#endif
+    env->cpu_num = next_cpu_id;
+    while (next_cpu_id < max_cpus - 1) {
+        if (!cpu_exists(++next_cpu_id)) {
+            break;
+        }
+    }
+#else
     env->cpu_num = next_cpu_id++;
+#endif
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 06ca60b..429bf90 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -531,6 +531,7 @@  static inline int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
 }
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
+int s390_register_cpustate(uint16_t cpu_addr, S390CPU *state);
 unsigned int s390_cpu_halt(S390CPU *cpu);
 void s390_cpu_unhalt(S390CPU *cpu);
 unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);