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

[Andrew Jones]

On Thu, Sep 17, 2020 at 09:19:34PM +0800, Ying Fang wrote:
> 
> 
> On 9/17/2020 6:59 PM, Andrew Jones wrote:
> > 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
> 
> Thanks for your information on MPIDR. As described in [*], MPIDR cannot
> be trusted as the actual topology. After applying:
> arm64: topology: Stop using MPIDR for topology information
> 
> Can we just use topology information from ACPI or fdt as topology and ignore
> MPIDR ?

Well the MPIDR will still be used as a unique identifier and it must
still confirm to the architecture (e.g. 0-15 for Aff0 for gicv3), but,
yes, the point is that the guest kernel should never try to use it for
topology information. The guest kernel should only use DT or ACPI.

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