Hi Eric,
On Tue, Dec 7, 2021 at 11:09 PM Eric Auger <eauger@xxxxxxxxxx> wrote:
>
> 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.
Thank you for the clarification ! I will document that.
Thanks,
Reiji
