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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>> ---
>>>>> 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
>
