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

[Ying Fang]

On 9/17/2020 3:53 PM, 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.

Yes, good catch. Should check against smp.threads before filling
the MPIDR value.

> > +        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'.

Yes, my careless, I will fix it.

> > +            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
>
> .