From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60181) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afmA2-0005q6-7k for qemu-devel@nongnu.org; Tue, 15 Mar 2016 06:25:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afm9z-0003lB-Uu for qemu-devel@nongnu.org; Tue, 15 Mar 2016 06:25:10 -0400 Date: Tue, 15 Mar 2016 20:34:28 +1100 From: David Gibson Message-ID: <20160315093428.GB9032@voom> References: <1457672078-17307-1-git-send-email-bharata@linux.vnet.ibm.com> <1457672078-17307-7-git-send-email-bharata@linux.vnet.ibm.com> <20160314112523.1c43461f@nial.brq.redhat.com> <20160315091401.GB13176@in.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="hQiwHBbRI9kgIhsi" Content-Disposition: inline In-Reply-To: <20160315091401.GB13176@in.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH v2 6/9] spapr: CPU core device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao Cc: mjrosato@linux.vnet.ibm.com, agraf@suse.de, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, aik@ozlabs.ru, qemu-devel@nongnu.org, armbru@redhat.com, borntraeger@de.ibm.com, qemu-ppc@nongnu.org, pbonzini@redhat.com, Igor Mammedov , afaerber@suse.de, mdroth@linux.vnet.ibm.com --hQiwHBbRI9kgIhsi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 15, 2016 at 02:44:01PM +0530, Bharata B Rao wrote: > On Mon, Mar 14, 2016 at 11:25:23AM +0100, Igor Mammedov wrote: > > On Fri, 11 Mar 2016 10:24:35 +0530 > > Bharata B Rao wrote: > >=20 > > > Add sPAPR specific CPU core device that is based on generic CPU core = device. > > > Creating this core device will result in creation of all the CPU thre= ad > > > devices that are part of this core. > > >=20 > > > Introduce sPAPRMachineClass.dr_cpu_enabled to indicate support for > > > CPU core hotplug. Initialize boot time CPUs as core deivces and preve= nt > > > topologies that result in partially filled cores. Both of these are d= one > > > only if CPU core hotplug is supported. > > >=20 > > > Note: An unrelated change in the call to xics_system_init() is done > > > in this patch as it makes sense to use the local variable smt introdu= ced > > > in this patch instead of kvmppc_smt_threads() call here. > > >=20 > > > Signed-off-by: Bharata B Rao > > > --- > > > hw/ppc/Makefile.objs | 1 + > > > hw/ppc/spapr.c | 68 +++++++++++--- > > > hw/ppc/spapr_cpu_core.c | 199 ++++++++++++++++++++++++++++++= ++++++++++ > > > include/hw/ppc/spapr.h | 4 + > > > include/hw/ppc/spapr_cpu_core.h | 28 ++++++ > > > 5 files changed, 287 insertions(+), 13 deletions(-) > > > create mode 100644 hw/ppc/spapr_cpu_core.c > > > create mode 100644 include/hw/ppc/spapr_cpu_core.h > > >=20 > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs > > > index c1ffc77..5cc6608 100644 > > > --- a/hw/ppc/Makefile.objs > > > +++ b/hw/ppc/Makefile.objs > > > @@ -4,6 +4,7 @@ obj-y +=3D ppc.o ppc_booke.o > > > obj-$(CONFIG_PSERIES) +=3D spapr.o spapr_vio.o spapr_events.o > > > obj-$(CONFIG_PSERIES) +=3D spapr_hcall.o spapr_iommu.o spapr_rtas.o > > > obj-$(CONFIG_PSERIES) +=3D spapr_pci.o spapr_rtc.o spapr_drc.o spapr= _rng.o > > > +obj-$(CONFIG_PSERIES) +=3D spapr_cpu_core.o > > > ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy) > > > obj-y +=3D spapr_pci_vfio.o > > > endif > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 64c4acc..cffe8c8 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -64,6 +64,7 @@ > > > =20 > > > #include "hw/compat.h" > > > #include "qemu-common.h" > > > +#include "hw/ppc/spapr_cpu_core.h" > > > =20 > > > #include > > > =20 > > > @@ -1180,7 +1181,7 @@ static void ppc_spapr_reset(void) > > > =20 > > > } > > > =20 > > > -static void spapr_cpu_reset(void *opaque) > > > +void spapr_cpu_reset(void *opaque) > > > { > > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > > PowerPCCPU *cpu =3D opaque; > > > @@ -1614,8 +1615,11 @@ static void spapr_boot_set(void *opaque, const= char *boot_device, > > > machine->boot_order =3D g_strdup(boot_device); > > > } > > > =20 > > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > > > - Error **errp) > > > +/* > > > + * TODO: Check if some of these can be moved to rtas_start_cpu() whe= re > > > + * a few other things required for hotplugged CPUs are being done. > > > + */ > > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error= **errp) > > > { > > > CPUPPCState *env =3D &cpu->env; > > > =20 > > > @@ -1728,7 +1732,6 @@ static void ppc_spapr_init(MachineState *machin= e) > > > const char *kernel_filename =3D machine->kernel_filename; > > > const char *kernel_cmdline =3D machine->kernel_cmdline; > > > const char *initrd_filename =3D machine->initrd_filename; > > > - PowerPCCPU *cpu; > > > PCIHostState *phb; > > > int i; > > > MemoryRegion *sysmem =3D get_system_memory(); > > > @@ -1742,6 +1745,22 @@ static void ppc_spapr_init(MachineState *machi= ne) > > > long load_limit, fw_size; > > > bool kernel_le =3D false; > > > char *filename; > > > + int smt =3D kvmppc_smt_threads(); > > > + int spapr_cores =3D smp_cpus / smp_threads; > > > + int spapr_max_cores =3D max_cpus / smp_threads; > > > + > > > + if (smc->dr_cpu_enabled) { > > > + if (smp_cpus % smp_threads) { > > > + error_report("smp_cpus (%u) must be multiple of threads = (%u)", > > > + smp_cpus, smp_threads); > > > + exit(1); > > > + } > > > + if (max_cpus % smp_threads) { > > > + error_report("max_cpus (%u) must be multiple of threads = (%u)", > > > + max_cpus, smp_threads); > > > + exit(1); > > > + } > > > + } > > > =20 > > > msi_supported =3D true; > > > =20 > > > @@ -1788,8 +1807,7 @@ static void ppc_spapr_init(MachineState *machin= e) > > > =20 > > > /* Set up Interrupt Controller before we create the VCPUs */ > > > spapr->icp =3D xics_system_init(machine, > > > - DIV_ROUND_UP(max_cpus * kvmppc_smt= _threads(), > > > - smp_threads), > > > + DIV_ROUND_UP(max_cpus * smt, smp_t= hreads), > > is smt =3D=3D smp_threads, if not then what's a difference? >=20 > smp_threads is the specified SMT mode of the guest, smt defines the max > SMT guest mode that can be supported on this host. >=20 > BTW as I noted in the patch description, this change is unrelated to > CPU hotplug. I did this since I introduced a separate variable (smt) > for kvmppc_smt_threads() and hence replaced the above use of > kvmppc_smt_threads() too. >=20 > >=20 > > > XICS_IRQS, &error_fatal); > > > =20 > > > if (smc->dr_lmb_enabled) { > > > @@ -1800,13 +1818,34 @@ static void ppc_spapr_init(MachineState *mach= ine) > > > if (machine->cpu_model =3D=3D NULL) { > > > machine->cpu_model =3D kvm_enabled() ? "host" : "POWER7"; > > > } > > > - for (i =3D 0; i < smp_cpus; i++) { > > > - cpu =3D cpu_ppc_init(machine->cpu_model); > > > - if (cpu =3D=3D NULL) { > > > - error_report("Unable to find PowerPC CPU definition"); > > > - exit(1); > > > + > > > + if (smc->dr_cpu_enabled) { > > > + spapr->cores =3D g_new0(Object *, spapr_max_cores); > > > + > > > + for (i =3D 0; i < spapr_max_cores; i++) { > > > + int core_dt_id =3D i * smt; > > > + > > > + if (i < spapr_cores) { > > > + Object *core =3D object_new(TYPE_SPAPR_CPU_CORE); > > > + > > > + object_property_set_str(core, machine->cpu_model, "c= pu_model", > > > + &error_fatal); > > > + object_property_set_int(core, smp_threads, "threads", > > > + &error_fatal); > > > + object_property_set_int(core, core_dt_id, CPU_CORE_P= ROP_CORE, > > > + &error_fatal); > > > + object_property_set_bool(core, true, "realized", &er= ror_fatal); > > > + } > > > } > > > - spapr_cpu_init(spapr, cpu, &error_fatal); > > > + } else { > > > + for (i =3D 0; i < smp_cpus; i++) { > > > + PowerPCCPU *cpu =3D cpu_ppc_init(machine->cpu_model); > > > + if (cpu =3D=3D NULL) { > > > + error_report("Unable to find PowerPC CPU definition"= ); > > > + exit(1); > > > + } > > > + spapr_cpu_init(spapr, cpu, &error_fatal); > > > + } > > > } > > > =20 > > > if (kvm_enabled()) { > > > @@ -2261,7 +2300,8 @@ static void spapr_machine_device_unplug(Hotplug= Handler *hotplug_dev, > > > static HotplugHandler *spapr_get_hotpug_handler(MachineState *machin= e, > > > 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_SPAPR_CPU_CORE)) { > > > return HOTPLUG_HANDLER(machine); > > > } > > > return NULL; > > > @@ -2305,6 +2345,7 @@ static void spapr_machine_class_init(ObjectClas= s *oc, void *data) > > > mc->cpu_index_to_socket_id =3D spapr_cpu_index_to_socket_id; > > > =20 > > > smc->dr_lmb_enabled =3D true; > > > + smc->dr_cpu_enabled =3D true; > > > fwc->get_dev_path =3D spapr_get_fw_dev_path; > > > nc->nmi_monitor_handler =3D spapr_nmi; > > > } > > > @@ -2384,6 +2425,7 @@ static void spapr_machine_2_5_class_options(Mac= hineClass *mc) > > > =20 > > > spapr_machine_2_6_class_options(mc); > > > smc->use_ohci_by_default =3D true; > > > + smc->dr_cpu_enabled =3D false; > > > SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_5); > > > } > > > =20 > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > new file mode 100644 > > > index 0000000..8c6d71d > > > --- /dev/null > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -0,0 +1,199 @@ > > > +/* > > > + * sPAPR CPU core device, acts as container of CPU thread devices. > > > + * > > > + * Copyright (C) 2016 Bharata B Rao > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2 o= r later. > > > + * See the COPYING file in the top-level directory. > > > + */ > > > +#include "hw/cpu/core.h" > > > +#include "hw/ppc/spapr_cpu_core.h" > > > +#include "hw/ppc/spapr.h" > > > +#include "hw/boards.h" > > > +#include "qemu/error-report.h" > > > +#include "qapi/visitor.h" > > > +#include > > > +#include "target-ppc/kvm_ppc.h" > > > + > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, int th= reads, > > > + Error **errp) > > > +{ > > > + int i; > > > + Error *local_err =3D NULL; > > > + > > > + for (i =3D 0; i < threads; i++) { > > > + char id[32]; > > > + > > > + object_initialize(&core->threads[i], sizeof(core->threads[i]= ), > > > + object_class_get_name(core->oc)); > > > + snprintf(id, sizeof(id), "thread[%d]", i); > > > + object_property_add_child(OBJECT(core), id, OBJECT(&core->th= reads[i]), > > > + &local_err); > > > + if (local_err) { > > > + goto err; > > > + } > > > + } > > > + return; > > > + > > > +err: > > > + while (--i) { > > > + object_unparent(OBJECT(&core->threads[i])); > > > + } > > > + error_propagate(errp, local_err); > > > +} > > > + > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque) > > > +{ > > > + Error **errp =3D opaque; > > > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > > + CPUState *cs =3D CPU(child); > > > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > > > + > > > + object_property_set_bool(child, true, "realized", errp); > > > + if (*errp) { > > > + return 1; > > > + } > > > + > > > + spapr_cpu_init(spapr, cpu, errp); > > > + if (*errp) { > > > + return 1; > > > + } > > > + > > > + spapr_cpu_reset(cpu); > > should it be move to spapr_cpu_init() ? >=20 > Could be moved. >=20 > >=20 > > > + return 0; > > > +} > > > + > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > +{ > > > + sPAPRCPUCore *core =3D SPAPR_CPU_CORE(OBJECT(dev)); > > > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > > + int spapr_max_cores =3D max_cpus / smp_threads; > > this should be in machine code >=20 > With ->pre_plug(), won't have this here. >=20 > >=20 > > > + Error *local_err =3D NULL; > > > + int threads =3D 0; > > > + int core_dt_id, core_id; > > > + int smt =3D kvmppc_smt_threads(); > > > + > > > + threads =3D object_property_get_int(OBJECT(dev), "threads", &loc= al_err); > > since spapr_core is inherited from cpu-core, > > you can cast to cpu-core and use fields directly here instead of using > > property setters. >=20 > Ah ok. >=20 > >=20 > > > + if (local_err) { > > > + goto out; > > > + } > > > + > > > + if (threads !=3D smp_threads) { > > > + error_setg(&local_err, "threads must be %d", smp_threads); > > > + goto out; > > > + } > > move to machine handler >=20 > Ok, guess you mean pre_plug handler ? >=20 > >=20 > > > + > > > + if (!core->oc) { > > > + error_setg(&local_err, "cpu_model property isn't set"); > > > + goto out; > > > + } > > > + > > > + core_dt_id =3D object_property_get_int(OBJECT(dev), "core", &loc= al_err); > > > + if (local_err) { > > > + goto out; > > > + } > > > + > > > + if (core_dt_id % smt) { > > > + error_setg(&local_err, "invalid core id %d\n", core_dt_id); > > > + goto out; > > > + } > > > + > > > + core_id =3D core_dt_id / smt; > > > + if (core_id < 0 || core_id >=3D spapr_max_cores) { > > > + error_setg(&local_err, "core id %d out of range", core_dt_id= ); > > > + goto out; > > > + } > > maybe due to nameing it's a bit confusing, > > what's difference between core_id and core_dt_id? >=20 > core_dt_id is the device tree IDs that we use with PowerPC cores. This is > what we use with "core" property of CPU_CORE. Since core_dt_id doesn't > grow contiguously (Eg. it will be 0, 8, 16 etc for SMT8 guest on a POWER8= host), > I am translating that to contiguous integer core_id so that I can > store the pointer of the realized core in the appropriate slot of > spapr->cpu_cores[] array. So, I see why the distinction is there, but it is kinda confusing. I'm wondering if we could do away with the spapr->cores array entirely and instead just access the core objects via the QOM tree - QOM "arrays" (i.e. properties named like foo[NNN]) can be sparse, so there's no need to allocate dense ids. Alternatively renaming "core_id" to "index" would be marginally less confusing. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --hQiwHBbRI9kgIhsi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW59ckAAoJEGw4ysog2bOSsXoQAMiSGZOHd7H7YzHjnoIz6nJE hb5Y4iyJXYpvY/j3XxlZegOoQJp3V7HN9kk4EK0YGha0vtTm8ewrctb8J8ryDGET N5IDo6YQflhfTn9SZW38DXt7SwN9VAbczfLCWNTGyUO0TZs5wg+PrIeCssiDlaJo D8EZAM/HKqKuENcoImkTMfQdYnX8KSotzLx1+6i+IG5MDtqicVLBOcxQdeJGDQhS aRQ+c0ayVFPvdBCuwTtNp1aj1ILjYEVg/ug46F4QQxlPG6H94rBcINoa+k7tzdRU G0Fp3EokZZjpwei4KrN6y/BnHO1PERU7qekYI9cL61cBC+gi5/iTaY+VmMwGugRF 8vbNe8w2EqwMrpd+9hue130kUp7+20XVqA2J/IHUXivwD4HU9ksMD5xWpyG2NTk5 YbYo2Tq4SZ5IWVzqw9VxHsJdB3fuqzLETB6cGPox2021J9lRhZTHVeJjK53TNv1R 3zulE/6rkEnPZ66ZhU/NN5gQCefQv5fk+3CqvZymyEX2mf6aLgOAESbxV7cdcirC Xc/XB4LzPS5O8QTgSP2kRzfeSRkwLwH1oWlNozq6Tc1IjRjiJApFAV4fuxOPVywD s5HbryAeZuUxtHxW0CsLwVUs6pzTtZPVPYCnike5CRKnUlEXpa16jl8esLyrzBOZ jdXd+ZnduJgcr/Zs6VvR =j6n1 -----END PGP SIGNATURE----- --hQiwHBbRI9kgIhsi--