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

[Eric Auger <eauger@redhat.com>]

Hi Reiji,

On 12/8/21 6:57 AM, Reiji Watanabe wrote:
> Hi Eric,
> 
> On Tue, Dec 7, 2021 at 1:34 AM Eric Auger <eauger@redhat.com> wrote:
>>
>> Hi Reiji,
>>
>> On 12/4/21 2:45 AM, Reiji Watanabe wrote:
>>> Hi Eric,
>>>
>>> On Thu, Dec 2, 2021 at 2:58 AM 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;
>>>> The KVM API allows to call VCPU_INIT several times (with same
>>>> target/feature). With above check on the second call the ID_REGS won't
>>>> be reset. Somehow this is aligned with target/feature behavior. However
>>>> if this is what we want, I think we would need to document it in the KVM
>>>> API doc.
>>>
>>> Thank you for the comment.
>>>
>>> That is what we want.  Since ID registers are read only registers,
>>> their values must not change across the reset.
>>>
>>> '4.82 KVM_ARM_VCPU_INIT' in api.rst says:
>>>
>>>   System registers: Reset to their architecturally defined
>>>   values as for a warm reset to EL1 (resp. SVC)
>>>
>>> Since this reset behavior for the ID registers follows what is
>>> described above, I'm not sure if we need to document the reset
>>> behavior of the ID registers specifically.
>>> If KVM changes the values across the resets, I would think it
>>> rather needs to be documented though.
>>
>> Makes sense to freeze the ID REGs on the 1st reset. Was just wondering
>> if we shouldn't add that the ID REG values are immutable after the 1st
>> VCPU_INIT.
> 
>> Makes sense to freeze the ID REGs on the 1st reset. Was just wondering
>> if we shouldn't add that the ID REG values are immutable after the 1st
>> VCPU_INIT.
> 
> Even after the 1st VCPU_INIT, ID REG values can be changed by
> KVM_SET_ONE_REG (KVM_SET_ONE_REG/KVM_GET_ONE_REG are allowed
> only after the 1st VCPU_INIT).
> 
> The ID REG values are immutable after the 1st KVM_RUN,
> and I think we should document that.  Is that what you meant ?
Yes that's what I meant sorry.

Eric
> 
> Thanks,
> Reiji
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm