On Wed, 17 Feb 2016 15:12:34 -0500
Matthew Rosato <mjros...@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 <mjros...@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);
