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

[Andrew Jones]

On Thu, Sep 17, 2020 at 09:53:35AM +0200, Andrew Jones wrote:
> 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;

Also, as pointed out in the KVM thread, we should not be attempting to
describe topology with the MPIDR at all. Alexandru pointed out [*] as
evidence for that.

However, we do need to consider the limits on Aff0 imposed by the GIC.
See hw/arm/virt.c:virt_cpu_mp_affinity() for how we currently do it
for TCG. We should do something similar for KVM guests when we're taking
full control of the MPIDR.

[*] 
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=3102bc0e6ac7

Thanks,
drew

> > +
> > +        /* 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