Re: [RFC PATCH 02/12] target/arm/kvm64: make MPIDR consistent with CPU Topology

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:23AM +0800, Ying Fang wrote:
> MPIDR helps to provide an additional PE identification in a multiprocessor
> system. This patch adds support for setting MPIDR from userspace, so that
> MPIDR is consistent with CPU topology configured.
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  target/arm/kvm64.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index ef1e960285..fcce261a10 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -757,10 +757,46 @@ static int kvm_arm_sve_set_vls(CPUState *cs)
>  
>  #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
>  
> +static int kvm_arm_set_mp_affinity(CPUState *cs)
> +{
> +    uint64_t mpidr;
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    if (kvm_check_extension(kvm_state, KVM_CAP_ARM_MP_AFFINITY)) {
> +        /* Make MPIDR consistent with CPU topology */
> +        MachineState *ms = MACHINE(qdev_get_machine());
> +
> +        mpidr = (kvm_arch_vcpu_id(cs) % ms->smp.threads) << ARM_AFF0_SHIFT;

We should query KVM first to determine if it wants guests to see their PEs
as threads or not. If not, and ms->smp.threads is > 1, then that's an
error. And, in any case, if ms->smp.threads == 1, then we shouldn't waste
aff0 on it, as that could reduce IPI broadcast performance.

> +        mpidr |= ((kvm_arch_vcpu_id(cs) / ms->smp.threads % ms->smp.cores)
> +                                    & 0xff) << ARM_AFF1_SHIFT;
> +        mpidr |= (kvm_arch_vcpu_id(cs) / (ms->smp.cores * ms->smp.threads)
> +                                    & 0xff) << ARM_AFF2_SHIFT;
> +
> +        /* Override mp affinity when KVM is in use */
> +        cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
> +
> +        /* Bit 31 is RES1 indicates the ARMv7 Multiprocessing Extensions */
> +        mpidr |= (1ULL << 31);
> +        return kvm_vcpu_ioctl(cs, KVM_ARM_SET_MP_AFFINITY, &mpidr);
> +    } else {
> +        /*
> +         * When KVM_CAP_ARM_MP_AFFINITY is not supported, it means KVM has 
> its
> +         * own idea about MPIDR assignment, so we override our defaults with
> +         * what we get from KVM.
> +         */
> +        int ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), 
> &mpidr);
> +        if (ret) {
> +            error_report("failed to set MPIDR");

We don't need this error, kvm_get_one_reg() has trace support already.
Anyway, the wording is wrong since it says 'set' instead of 'get'.

> +            return ret;
> +        }
> +        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
> +        return ret;
> +    }
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      int ret;
> -    uint64_t mpidr;
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>  
> @@ -814,16 +850,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          }
>      }
>  
> -    /*
> -     * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
> -     * Currently KVM has its own idea about MPIDR assignment, so we
> -     * override our defaults with what we get from KVM.
> -     */
> -    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
> +    ret = kvm_arm_set_mp_affinity(cs);
>      if (ret) {
>          return ret;
>      }
> -    cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>  
>      kvm_arm_init_debug(cs);
>  
> -- 
> 2.23.0
> 
>

Thanks,
drew