Re: [RFC PATCH v3 02/29] KVM: arm64: Save ID registers' sanitized value per vCPU

[Eric Auger <eauger@redhat.com>]

Hi Reiji,

On 11/18/21 11:00 PM, Reiji Watanabe wrote:
> Hi Eric,
> 
> On Thu, Nov 18, 2021 at 12:37 PM Eric Auger <eauger@redhat.com> wrote:
>>
>> Hi Reiji,
>>
>> On 11/17/21 7:43 AM, Reiji Watanabe wrote:
>>> Extend sys_regs[] of kvm_cpu_context for ID registers and save ID
>>> registers' sanitized value in the array for the vCPU at the first
>>> vCPU reset. Use the saved ones when ID registers are read by
>>> userspace (via KVM_GET_ONE_REG) or the guest.
>>>
>>> Signed-off-by: Reiji Watanabe <reijiw@google.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h | 10 +++++++
>>>  arch/arm64/kvm/sys_regs.c         | 43 +++++++++++++++++++------------
>>>  2 files changed, 37 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index edbe2cb21947..72db73c79403 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info {
>>>       u64 disr_el1;           /* Deferred [SError] Status Register */
>>>  };
>>>
>>> +/*
>>> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
>>> + * where 0<=crm<8, 0<=op2<8.
>>> + */
>>> +#define KVM_ARM_ID_REG_MAX_NUM 64
>>> +#define IDREG_IDX(id)                ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id))
>>> +#define IDREG_SYS_IDX(id)    (ID_REG_BASE + IDREG_IDX(id))
>>> +
>>>  enum vcpu_sysreg {
>>>       __INVALID_SYSREG__,   /* 0 is reserved as an invalid value */
>>>       MPIDR_EL1,      /* MultiProcessor Affinity Register */
>>> @@ -210,6 +218,8 @@ enum vcpu_sysreg {
>>>       CNTP_CVAL_EL0,
>>>       CNTP_CTL_EL0,
>>>
>>> +     ID_REG_BASE,
>>> +     ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1,
>>>       /* Memory Tagging Extension registers */
>>>       RGSR_EL1,       /* Random Allocation Tag Seed Register */
>>>       GCR_EL1,        /* Tag Control Register */
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index e3ec1a44f94d..5608d3410660 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -33,6 +33,8 @@
>>>
>>>  #include "trace.h"
>>>
>>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id);
>>> +
>>>  /*
>>>   * All of this file is extremely similar to the ARM coproc.c, but the
>>>   * types are different. My gut feeling is that it should be pretty
>>> @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
>>>                         struct sys_reg_params *p,
>>>                         const struct sys_reg_desc *r)
>>>  {
>>> -     u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
>>> +     u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1);
>>>       u32 sr = reg_to_encoding(r);
>>>
>>>       if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) {
>>> @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>>>       return true;
>>>  }
>>>
>>> -/* Read a sanitised cpufeature ID register by sys_reg_desc */
>>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>>> -             struct sys_reg_desc const *r, bool raz)
>>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
>>>  {
>>> -     u32 id = reg_to_encoding(r);
>>> -     u64 val;
>>> -
>>> -     if (raz)
>>> -             return 0;
>>> -
>>> -     val = read_sanitised_ftr_reg(id);
>>> +     u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id));
>>>
>>>       switch (id) {
>>>       case SYS_ID_AA64PFR0_EL1:
>>> @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>>>       return val;
>>>  }
>>>
>>> +static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>>> +                    struct sys_reg_desc const *r, bool raz)
>>> +{
>>> +     u32 id = reg_to_encoding(r);
>>> +
>>> +     return raz ? 0 : __read_id_reg(vcpu, id);
>>> +}
>>> +
>>>  static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
>>>                                 const struct sys_reg_desc *r)
>>>  {
>>> @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>>>       return REG_HIDDEN;
>>>  }
>>>
>>> +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
>>> +{
>>> +     u32 id = reg_to_encoding(rd);
>>> +
>>> +     if (vcpu_has_reset_once(vcpu))
>>> +             return;
>>> +
>>> +     __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = read_sanitised_ftr_reg(id);
>>> +}
>>> +
>>>  static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>>>                              const struct sys_reg_desc *rd,
>>>                              const struct kvm_one_reg *reg, void __user *uaddr)
>>> @@ -1223,9 +1235,7 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>>>  /*
>>>   * cpufeature ID register user accessors
>>>   *
>>> - * For now, these registers are immutable for userspace, so no values
>>> - * are stored, and for set_id_reg() we don't allow the effective value
>>> - * to be changed.
>>> + * We don't allow the effective value to be changed.
>> This change may be moved to a subsequent patch as this patch does not
>> change the behavior yet.
> 
> Thank you for the review.
> 
> There are three main parts in the original comments.
> 
>  (A) these registers are immutable for userspace
>  (B) no values are stored
>  (C) we don't allow the effective value to be changed
> 
> This patch stores ID register values in sys_regs[].
> So, I don't think (B) should be there, and I removed (B).
> Since (A) is essentially the same as (C), I removed (A)
> (and left (C)).
> 
> Do you think it is better to leave (A) in this patch, too ?
yes I think I would leave 'for now,  these registers are immutable for
userspace'

Eric
> 
> Thanks,
> Reiji
> 
> 
>>>   */
>>>  static int __get_id_reg(const struct kvm_vcpu *vcpu,
>>>                       const struct sys_reg_desc *rd, void __user *uaddr,
>>> @@ -1382,6 +1392,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>>>  #define ID_SANITISED(name) {                 \
>>>       SYS_DESC(SYS_##name),                   \
>>>       .access = access_id_reg,                \
>>> +     .reset  = reset_id_reg,                 \
>>>       .get_user = get_id_reg,                 \
>>>       .set_user = set_id_reg,                 \
>>>       .visibility = id_visibility,            \
>>> @@ -1837,8 +1848,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu,
>>>       if (p->is_write) {
>>>               return ignore_write(vcpu, p);
>>>       } else {
>>> -             u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
>>> -             u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
>>> +             u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1);
>>> +             u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1);
>>>               u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT);
>>>
>>>               p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
>>>
>> Thanks
>>
>> Eric
>>
>