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 <gr...@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);
>
--
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
signature.asc
Description: PGP signature
