Message ID | 151863725388.3003.620735626834741475.stgit@bahia.lan |
---|---|
State | New |
Headers | show |
Series | spapr: fix VCPU ids miscalculation | expand |
On Wed, Feb 14, 2018 at 08:40:53PM +0100, Greg Kurz wrote: > Several places in the code need to calculate a VCPU id: > > (cpu_index / smp_threads) * spapr->vsmt + cpu_index % smp_threads > (core_id / smp_threads) * spapr->vsmt (1 user) > index * spapr->vsmt (2 users) > > or guess that the VCPU id of a given VCPU is the first thread of a virtual > core: > > index % spapr->vsmt != 0 > > Even if the numbering logic isn't that complex, it is rather fragile to > have these assumptions open-coded in several places. FWIW this was > proved with recent issues related to VSMT. > > This patch moves the VCPU id formula to a single function to be called > everywhere the code needs to compute one. It also adds an helper to > guess if a VCPU is the first thread of a VCORE. > > Signed-off-by: Greg Kurz <groug@kaod.org> Good change. I don't like the name 'spapr_is_vcore' though - cores are a logically different thing from thread0 of the core. So I've renamed it to spapr_is_thread0_of_vcore() as I've applied it. > --- > hw/ppc/spapr.c | 29 ++++++++++++++++++++++------- > 1 file changed, 22 insertions(+), 7 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 18ebc058acdd..800d3f001253 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -99,6 +99,20 @@ > > #define PHANDLE_XICP 0x00001111 > > +/* These two functions implement the VCPU id numbering: one to compute them > + * all and one to identify thread 0 of a VCORE. Any change to the first one > + * is likely to have an impact on the second one, so let's keep them close. > + */ > +static int spapr_vcpu_id(sPAPRMachineState *spapr, int cpu_index) > +{ > + return > + (cpu_index / smp_threads) * spapr->vsmt + cpu_index % smp_threads; > +} > +static bool spapr_is_vcore(sPAPRMachineState *spapr, PowerPCCPU *cpu) > +{ > + return spapr_get_vcpu_id(cpu) % spapr->vsmt == 0; > +} > + > static ICSState *spapr_ics_create(sPAPRMachineState *spapr, > const char *type_ics, > int nr_irqs, Error **errp) > @@ -345,7 +359,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr) > int index = spapr_get_vcpu_id(cpu); > int compat_smt = MIN(smp_threads, ppc_compat_max_vthreads(cpu)); > > - if (index % spapr->vsmt != 0) { > + if (!spapr_is_vcore(spapr, cpu)) { > continue; > } > > @@ -630,7 +644,7 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr) > DeviceClass *dc = DEVICE_GET_CLASS(cs); > int offset; > > - if (index % spapr->vsmt != 0) { > + if (!spapr_is_vcore(spapr, cpu)) { > continue; > } > > @@ -2251,7 +2265,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr) > > if (mc->has_hotpluggable_cpus) { > spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU, > - (core_id / smp_threads) * spapr->vsmt); > + spapr_vcpu_id(spapr, core_id)); > } > > if (i < boot_cores_nr) { > @@ -3293,7 +3307,8 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, > return; > } > > - drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * spapr->vsmt); > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, > + spapr_vcpu_id(spapr, cc->core_id)); > g_assert(drc); > > spapr_drc_detach(drc); > @@ -3322,7 +3337,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > cc->core_id); > return; > } > - drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * spapr->vsmt); > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, > + spapr_vcpu_id(spapr, cc->core_id)); > > g_assert(drc || !mc->has_hotpluggable_cpus); > > @@ -3807,8 +3823,7 @@ void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp) > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > int vcpu_id; > > - vcpu_id = > - (cpu_index / smp_threads) * spapr->vsmt + cpu_index % smp_threads; > + vcpu_id = spapr_vcpu_id(spapr, cpu_index); > > if (kvm_enabled() && !kvm_vcpu_id_is_valid(vcpu_id)) { > error_setg(errp, "Can't create CPU with id %d in KVM", vcpu_id); >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 18ebc058acdd..800d3f001253 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -99,6 +99,20 @@ #define PHANDLE_XICP 0x00001111 +/* These two functions implement the VCPU id numbering: one to compute them + * all and one to identify thread 0 of a VCORE. Any change to the first one + * is likely to have an impact on the second one, so let's keep them close. + */ +static int spapr_vcpu_id(sPAPRMachineState *spapr, int cpu_index) +{ + return + (cpu_index / smp_threads) * spapr->vsmt + cpu_index % smp_threads; +} +static bool spapr_is_vcore(sPAPRMachineState *spapr, PowerPCCPU *cpu) +{ + return spapr_get_vcpu_id(cpu) % spapr->vsmt == 0; +} + static ICSState *spapr_ics_create(sPAPRMachineState *spapr, const char *type_ics, int nr_irqs, Error **errp) @@ -345,7 +359,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr) int index = spapr_get_vcpu_id(cpu); int compat_smt = MIN(smp_threads, ppc_compat_max_vthreads(cpu)); - if (index % spapr->vsmt != 0) { + if (!spapr_is_vcore(spapr, cpu)) { continue; } @@ -630,7 +644,7 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr) DeviceClass *dc = DEVICE_GET_CLASS(cs); int offset; - if (index % spapr->vsmt != 0) { + if (!spapr_is_vcore(spapr, cpu)) { continue; } @@ -2251,7 +2265,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr) if (mc->has_hotpluggable_cpus) { spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU, - (core_id / smp_threads) * spapr->vsmt); + spapr_vcpu_id(spapr, core_id)); } if (i < boot_cores_nr) { @@ -3293,7 +3307,8 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, return; } - drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * spapr->vsmt); + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, + spapr_vcpu_id(spapr, cc->core_id)); g_assert(drc); spapr_drc_detach(drc); @@ -3322,7 +3337,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, cc->core_id); return; } - drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * spapr->vsmt); + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, + spapr_vcpu_id(spapr, cc->core_id)); g_assert(drc || !mc->has_hotpluggable_cpus); @@ -3807,8 +3823,7 @@ void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp) sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); int vcpu_id; - vcpu_id = - (cpu_index / smp_threads) * spapr->vsmt + cpu_index % smp_threads; + vcpu_id = spapr_vcpu_id(spapr, cpu_index); if (kvm_enabled() && !kvm_vcpu_id_is_valid(vcpu_id)) { error_setg(errp, "Can't create CPU with id %d in KVM", vcpu_id);
Several places in the code need to calculate a VCPU id: (cpu_index / smp_threads) * spapr->vsmt + cpu_index % smp_threads (core_id / smp_threads) * spapr->vsmt (1 user) index * spapr->vsmt (2 users) or guess that the VCPU id of a given VCPU is the first thread of a virtual core: index % spapr->vsmt != 0 Even if the numbering logic isn't that complex, it is rather fragile to have these assumptions open-coded in several places. FWIW this was proved with recent issues related to VSMT. This patch moves the VCPU id formula to a single function to be called everywhere the code needs to compute one. It also adds an helper to guess if a VCPU is the first thread of a VCORE. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)