Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

Ying Fang
2020-09-17T11:52:20Z

On 9/17/2020 3:47 PM, Marc Zyngier wrote:
> On 2020-09-17 03:30, Ying Fang wrote:
>> Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
>> so that we can support cpu topology for arm.
>
> MPIDR has *nothing* to do with CPU topology in the ARM architecture.
> I encourage you to have a look at the ARM ARM and find out how often
> the word "topology" is used in conjunction with the MPIDR_EL1 register.
>
>>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  5 +++++
>>  arch/arm64/kvm/arm.c              |  8 ++++++++
>>  arch/arm64/kvm/reset.c            | 11 +++++++++++
>>  arch/arm64/kvm/sys_regs.c         | 30 +++++++++++++++++++-----------
>>  include/uapi/linux/kvm.h          |  2 ++
>>  5 files changed, 45 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h
>> b/arch/arm64/include/asm/kvm_host.h
>> index e52c927aade5..7adc351ee70a 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -372,6 +372,9 @@ struct kvm_vcpu_arch {
>>          u64 last_steal;
>>          gpa_t base;
>>      } steal;
>> +
>> +    /* vCPU MP Affinity */
>> +    u64 mp_affinity;
>
> No. We already have a per-CPU MPIDR_EL1 register, we don't need another
> piece of state.
I am also not sure if it is needed here.
>
>>  };
>>
>>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>> @@ -685,6 +688,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned
>> long type);
>>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
>>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
>>
>> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr);
>> +
>>  #define kvm_arm_vcpu_sve_finalized(vcpu) \
>>      ((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 913c8da539b3..5f1fa625dc11 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -1178,6 +1178,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>
>>          return kvm_arm_vcpu_finalize(vcpu, what);
>>      }
>> +    case KVM_ARM_SET_MP_AFFINITY: {
>> +        u64 mpidr;
>> +
>> +        if (get_user(mpidr, (const unsigned int __user *)argp))
>> +            return -EFAULT;
>> +
>> +        return kvm_arm_vcpu_set_mp_affinity(vcpu, mpidr);
>> +    }
>
> That's not the way we access system registers from userspace.

I know that we already had the KVM_SET_ONE_REG api and well designed
sys_regs subsystem.

However when I tried to set MPIDR using the KVM_SET_ONE_REG api in
kvm_arch_init_vcpu, the value may get erased by cpu_reset called within
arm_cpu_realizefn. Maybe we should make change on qemu instead of adding
this redundant ioctl.

>
>>      default:
>>          r = -EINVAL;
>>      }
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index ee33875c5c2a..4918c967b0c9 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -188,6 +188,17 @@ int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu,
>> int feature)
>>      return -EINVAL;
>>  }
>>
>> +int kvm_arm_vcpu_set_mp_affinity(struct kvm_vcpu *vcpu, uint64_t mpidr)
>> +{
>> +    if (!(mpidr & (1ULL << 31))) {
>> +        pr_warn("invalid mp_affinity format\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    vcpu->arch.mp_affinity = mpidr;
>
> This doesn't match the definition of the MPIDR_EL1 register. It also
> doesn't take into account any of the existing restrictions for the
> supported affinity levels and number of PEs at the lowest affinity
> level.
Yes, we should do sanity check here and we must keep the format of MPIDR
following the Spec definition.

>
>> +    return 0;
>> +}
>> +
>>  bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
>>  {
>>      if (vcpu_has_sve(vcpu) && !kvm_arm_vcpu_sve_finalized(vcpu))
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 077293b5115f..e76f483475ad 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -646,17 +646,25 @@ static void reset_mpidr(struct kvm_vcpu *vcpu,
>> const struct sys_reg_desc *r)
>>  {
>>      u64 mpidr;
>>
>> -    /*
>> -     * Map the vcpu_id into the first three affinity level fields of
>> -     * the MPIDR. We limit the number of VCPUs in level 0 due to a
>> -     * limitation to 16 CPUs in that level in the ICC_SGIxR registers
>> -     * of the GICv3 to be able to address each CPU directly when
>> -     * sending IPIs.
>> -     */
>> -    mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
>> -    mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
>> -    mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
>> -    vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
>> +    if (vcpu->arch.mp_affinity) {
>> +        /* If mp_affinity is set by userspace, it means an customized
>> cpu
>> +         * topology is defined. Let it be MPIDR of the vcpu
>> +         */
>> +        mpidr = vcpu->arch.mp_affinity;
>> +    } else {
>> +        /*
>> +         * Map the vcpu_id into the first three affinity level fields of
>> +         * the MPIDR. We limit the number of VCPUs in level 0 due to a
>> +         * limitation to 16 CPUs in that level in the ICC_SGIxR
>> registers
>> +         * of the GICv3 to be able to address each CPU directly when
>> +         * sending IPIs.
>> +         */
>> +        mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
>> +        mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
>> +        mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
>> +        mpidr |= (1ULL << 31);
>> +    }
>> +    vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
>>  }
>>
>>  static void reset_pmcr(struct kvm_vcpu *vcpu, const struct
>> sys_reg_desc *r)
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index c4874905cd9c..ae45876a689d 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1475,6 +1475,8 @@ struct kvm_s390_ucas_mapping {
>>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct
>> kvm_s390_cmma_log)
>>  /* Memory Encryption Commands */
>>  #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
>> +/* Available with KVM_CAP_ARM_MP_AFFINITY */
>> +#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
>>
>>  struct kvm_enc_region {
>>      __u64 addr;
>
> As it is, this patch is unacceptable. It ignores the requirements of the
> architecture, as well as those of imposed by KVM as a platform.
> It also pointlessly creates additional state and invents unnecessary
> userspace interfaces. In short, it requires some *major* rework.

Yes, thanks for your comments.

We should set MPIDR from userspace in another elegant way.
I'll drop this and figure it out.
>
>         M.