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 ?
Thanks,
Reiji
