On Mon, 22 May 2017 10:59:50 +0200 Greg Kurz <gr...@kaod.org> wrote: > On Mon, 22 May 2017 12:04:13 +1000 > David Gibson <da...@gibson.dropbear.id.au> wrote: > > > On Fri, May 19, 2017 at 12:32:20PM +0200, Greg Kurz wrote: > > > For historical reasons, we compute CPU device-tree ids with a non-trivial > > > logic. This patch consolidate the logic in a single helper to be used > > > in various places where it is currently open-coded. > > > > > > It is okay to get rid of DIV_ROUND_UP() because we're sure that the number > > > of threads per core in the guest cannot exceed the number of threads per > > > core in the host. > > > > However, your new logic still gives different answers in some cases. > > In particular when max_cpus is not a multiple of smp_threads. Which > > is generally a bad idea, but allowed for older machine types for > > compatibility. e.g. smp_threads=4, max_cpus=6 smt=8 > >
FWIW, this topology was never supported for pseries >= 2.7 since commit
94a94e4c4919 ("spapr: convert boot CPUs into CPU core devices", QEMU 2.7):
qemu-system-ppc64: max_cpus (6) must be multiple of threads (4)
The same happens goes with smp_cpus.
If we care for compat with pre-2.7 machine types (ie, no support for CPU
hotplug), this topology isn't valid anymore since QEMU 2.9, with these
commits:
0c86d0fd92aa ("pseries: Always use core objects for CPU construction") which
causes the following error if we only set max_cpus:
qemu-system-ppc64: This machine version does not support CPU hotplug
8149e2992f78 ("pseries: Enforce homogeneous threads-per-core") which
causes the following error if we set smp_cpus (or smp_cpus == max_cpus):
qemu-system-ppc64: invalid nr-threads 2, must be 4
So in the end, we already enforce max_cpus and smp_cpus to be multiple
of smp_threads for all machine types. In this case...
> > Old logic:
> > DIV_ROUND_UP(6 * 8, 4)
> > = ⌈48 / 4⌉ = 12
> >
> > New logic gives: ⌊6 / 4⌋ * 8 + (6 % 4)
> > = 1 * 8 + 2
> > = 10
> >
>
> I now realize this two formulas are hardly reconcilable... this
> probably means that this patch shouldn't try to consolidate the
> logic in hw/ppc/spapr.c with the one in target/ppc/translate_init.c.
>
... both formulas are equivalent, unless I'm missing something. Or
do we really want to re-allow the funky topologies for older machines ?
> > In any case the DIV_ROUND_UP() isn't to handle the case where guest
> > threads-per-core is bigger than host threads-per-core, it's (IIRC) for
> > the case where guest threads-per-core is not a factor of host
> > threads-per-core. Again, a bad idea, but I think allowed in some old
> > cases.
> >
>
> FWIW, DIV_ROUND_UP() comes from this commit:
>
> f303f117fec3 spapr: ensure we have at least one XICS server
>
> but I agree that this was a bad idea...
>
> > >
> > > Signed-off-by: Greg Kurz <gr...@kaod.org>
> > > ---
> > > hw/ppc/spapr.c | 6 ++----
> > > target/ppc/cpu.h | 17 +++++++++++++++++
> > > target/ppc/translate_init.c | 3 +--
> > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 75e298b4c6be..1bb05a9a6b07 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -981,7 +981,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> > > void *fdt;
> > > sPAPRPHBState *phb;
> > > char *buf;
> > > - int smt = kvmppc_smt_threads();
> > >
> > > fdt = g_malloc0(FDT_MAX_SIZE);
> > > _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> > > @@ -1021,7 +1020,7 @@ static void *spapr_build_fdt(sPAPRMachineState
> > > *spapr,
> > > _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> > >
> > > /* /interrupt controller */
> > > - spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt,
> > > PHANDLE_XICP);
> > > + spapr_dt_xics(ppc_cpu_dt_id_from_index(max_cpus), fdt, PHANDLE_XICP);
> > >
> > > ret = spapr_populate_memory(spapr, fdt);
> > > if (ret < 0) {
> > > @@ -1977,7 +1976,6 @@ static void spapr_init_cpus(sPAPRMachineState
> > > *spapr)
> > > MachineState *machine = MACHINE(spapr);
> > > MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > > - int smt = kvmppc_smt_threads();
> > > const CPUArchIdList *possible_cpus;
> > > int boot_cores_nr = smp_cpus / smp_threads;
> > > int i;
> > > @@ -2014,7 +2012,7 @@ static void spapr_init_cpus(sPAPRMachineState
> > > *spapr)
> > > sPAPRDRConnector *drc =
> > > spapr_dr_connector_new(OBJECT(spapr),
> > > SPAPR_DR_CONNECTOR_TYPE_CPU,
> > > - (core_id / smp_threads) * smt);
> > > +
> > > ppc_cpu_dt_id_from_index(core_id));
> > >
> > > qemu_register_reset(spapr_drc_reset, drc);
> > > }
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index 401e10e7dad8..47fe6c64698f 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -2529,4 +2529,21 @@ int ppc_get_vcpu_dt_id(PowerPCCPU *cpu);
> > > PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id);
> > >
> > > void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int
> > > len);
> > > +
> > > +#if !defined(CONFIG_USER_ONLY)
> > > +#include "sysemu/cpus.h"
> > > +#include "target/ppc/kvm_ppc.h"
> > > +
> > > +static inline int ppc_cpu_dt_id_from_index(int cpu_index)
> > > +{
> > > + /* POWER HV support has an historical limitation that different
> > > threads
> > > + * on a single core cannot be in different guests at the same time.
> > > In
> > > + * order to allow KVM to assign guest threads to host cores
> > > accordingly,
> > > + * CPU device tree ids are spaced by the number of threads per host
> > > cores.
> > > + */
> > > + return (cpu_index / smp_threads) * kvmppc_smt_threads()
> > > + + (cpu_index % smp_threads);
> > > +}
> > > +#endif
> > > +
> > > #endif /* PPC_CPU_H */
> > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > > index 56a0ab22cfbe..837a9a496a65 100644
> > > --- a/target/ppc/translate_init.c
> > > +++ b/target/ppc/translate_init.c
> > > @@ -9851,8 +9851,7 @@ static void ppc_cpu_realizefn(DeviceState *dev,
> > > Error **errp)
> > > }
> > >
> > > #if !defined(CONFIG_USER_ONLY)
> > > - cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
> > > - + (cs->cpu_index % smp_threads);
> > > + cpu->cpu_dt_id = ppc_cpu_dt_id_from_index(cs->cpu_index);
> > >
> > > if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->cpu_dt_id)) {
> > > error_setg(errp, "Can't create CPU with id %d in KVM",
> > > cpu->cpu_dt_id);
> > >
> >
>
pgpoCOIwY3MVa.pgp
Description: OpenPGP digital signature
