| Message ID | 1456417362-20652-4-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 25, 2016 at 09:52:39PM +0530, Bharata B Rao wrote: > Initialize boot CPUs as spapr-cpu-core devices and create links from > machine object to these core devices. These links can be considered > as CPU slots in which core devices will get hot-plugged. spapr-cpu-core > device's slot property indicates the slot where it is plugged. Information > about all the CPU slots can be obtained by walking these links. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > hw/ppc/spapr.c | 65 +++++++++++++++++++++++++++++++++++++++++++------- > include/hw/ppc/spapr.h | 3 +++ > 2 files changed, 60 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index e214a34..1f0d232 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -64,6 +64,7 @@ > > #include "hw/compat.h" > #include "qemu-common.h" > +#include "hw/ppc/spapr_cpu_core.h" > > #include <libfdt.h> > > @@ -1720,6 +1721,21 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp) > } > } > > +/* > + * Check to see if core is being hot-plugged into an already populated slot. > + */ > +static void spapr_cpu_core_allow_set_link(Object *obj, const char *name, > + Object *val, Error **errp) > +{ > + Object *core = object_property_get_link(qdev_get_machine(), name, NULL); > + > + if (core) { > + char *path = object_get_canonical_path(core); > + error_setg(errp, "Slot %s already populated with %s", name, path); > + g_free(path); > + } > +} > + > /* pSeries LPAR / sPAPR hardware init */ > static void ppc_spapr_init(MachineState *machine) > { > @@ -1728,7 +1744,6 @@ static void ppc_spapr_init(MachineState *machine) > const char *kernel_filename = machine->kernel_filename; > const char *kernel_cmdline = machine->kernel_cmdline; > const char *initrd_filename = machine->initrd_filename; > - PowerPCCPU *cpu; > PCIHostState *phb; > int i; > MemoryRegion *sysmem = get_system_memory(); > @@ -1742,6 +1757,8 @@ static void ppc_spapr_init(MachineState *machine) > long load_limit, fw_size; > bool kernel_le = false; > char *filename; > + int spapr_cores = smp_cpus / smp_threads; > + int spapr_max_cores = max_cpus / smp_threads; > > msi_supported = true; > > @@ -1800,13 +1817,38 @@ static void ppc_spapr_init(MachineState *machine) > if (machine->cpu_model == NULL) { > machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; > } > - for (i = 0; i < smp_cpus; i++) { > - cpu = cpu_ppc_init(machine->cpu_model); > - if (cpu == NULL) { > - error_report("Unable to find PowerPC CPU definition"); > - exit(1); > + > + spapr->cores = g_malloc0(spapr_max_cores * sizeof(Object)); > + > + for (i = 0; i < spapr_max_cores; i++) { > + Object *spapr_cpu_core = object_new(TYPE_SPAPR_CPU_CORE); So.. if I understand correctly this will always construct maxcpus threads at startup. I's just that the ones beyond initial cpus won't be realized. Was that your intention? I thought the plan was to only construct the hotplugged cpus when they were hotplugged (in contrast to my earlier proposal). > + char name[32]; > + > + object_property_set_str(spapr_cpu_core, machine->cpu_model, "cpu_model", > + &error_fatal); > + object_property_set_int(spapr_cpu_core, smp_threads, "nr_threads", > + &error_fatal); > + /* > + * Create links from machine objects to all possible cores. > + */ > + snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i); Why do SPAPR_MACHINE_CPU_CORE_PROP and SPAPR_CPU_CORE_SLOT_PROP get #defines, but the other properties get bare strings? I find the bare strings are usually easier to follow. > + object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE, > + (Object **)&spapr->cores[i], > + spapr_cpu_core_allow_set_link, 0, > + &error_fatal); > + > + /* > + * Set the link from machine object to core object for all > + * boot time CPUs specified with -smp and realize them. > + * For rest of the hotpluggable cores this is happens from > + * the core hotplug/realization path. > + */ > + if (i < spapr_cores) { > + object_property_set_str(spapr_cpu_core, name, > + SPAPR_CPU_CORE_SLOT_PROP, &error_fatal); > + object_property_set_bool(spapr_cpu_core, true, "realized", > + &error_fatal); > } > - spapr_cpu_init(spapr, cpu, &error_fatal); > } > > if (kvm_enabled()) { > @@ -2209,6 +2251,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); > + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > int node; > @@ -2245,6 +2288,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > } > > spapr_memory_plug(hotplug_dev, dev, node, errp); > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > + CPUState *cs = CPU(dev); > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + > + spapr_cpu_init(ms, cpu, errp); I tend to thin the spapr_cpu_init() should be done from the core's create_threads() function, but I'm willing to be persuaded otherwise. > } > } > > @@ -2259,7 +2307,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine, > DeviceState *dev) > { > - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) || > + object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > return HOTPLUG_HANDLER(machine); > } > return NULL; > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 098d85d..20b3417 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -79,8 +79,11 @@ struct sPAPRMachineState { > /*< public >*/ > char *kvm_type; > MemoryHotplugState hotplug_memory; > + Object *cores; > }; > > +#define SPAPR_MACHINE_CPU_CORE_PROP "core" > + > #define H_SUCCESS 0 > #define H_BUSY 1 /* Hardware busy -- retry later */ > #define H_CLOSED 2 /* Resource closed */
On Fri, Feb 26, 2016 at 02:45:35PM +1100, David Gibson wrote: > On Thu, Feb 25, 2016 at 09:52:39PM +0530, Bharata B Rao wrote: > > Initialize boot CPUs as spapr-cpu-core devices and create links from > > machine object to these core devices. These links can be considered > > as CPU slots in which core devices will get hot-plugged. spapr-cpu-core > > device's slot property indicates the slot where it is plugged. Information > > about all the CPU slots can be obtained by walking these links. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > hw/ppc/spapr.c | 65 +++++++++++++++++++++++++++++++++++++++++++------- > > include/hw/ppc/spapr.h | 3 +++ > > 2 files changed, 60 insertions(+), 8 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index e214a34..1f0d232 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -64,6 +64,7 @@ > > > > #include "hw/compat.h" > > #include "qemu-common.h" > > +#include "hw/ppc/spapr_cpu_core.h" > > > > #include <libfdt.h> > > > > @@ -1720,6 +1721,21 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp) > > } > > } > > > > +/* > > + * Check to see if core is being hot-plugged into an already populated slot. > > + */ > > +static void spapr_cpu_core_allow_set_link(Object *obj, const char *name, > > + Object *val, Error **errp) > > +{ > > + Object *core = object_property_get_link(qdev_get_machine(), name, NULL); > > + > > + if (core) { > > + char *path = object_get_canonical_path(core); > > + error_setg(errp, "Slot %s already populated with %s", name, path); > > + g_free(path); > > + } > > +} > > + > > /* pSeries LPAR / sPAPR hardware init */ > > static void ppc_spapr_init(MachineState *machine) > > { > > @@ -1728,7 +1744,6 @@ static void ppc_spapr_init(MachineState *machine) > > const char *kernel_filename = machine->kernel_filename; > > const char *kernel_cmdline = machine->kernel_cmdline; > > const char *initrd_filename = machine->initrd_filename; > > - PowerPCCPU *cpu; > > PCIHostState *phb; > > int i; > > MemoryRegion *sysmem = get_system_memory(); > > @@ -1742,6 +1757,8 @@ static void ppc_spapr_init(MachineState *machine) > > long load_limit, fw_size; > > bool kernel_le = false; > > char *filename; > > + int spapr_cores = smp_cpus / smp_threads; > > + int spapr_max_cores = max_cpus / smp_threads; > > > > msi_supported = true; > > > > @@ -1800,13 +1817,38 @@ static void ppc_spapr_init(MachineState *machine) > > if (machine->cpu_model == NULL) { > > machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; > > } > > - for (i = 0; i < smp_cpus; i++) { > > - cpu = cpu_ppc_init(machine->cpu_model); > > - if (cpu == NULL) { > > - error_report("Unable to find PowerPC CPU definition"); > > - exit(1); > > + > > + spapr->cores = g_malloc0(spapr_max_cores * sizeof(Object)); > > + > > + for (i = 0; i < spapr_max_cores; i++) { > > + Object *spapr_cpu_core = object_new(TYPE_SPAPR_CPU_CORE); > > So.. if I understand correctly this will always construct maxcpus > threads at startup. I's just that the ones beyond initial cpus won't > be realized. Was that your intention? I thought the plan was to only > construct the hotplugged cpus when they were hotplugged (in contrast > to my earlier proposal). Oops! The intention was to to create only cores for smp_cpus and let device_add create the rest. Will fix. > > > + char name[32]; > > + > > + object_property_set_str(spapr_cpu_core, machine->cpu_model, "cpu_model", > > + &error_fatal); > > + object_property_set_int(spapr_cpu_core, smp_threads, "nr_threads", > > + &error_fatal); > > + /* > > + * Create links from machine objects to all possible cores. > > + */ > > + snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i); > > Why do SPAPR_MACHINE_CPU_CORE_PROP and SPAPR_CPU_CORE_SLOT_PROP get > #defines, but the other properties get bare strings? I find the bare > strings are usually easier to follow. Let me check the convention regarding properties and stick to the common usage. > > > + object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE, > > + (Object **)&spapr->cores[i], > > + spapr_cpu_core_allow_set_link, 0, > > + &error_fatal); > > + > > + /* > > + * Set the link from machine object to core object for all > > + * boot time CPUs specified with -smp and realize them. > > + * For rest of the hotpluggable cores this is happens from > > + * the core hotplug/realization path. > > + */ > > + if (i < spapr_cores) { > > + object_property_set_str(spapr_cpu_core, name, > > + SPAPR_CPU_CORE_SLOT_PROP, &error_fatal); > > + object_property_set_bool(spapr_cpu_core, true, "realized", > > + &error_fatal); > > } > > - spapr_cpu_init(spapr, cpu, &error_fatal); > > } > > > > if (kvm_enabled()) { > > @@ -2209,6 +2251,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > DeviceState *dev, Error **errp) > > { > > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); > > + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > > > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > int node; > > @@ -2245,6 +2288,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > } > > > > spapr_memory_plug(hotplug_dev, dev, node, errp); > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > > + CPUState *cs = CPU(dev); > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > + > > + spapr_cpu_init(ms, cpu, errp); > > I tend to thin the spapr_cpu_init() should be done from the core's > create_threads() function, but I'm willing to be persuaded otherwise. In addition to core device, every cpu thread device will go through ->plug() invocation via its realization path. And spapr_cpu_init() is needed for every CPU thread. Let me check if it is possible to invoke this from core's routines. Regards, Bharata.
On Thu, 25 Feb 2016 21:52:39 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > Initialize boot CPUs as spapr-cpu-core devices and create links from > machine object to these core devices. These links can be considered > as CPU slots in which core devices will get hot-plugged. spapr-cpu-core > device's slot property indicates the slot where it is plugged. Information > about all the CPU slots can be obtained by walking these links. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > hw/ppc/spapr.c | 65 +++++++++++++++++++++++++++++++++++++++++++------- > include/hw/ppc/spapr.h | 3 +++ > 2 files changed, 60 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index e214a34..1f0d232 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -64,6 +64,7 @@ > > #include "hw/compat.h" > #include "qemu-common.h" > +#include "hw/ppc/spapr_cpu_core.h" > > #include <libfdt.h> > > @@ -1720,6 +1721,21 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp) > } > } > > +/* > + * Check to see if core is being hot-plugged into an already populated slot. > + */ > +static void spapr_cpu_core_allow_set_link(Object *obj, const char *name, > + Object *val, Error **errp) > +{ > + Object *core = object_property_get_link(qdev_get_machine(), name, NULL); > + > + if (core) { > + char *path = object_get_canonical_path(core); > + error_setg(errp, "Slot %s already populated with %s", name, path); > + g_free(path); > + } > +} > + > /* pSeries LPAR / sPAPR hardware init */ > static void ppc_spapr_init(MachineState *machine) > { > @@ -1728,7 +1744,6 @@ static void ppc_spapr_init(MachineState *machine) > const char *kernel_filename = machine->kernel_filename; > const char *kernel_cmdline = machine->kernel_cmdline; > const char *initrd_filename = machine->initrd_filename; > - PowerPCCPU *cpu; > PCIHostState *phb; > int i; > MemoryRegion *sysmem = get_system_memory(); > @@ -1742,6 +1757,8 @@ static void ppc_spapr_init(MachineState *machine) > long load_limit, fw_size; > bool kernel_le = false; > char *filename; > + int spapr_cores = smp_cpus / smp_threads; > + int spapr_max_cores = max_cpus / smp_threads; > > msi_supported = true; > > @@ -1800,13 +1817,38 @@ static void ppc_spapr_init(MachineState *machine) > if (machine->cpu_model == NULL) { > machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; > } > - for (i = 0; i < smp_cpus; i++) { > - cpu = cpu_ppc_init(machine->cpu_model); > - if (cpu == NULL) { > - error_report("Unable to find PowerPC CPU definition"); > - exit(1); > + > + spapr->cores = g_malloc0(spapr_max_cores * sizeof(Object)); souldn't it be sizeof(Object *) > + > + for (i = 0; i < spapr_max_cores; i++) { > + Object *spapr_cpu_core = object_new(TYPE_SPAPR_CPU_CORE); you allocate spapr_max_cores cores but set links to only to spapr_cores only, it looks like all spapr_cpu_core objects are leaked for range spapr_cores..spapr_max_cores > + char name[32]; > + > + object_property_set_str(spapr_cpu_core, machine->cpu_model, "cpu_model", > + &error_fatal); > + object_property_set_int(spapr_cpu_core, smp_threads, "nr_threads", > + &error_fatal); > + /* > + * Create links from machine objects to all possible cores. > + */ > + snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i); > + object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE, > + (Object **)&spapr->cores[i], > + spapr_cpu_core_allow_set_link, 0, > + &error_fatal); > + > + /* > + * Set the link from machine object to core object for all > + * boot time CPUs specified with -smp and realize them. > + * For rest of the hotpluggable cores this is happens from > + * the core hotplug/realization path. > + */ > + if (i < spapr_cores) { > + object_property_set_str(spapr_cpu_core, name, > + SPAPR_CPU_CORE_SLOT_PROP, &error_fatal); > + object_property_set_bool(spapr_cpu_core, true, "realized", > + &error_fatal); > } > - spapr_cpu_init(spapr, cpu, &error_fatal); > } > > if (kvm_enabled()) { > @@ -2209,6 +2251,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); > + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > int node; > @@ -2245,6 +2288,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > } > > spapr_memory_plug(hotplug_dev, dev, node, errp); > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { here probably should be CORE and not TYPE_CPU, then if board needs to set some state for child threads it could ennumerate child of core here and do the required job. > + CPUState *cs = CPU(dev); > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + > + spapr_cpu_init(ms, cpu, errp); > } > } > > @@ -2259,7 +2307,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine, > DeviceState *dev) > { > - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) || > + object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > return HOTPLUG_HANDLER(machine); > } > return NULL; > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 098d85d..20b3417 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -79,8 +79,11 @@ struct sPAPRMachineState { > /*< public >*/ > char *kvm_type; > MemoryHotplugState hotplug_memory; > + Object *cores; > }; > > +#define SPAPR_MACHINE_CPU_CORE_PROP "core" > + > #define H_SUCCESS 0 > #define H_BUSY 1 /* Hardware busy -- retry later */ > #define H_CLOSED 2 /* Resource closed */
On Fri, Feb 26, 2016 at 04:18:57PM +0100, Igor Mammedov wrote: > On Thu, 25 Feb 2016 21:52:39 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > Initialize boot CPUs as spapr-cpu-core devices and create links from > > machine object to these core devices. These links can be considered > > as CPU slots in which core devices will get hot-plugged. spapr-cpu-core > > device's slot property indicates the slot where it is plugged. Information > > about all the CPU slots can be obtained by walking these links. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > hw/ppc/spapr.c | 65 +++++++++++++++++++++++++++++++++++++++++++------- > > include/hw/ppc/spapr.h | 3 +++ > > 2 files changed, 60 insertions(+), 8 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index e214a34..1f0d232 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -64,6 +64,7 @@ > > > > #include "hw/compat.h" > > #include "qemu-common.h" > > +#include "hw/ppc/spapr_cpu_core.h" > > > > #include <libfdt.h> > > > > @@ -1720,6 +1721,21 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp) > > } > > } > > > > +/* > > + * Check to see if core is being hot-plugged into an already populated slot. > > + */ > > +static void spapr_cpu_core_allow_set_link(Object *obj, const char *name, > > + Object *val, Error **errp) > > +{ > > + Object *core = object_property_get_link(qdev_get_machine(), name, NULL); > > + > > + if (core) { > > + char *path = object_get_canonical_path(core); > > + error_setg(errp, "Slot %s already populated with %s", name, path); > > + g_free(path); > > + } > > +} > > + > > /* pSeries LPAR / sPAPR hardware init */ > > static void ppc_spapr_init(MachineState *machine) > > { > > @@ -1728,7 +1744,6 @@ static void ppc_spapr_init(MachineState *machine) > > const char *kernel_filename = machine->kernel_filename; > > const char *kernel_cmdline = machine->kernel_cmdline; > > const char *initrd_filename = machine->initrd_filename; > > - PowerPCCPU *cpu; > > PCIHostState *phb; > > int i; > > MemoryRegion *sysmem = get_system_memory(); > > @@ -1742,6 +1757,8 @@ static void ppc_spapr_init(MachineState *machine) > > long load_limit, fw_size; > > bool kernel_le = false; > > char *filename; > > + int spapr_cores = smp_cpus / smp_threads; > > + int spapr_max_cores = max_cpus / smp_threads; > > > > msi_supported = true; > > > > @@ -1800,13 +1817,38 @@ static void ppc_spapr_init(MachineState *machine) > > if (machine->cpu_model == NULL) { > > machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; > > } > > - for (i = 0; i < smp_cpus; i++) { > > - cpu = cpu_ppc_init(machine->cpu_model); > > - if (cpu == NULL) { > > - error_report("Unable to find PowerPC CPU definition"); > > - exit(1); > > + > > + spapr->cores = g_malloc0(spapr_max_cores * sizeof(Object)); > souldn't it be sizeof(Object *) Yes, I meant to store the pointers to Object here, will change. > > > + > > + for (i = 0; i < spapr_max_cores; i++) { > > + Object *spapr_cpu_core = object_new(TYPE_SPAPR_CPU_CORE); > you allocate spapr_max_cores cores but set links to only to spapr_cores only, > it looks like all spapr_cpu_core objects are leaked for range spapr_cores..spapr_max_cores > Yes, as I replied in an ealier thread, the intention is to create links for all possible cores, but create only those many cores required for CPUs specified with -smp from machine init. The links will be set only for those cores. Hotplugged cores will get created and the links will be set for them during device_add. So the changed code now looks like this: for (i = 0; i < spapr_max_cores; i++) { char name[32]; /* * Create links from machine objects to all possible cores. */ snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i); object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE, (Object **)&spapr->cores[i], spapr_cpu_core_allow_set_link, 0, &error_fatal); /* * Create cores and set link from machine object to core object for * boot time CPUs and realize them. */ if (i < spapr_cores) { Object *core = object_new(TYPE_SPAPR_CPU_CORE); object_property_set_str(core, machine->cpu_model, "cpu_model", &error_fatal); object_property_set_int(core, smp_threads, "nr_threads", &error_fatal); object_property_set_str(core, name, SPAPR_CPU_CORE_SLOT_PROP, &error_fatal); object_property_set_bool(core, true, "realized", &error_fatal); } } > > > + char name[32]; > > + > > + object_property_set_str(spapr_cpu_core, machine->cpu_model, "cpu_model", > > + &error_fatal); > > + object_property_set_int(spapr_cpu_core, smp_threads, "nr_threads", > > + &error_fatal); > > + /* > > + * Create links from machine objects to all possible cores. > > + */ > > + snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i); > > + object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE, > > + (Object **)&spapr->cores[i], > > + spapr_cpu_core_allow_set_link, 0, > > + &error_fatal); > > + > > + /* > > + * Set the link from machine object to core object for all > > + * boot time CPUs specified with -smp and realize them. > > + * For rest of the hotpluggable cores this is happens from > > + * the core hotplug/realization path. > > + */ > > + if (i < spapr_cores) { > > + object_property_set_str(spapr_cpu_core, name, > > + SPAPR_CPU_CORE_SLOT_PROP, &error_fatal); > > + object_property_set_bool(spapr_cpu_core, true, "realized", > > + &error_fatal); > > } > > - spapr_cpu_init(spapr, cpu, &error_fatal); > > } > > > > if (kvm_enabled()) { > > @@ -2209,6 +2251,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > DeviceState *dev, Error **errp) > > { > > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); > > + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > > > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > int node; > > @@ -2245,6 +2288,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > } > > > > spapr_memory_plug(hotplug_dev, dev, node, errp); > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > here probably should be CORE and not TYPE_CPU, > then if board needs to set some state for child threads it > could ennumerate child of core here and do the required job. Hmm, we have things to set for CPUs and cores as well from hotplug handler. So the above code is for CPUs and I have spapr_core_plug() which is called conditionally when device is of type TYPE_SPAPR_CPU_CORE. Given that ->plug() is called during realization of both individual CPU threads as well as their parent core, I thought handling the setup for them separately like the above is simpler, no ? Regards, Bharata.
On Mon, Feb 29, 2016 at 11:05:32AM +0530, Bharata B Rao wrote: > On Fri, Feb 26, 2016 at 04:18:57PM +0100, Igor Mammedov wrote: > > On Thu, 25 Feb 2016 21:52:39 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: [snip] > > > @@ -2209,6 +2251,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > > DeviceState *dev, Error **errp) > > > { > > > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); > > > + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > > > > > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > > int node; > > > @@ -2245,6 +2288,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > > } > > > > > > spapr_memory_plug(hotplug_dev, dev, node, errp); > > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > > here probably should be CORE and not TYPE_CPU, > > then if board needs to set some state for child threads it > > could ennumerate child of core here and do the required job. > > Hmm, we have things to set for CPUs and cores as well from hotplug > handler. So the above code is for CPUs and I have spapr_core_plug() > which is called conditionally when device is of type TYPE_SPAPR_CPU_CORE. > > Given that ->plug() is called during realization of both individual CPU > threads as well as their parent core, I thought handling the setup for > them separately like the above is simpler, no ? So, this bothered me a bit as well. I think having the hotplug handler for the threads is misleading, since it suggests you can individually hotplug them, when in fact the intention is just that this is part of the hotplug code path for a whole core. My suggestion earlier was to have the core realize code itself perform this configuration on the sub-threads. However, Igor's suggestion of, essentially, an explicit loop over the threads in the core hotplug handler would be fine by me also.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index e214a34..1f0d232 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -64,6 +64,7 @@ #include "hw/compat.h" #include "qemu-common.h" +#include "hw/ppc/spapr_cpu_core.h" #include <libfdt.h> @@ -1720,6 +1721,21 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp) } } +/* + * Check to see if core is being hot-plugged into an already populated slot. + */ +static void spapr_cpu_core_allow_set_link(Object *obj, const char *name, + Object *val, Error **errp) +{ + Object *core = object_property_get_link(qdev_get_machine(), name, NULL); + + if (core) { + char *path = object_get_canonical_path(core); + error_setg(errp, "Slot %s already populated with %s", name, path); + g_free(path); + } +} + /* pSeries LPAR / sPAPR hardware init */ static void ppc_spapr_init(MachineState *machine) { @@ -1728,7 +1744,6 @@ static void ppc_spapr_init(MachineState *machine) const char *kernel_filename = machine->kernel_filename; const char *kernel_cmdline = machine->kernel_cmdline; const char *initrd_filename = machine->initrd_filename; - PowerPCCPU *cpu; PCIHostState *phb; int i; MemoryRegion *sysmem = get_system_memory(); @@ -1742,6 +1757,8 @@ static void ppc_spapr_init(MachineState *machine) long load_limit, fw_size; bool kernel_le = false; char *filename; + int spapr_cores = smp_cpus / smp_threads; + int spapr_max_cores = max_cpus / smp_threads; msi_supported = true; @@ -1800,13 +1817,38 @@ static void ppc_spapr_init(MachineState *machine) if (machine->cpu_model == NULL) { machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; } - for (i = 0; i < smp_cpus; i++) { - cpu = cpu_ppc_init(machine->cpu_model); - if (cpu == NULL) { - error_report("Unable to find PowerPC CPU definition"); - exit(1); + + spapr->cores = g_malloc0(spapr_max_cores * sizeof(Object)); + + for (i = 0; i < spapr_max_cores; i++) { + Object *spapr_cpu_core = object_new(TYPE_SPAPR_CPU_CORE); + char name[32]; + + object_property_set_str(spapr_cpu_core, machine->cpu_model, "cpu_model", + &error_fatal); + object_property_set_int(spapr_cpu_core, smp_threads, "nr_threads", + &error_fatal); + /* + * Create links from machine objects to all possible cores. + */ + snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i); + object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE, + (Object **)&spapr->cores[i], + spapr_cpu_core_allow_set_link, 0, + &error_fatal); + + /* + * Set the link from machine object to core object for all + * boot time CPUs specified with -smp and realize them. + * For rest of the hotpluggable cores this is happens from + * the core hotplug/realization path. + */ + if (i < spapr_cores) { + object_property_set_str(spapr_cpu_core, name, + SPAPR_CPU_CORE_SLOT_PROP, &error_fatal); + object_property_set_bool(spapr_cpu_core, true, "realized", + &error_fatal); } - spapr_cpu_init(spapr, cpu, &error_fatal); } if (kvm_enabled()) { @@ -2209,6 +2251,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { int node; @@ -2245,6 +2288,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, } spapr_memory_plug(hotplug_dev, dev, node, errp); + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { + CPUState *cs = CPU(dev); + PowerPCCPU *cpu = POWERPC_CPU(cs); + + spapr_cpu_init(ms, cpu, errp); } } @@ -2259,7 +2307,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine, DeviceState *dev) { - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) || + object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { return HOTPLUG_HANDLER(machine); } return NULL; diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 098d85d..20b3417 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -79,8 +79,11 @@ struct sPAPRMachineState { /*< public >*/ char *kvm_type; MemoryHotplugState hotplug_memory; + Object *cores; }; +#define SPAPR_MACHINE_CPU_CORE_PROP "core" + #define H_SUCCESS 0 #define H_BUSY 1 /* Hardware busy -- retry later */ #define H_CLOSED 2 /* Resource closed */
Initialize boot CPUs as spapr-cpu-core devices and create links from machine object to these core devices. These links can be considered as CPU slots in which core devices will get hot-plugged. spapr-cpu-core device's slot property indicates the slot where it is plugged. Information about all the CPU slots can be obtained by walking these links. Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 65 +++++++++++++++++++++++++++++++++++++++++++------- include/hw/ppc/spapr.h | 3 +++ 2 files changed, 60 insertions(+), 8 deletions(-)