Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/1] ppc: spapr: Move VCPU ID calculation into sPAPR

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 4417 bytes --]

On Thu, 31 Aug 2017 16:38:46 +1000
Sam Bobroff <sam.bobroff@au1.ibm.com> wrote:

> Move the calculation of a CPU's VCPU ID out of the generic PPC code
> (ppc_cpu_realizefn()) and into sPAPR specific code
> (spapr_cpu_core_realize()) where it belongs.
> 
> Unfortunately, due to the way things are ordered, we still need to
> default the VCPU ID in ppc_cpu_realizfn() but at least doing that
> doesn't require any interaction with sPAPR.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
> This is follow up work arising from my work to clean up the way CPU VCPU IDs are
> handled on PowerPC. It had looked like it would be difficult to move the actual
> VCPU ID calculation out of generic code but it turned out to be OK.
> 
> It's based on dgibson/ppc-for-2.11.
> 
> Cheers,
> Sam.
> 
>  hw/ppc/spapr_cpu_core.c     | 11 +++++++++++
>  target/ppc/translate_init.c | 18 +++---------------
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 5e319d9bbb..84dcc6e264 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -211,6 +211,7 @@ error:
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  {
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(OBJECT(dev));
> @@ -237,6 +238,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          cs = CPU(obj);
>          cpu = POWERPC_CPU(cs);
>          cs->cpu_index = cc->core_id + i;
> +        cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i;

According to what we currently have in the generic code, this should be:

((cc->core_id + i) / smp_threads) * spapr->vsmt + ((cc->core_id + i) % smp_threads)

but since cc->core_id is a multiple of smp_threads and i < cc->nr_threads <= smp_threads,
then we can indeed simplify the computation as you did. :)

\o/

Reviewed-by: Greg Kurz <groug@kaod.org>

> +        if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->vcpu_id)) {
> +            error_setg(&local_err, "Can't create CPU with id %d in KVM",
> +                       cpu->vcpu_id);
> +            error_append_hint(&local_err, "Adjust the number of cpus to %d "
> +                              "or try to raise the number of threads per core\n",
> +                              cpu->vcpu_id * smp_threads / spapr->vsmt);
> +            goto err;
> +        }
> +
>  
>          /* Set NUMA node for the threads belonged to core  */
>          cpu->node_id = sc->node_id;
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 7f6a349e43..1f7286c893 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -9903,28 +9903,15 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>      PowerPCCPU *cpu = POWERPC_CPU(dev);
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
> -#if !defined(CONFIG_USER_ONLY)
> -    int max_smt = kvmppc_smt_threads();
> -#endif
>  
>      cpu_exec_realizefn(cs, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -#if !defined(CONFIG_USER_ONLY)
> -    cpu->vcpu_id = (cs->cpu_index / smp_threads) * max_smt
> -        + (cs->cpu_index % smp_threads);
> -
> -    if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->vcpu_id)) {
> -        error_setg(errp, "Can't create CPU with id %d in KVM", cpu->vcpu_id);
> -        error_append_hint(errp, "Adjust the number of cpus to %d "
> -                          "or try to raise the number of threads per core\n",
> -                          cpu->vcpu_id * smp_threads / max_smt);
> -        goto unrealize;
> +    if (cpu->vcpu_id == UNASSIGNED_CPU_INDEX) {
> +        cpu->vcpu_id = cs->cpu_index;
>      }
> -#endif
>  
>      if (tcg_enabled()) {
>          if (ppc_fixup_cpu(cpu) != 0) {
> @@ -10625,6 +10612,7 @@ static void ppc_cpu_initfn(Object *obj)
>      CPUPPCState *env = &cpu->env;
>  
>      cs->env_ptr = env;
> +    cpu->vcpu_id = UNASSIGNED_CPU_INDEX;
>  
>      env->msr_mask = pcc->msr_mask;
>      env->mmu_model = pcc->mmu_model;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]