From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34666) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c3z4Z-0000Iq-1W for qemu-devel@nongnu.org; Tue, 08 Nov 2016 00:35:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c3z4W-0002Fx-IE for qemu-devel@nongnu.org; Tue, 08 Nov 2016 00:35:51 -0500 Date: Tue, 8 Nov 2016 16:34:45 +1100 From: David Gibson Message-ID: <20161108053445.GX28688@umbus.fritz.box> References: <1477825928-10803-1-git-send-email-david@gibson.dropbear.id.au> <1477825928-10803-4-git-send-email-david@gibson.dropbear.id.au> <20161104105140.4703b2b2@bahia> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="QQ0dNM4HnH4+xgqD" Content-Disposition: inline In-Reply-To: <20161104105140.4703b2b2@bahia> Subject: Re: [Qemu-devel] [Qemu-ppc] [RFC 03/17] pseries: Always use core objects for CPU construction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Alexey Kardashevskiy , nikunj@linux.vnet.ibm.com, mdroth@linux.vnet.ibm.com, lvivier@redhat.com, thuth@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --QQ0dNM4HnH4+xgqD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 04, 2016 at 10:51:40AM +0100, Greg Kurz wrote: > On Thu, 3 Nov 2016 19:11:48 +1100 > Alexey Kardashevskiy wrote: >=20 > > On 30/10/16 22:11, David Gibson wrote: > > > Currently the pseries machine has two paths for constructing CPUs. On > > > newer machine type versions, which support cpu hotplug, it constructs > > > cpu core objects, which in turn construct CPU threads. For older mac= hine > > > versions it individually constructs the CPU threads. > > >=20 > > > This division is going to make some future changes to the cpu constru= ction > > > harder, so this patch unifies them. Now cpu core objects are always > > > created. This requires some updates to allow core objects to be crea= ted > > > without a full complement of threads (since older versions allowed a > > > number of cpus not a multiple of the threads-per-core). Likewise it = needs > > > some changes to the cpu core hot/cold plug path so as not to choke on= the > > > old machine types without hotplug support. > > >=20 > > > For good measure, we move the cpu construction to its own subfunction, > > > spapr_init_cpus(). > > >=20 > > > Signed-off-by: David Gibson > > > --- > > > hw/ppc/spapr.c | 125 +++++++++++++++++++++++++++-----------= ---------- > > > hw/ppc/spapr_cpu_core.c | 30 +++++++----- > > > include/hw/ppc/spapr.h | 1 - > > > 3 files changed, 89 insertions(+), 67 deletions(-) > > >=20 > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index c8e2921..ad68a9d 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -1688,11 +1688,80 @@ static void spapr_validate_node_memory(Machin= eState *machine, Error **errp) > > > } > > > } > > > =20 > > > +static void spapr_init_cpus(sPAPRMachineState *spapr) > > > +{ > > > + MachineState *machine =3D MACHINE(spapr); > > > + MachineClass *mc =3D MACHINE_GET_CLASS(machine); > > > + char *type =3D spapr_get_cpu_core_type(machine->cpu_model); > > > + int smt =3D kvmppc_smt_threads(); > > > + int spapr_max_cores, spapr_cores; > > > + int i; > > > + > > > + if (!type) { > > > + error_report("Unable to find sPAPR CPU Core definition"); > > > + exit(1); > > > + } > > > + > > > + if (mc->query_hotpluggable_cpus) { > > > + 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); > > > + } > > > + > > > + spapr_max_cores =3D max_cpus / smp_threads; > > > + spapr_cores =3D smp_cpus / smp_threads; > > > + } else { > > > + if (max_cpus !=3D smp_cpus) { > > > + error_report("This machine version does not support CPU = hotplug"); > > > + exit(1); > > > + } > > > + > > > + spapr_max_cores =3D QEMU_ALIGN_UP(smp_cpus, smp_threads) / s= mp_threads; > > > + spapr_cores =3D spapr_max_cores; > > > + } > > > + > > > + spapr->cores =3D g_new0(Object *, spapr_max_cores); > > > + for (i =3D 0; i < spapr_max_cores; i++) { > > > + int core_id =3D i * smp_threads; > > > + > > > + if (mc->query_hotpluggable_cpus) { > > > + sPAPRDRConnector *drc =3D > > > + spapr_dr_connector_new(OBJECT(spapr), > > > + SPAPR_DR_CONNECTOR_TYPE_CPU, > > > + (core_id / smp_threads) * smt= ); > > > + > > > + qemu_register_reset(spapr_drc_reset, drc); > > > + } > > > + > > > + if (i < spapr_cores) { > > > + Object *core =3D object_new(type); > > > + int nr_threads =3D smp_threads; > > > + > > > + /* Handle the partially filled core for older machine ty= pes */ > > > + if ((i + 1) * smp_threads >=3D smp_cpus) { > > > + nr_threads =3D smp_cpus - i * smp_threads; > > > + } =20 > >=20 > >=20 > > What is this exactly for? Older machines report "qemu-system-ppc64: thr= eads > > must be 8" when I do "-smp 12,threads=3D8 -machine pseries-2.2". > >=20 >=20 > IIUC, this lowers nr_threads for the last core to end up with the request= ed > number of vCPUs... but spapr_core_pre_plug() doesn't like partially filled > cores. >=20 > if (cc->nr_threads !=3D smp_threads) { > error_setg(&local_err, "threads must be %d", smp_threads); > goto out; > } Ah, yeah, that's a bug. I hadn't had a chance to test on real hardware yet, just TCG, which only supports 1 thread per core, so I hadn't spotted this. I'll fix it in the next spin. > BTW, this error message looks weird when ones has passed "-smp threads=3D= 8"... > It should better reads: >=20 > "unsupported partially filled core (%d threads, should have %d)" >=20 > If this check is removed, then we hit: >=20 > qemu-system-ppc64: core id 8 out of range >=20 > because of: >=20 > int spapr_max_cores =3D max_cpus / smp_threads; >=20 > index =3D cc->core_id / smp_threads; > if (index < 0 || index >=3D spapr_max_cores) { > error_setg(&local_err, "core id %d out of range", cc->core_id); > goto out; > } >=20 > Since the cc->core_id / smp_threads pattern is only used on the plug/unpl= ug > paths, maybe these checks in spapr_core_pre_plug() should only be done > when mc->query_hotpluggable_cpus !=3D NULL ? >=20 > >=20 > >=20 > > > + > > > + object_property_set_int(core, nr_threads, "nr-threads", > > > + &error_fatal); > > > + object_property_set_int(core, core_id, CPU_CORE_PROP_COR= E_ID, > > > + &error_fatal); > > > + object_property_set_bool(core, true, "realized", &error_= fatal); > > > + } > > > + } > > > + g_free(type); > > > +} > > > + > > > /* pSeries LPAR / sPAPR hardware init */ > > > static void ppc_spapr_init(MachineState *machine) > > > { > > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(machine); > > > - MachineClass *mc =3D MACHINE_GET_CLASS(machine); > > > sPAPRMachineClass *smc =3D SPAPR_MACHINE_GET_CLASS(machine); > > > const char *kernel_filename =3D machine->kernel_filename; > > > const char *initrd_filename =3D machine->initrd_filename; > > > @@ -1707,21 +1776,6 @@ static void ppc_spapr_init(MachineState *machi= ne) > > > long load_limit, fw_size; > > > 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 (mc->query_hotpluggable_cpus) { > > > - 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_nonbroken =3D true; > > > =20 > > > @@ -1801,44 +1855,7 @@ static void ppc_spapr_init(MachineState *machi= ne) > > > =20 > > > ppc_cpu_parse_features(machine->cpu_model); > > > =20 > > > - if (mc->query_hotpluggable_cpus) { > > > - char *type =3D spapr_get_cpu_core_type(machine->cpu_model); > > > - > > > - if (type =3D=3D NULL) { > > > - error_report("Unable to find sPAPR CPU Core definition"); > > > - exit(1); > > > - } > > > - > > > - spapr->cores =3D g_new0(Object *, spapr_max_cores); > > > - for (i =3D 0; i < spapr_max_cores; i++) { > > > - int core_id =3D i * smp_threads; > > > - sPAPRDRConnector *drc =3D > > > - spapr_dr_connector_new(OBJECT(spapr), > > > - SPAPR_DR_CONNECTOR_TYPE_CPU, > > > - (core_id / smp_threads) * smt= ); > > > - > > > - qemu_register_reset(spapr_drc_reset, drc); > > > - > > > - if (i < spapr_cores) { > > > - Object *core =3D object_new(type); > > > - object_property_set_int(core, smp_threads, "nr-threa= ds", > > > - &error_fatal); > > > - object_property_set_int(core, core_id, CPU_CORE_PROP= _CORE_ID, > > > - &error_fatal); > > > - object_property_set_bool(core, true, "realized", &er= ror_fatal); > > > - } > > > - } > > > - g_free(type); > > > - } 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); > > > - } > > > - } > > > + spapr_init_cpus(spapr); > > > =20 > > > if (kvm_enabled()) { > > > /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devic= es */ > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > index e0c14f6..1357293 100644 > > > --- a/hw/ppc/spapr_cpu_core.c > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -46,7 +46,8 @@ static void spapr_cpu_destroy(PowerPCCPU *cpu) > > > qemu_unregister_reset(spapr_cpu_reset, cpu); > > > } > > > =20 > > > -void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error= **errp) > > > +static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > > > + Error **errp) > > > { > > > CPUPPCState *env =3D &cpu->env; > > > CPUState *cs =3D CPU(cpu); > > > @@ -166,6 +167,7 @@ void spapr_core_plug(HotplugHandler *hotplug_dev,= DeviceState *dev, > > > Error **errp) > > > { > > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(OBJECT(hotplug_dev)); > > > + MachineClass *mc =3D MACHINE_GET_CLASS(spapr); > > > sPAPRCPUCore *core =3D SPAPR_CPU_CORE(OBJECT(dev)); > > > CPUCore *cc =3D CPU_CORE(dev); > > > CPUState *cs =3D CPU(core->threads); > > > @@ -180,7 +182,7 @@ void spapr_core_plug(HotplugHandler *hotplug_dev,= DeviceState *dev, > > > drc =3D spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, in= dex * smt); > > > spapr->cores[index] =3D OBJECT(dev); > > > =20 > > > - g_assert(drc); > > > + g_assert(drc || !mc->query_hotpluggable_cpus); > > > =20 > > > /* > > > * Setup CPU DT entries only for hotplugged CPUs. For boot time = or > > > @@ -190,13 +192,15 @@ void spapr_core_plug(HotplugHandler *hotplug_de= v, DeviceState *dev, > > > fdt =3D spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr= ); > > > } > > > =20 > > > - drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > > - drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local= _err); > > > - if (local_err) { > > > - g_free(fdt); > > > - spapr->cores[index] =3D NULL; > > > - error_propagate(errp, local_err); > > > - return; > > > + if (drc) { > > > + drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > > + drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &l= ocal_err); > > > + if (local_err) { > > > + g_free(fdt); > > > + spapr->cores[index] =3D NULL; > > > + error_propagate(errp, local_err); > > > + return; > > > + } > > > } > > > =20 > > > if (dev->hotplugged) { > > > @@ -209,8 +213,10 @@ void spapr_core_plug(HotplugHandler *hotplug_dev= , DeviceState *dev, > > > /* > > > * Set the right DRC states for cold plugged CPU. > > > */ > > > - drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_US= ABLE); > > > - drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNIS= OLATED); > > > + if (drc) { > > > + drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STAT= E_USABLE); > > > + drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_= UNISOLATED); > > > + } > > > } > > > } > > > =20 > > > @@ -227,7 +233,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_= dev, DeviceState *dev, > > > char *base_core_type =3D spapr_get_cpu_core_type(machine->cpu_mo= del); > > > const char *type =3D object_get_typename(OBJECT(dev)); > > > =20 > > > - if (!mc->query_hotpluggable_cpus) { > > > + if (dev->hotplugged && !mc->query_hotpluggable_cpus) { > > > error_setg(&local_err, "CPU hotplug not supported for this m= achine"); > > > goto out; > > > } > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > > index bd5bcf7..f8d444d 100644 > > > --- a/include/hw/ppc/spapr.h > > > +++ b/include/hw/ppc/spapr.h > > > @@ -614,7 +614,6 @@ void spapr_hotplug_req_add_by_count_indexed(sPAPR= DRConnectorType drc_type, > > > uint32_t count, uint32_t= index); > > > void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType = drc_type, > > > uint32_t count, uint3= 2_t index); > > > -void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error= **errp); > > > void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset, > > > sPAPRMachineState *spapr); > > > =20 > > > =20 > >=20 > >=20 >=20 --=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 --QQ0dNM4HnH4+xgqD Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYIWP0AAoJEGw4ysog2bOSPbQQAOUz2upNuX0/4XTibB8TiYJT D/G5bbPIsfPXYbn6H+DY+tjX0uJj2UQg9jIRQM/SgNI0exeCd+wI4jFm+t39VzoM XJaaU2e27xiTc1ABoino4fI/02TtFpmc3IA9vSlaydY1cAdG7zeP8oFXU01y+YWH SX/sCoC/aQIjxysM4XRzDE05EAa2I6jAX/rvB9DvV1RrLGp3hyWoB1SIjhXzqeHe 7SQYpWxucgjW54NqlsY839GfZHclAKsXTOOj28WFqK18ZE+ZHLMEw6v8QZH6tswu ggaYZ6ZEv2hFuiCvbGAxpYFtE/eFgFEBaAcqkjOjidzZbmOPFLCZtiU+7UnDtiqS xI+OvpEPzVu/RElx1TE8rUMZ/ZBtLGdiC90k0fCXbdTctgMSa+cDbweFBk02kSNv YXxdW0NNmwFu0sTviCPacruQf9MSy3vSCESKCyndgQIcToxup9dhzPRMmuq/7YYq Xk5Bv+FxEzGZijtskpDPWXVwIhGVQr2rAId02Ld02LGWPGU1dJhj6WUkFesaJeLK xzXrJqZx1mu8Ak2RFGcV65hJV5zgJ/YUp8JWvvE0K7bdxU4k2lAIhe9rXyzzIRO+ 5mpSFuzG097kyFsGqzhJb4qkeLnFufyq/y/SPpyOIHGAUenuW8G/+OLGIQJKrKMd YlSoGCMmbQj3NK9B2H5d =+eJV -----END PGP SIGNATURE----- --QQ0dNM4HnH4+xgqD--