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