| Message ID | 20240620065807.151540-11-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Enable fine grained undefined for MDSELR_EL1 | expand |
On Thu, 20 Jun 2024 07:58:07 +0100, Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id > for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers. > > Cc: Marc Zyngier <maz@kernel.org> > Cc: Oliver Upton <oliver.upton@linux.dev> > Cc: James Morse <james.morse@arm.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: kvmarm@lists.linux.dev > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > arch/arm64/include/asm/kvm_arm.h | 8 +++++ > arch/arm64/include/asm/kvm_host.h | 2 ++ > arch/arm64/kvm/emulate-nested.c | 14 ++++++++ > arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++++++ > arch/arm64/kvm/nested.c | 36 ++++++++++++++++++++ > arch/arm64/kvm/sys_regs.c | 45 +++++++++++++++++++++++++ > 6 files changed, 115 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > index b2adc2c6c82a..b3fb368bcadb 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -354,6 +354,14 @@ > #define __HFGRTR_EL2_MASK GENMASK(49, 0) > #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK) > > +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 > +#define __HDFGRTR2_EL2_MASK (GENMASK(22, 22) | GENMASK(20, 0)) How about bit 23? How comes you are considering all these bits as positive? > +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) Note the *nMASK* here. 'n' is for *negative*. Just look at how __HDFGRTR_EL2_MASK and __HDFGRTR_EL2_nMASK are written. > + > +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 > +#define __HDFGWTR2_EL2_MASK (GENMASK(22, 19) | GENMASK(16, 7) | GENMASK(5, 0)) Again, how about bit 23? Same remark for the polarity. > +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) > + > /* > * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any > * future additions, define __HFGWTR* macros relative to __HFGRTR* ones. > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 7b44e96e7270..d6fbd6ebc32d 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -239,6 +239,8 @@ enum fgt_group_id { > HDFGWTR_GROUP = HDFGRTR_GROUP, > HFGITR_GROUP, > HAFGRTR_GROUP, > + HDFGRTR2_GROUP, > + HDFGWTR2_GROUP = HDFGRTR2_GROUP, > > /* Must be last */ > __NR_FGT_GROUP_IDS__ > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c > index 54090967a335..bc5ea1e60a0a 100644 > --- a/arch/arm64/kvm/emulate-nested.c > +++ b/arch/arm64/kvm/emulate-nested.c > @@ -1724,6 +1724,9 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = { > SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1), > SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1), > SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1), > + > + /* HDFGRTR2_EL2 */ > + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 1), No. See the 'n' prefix on this bit? Also, where are all the other bits for the HDFRxTR2 registers? > }; > > static union trap_config get_trap_config(u32 sysreg) > @@ -1979,6 +1982,10 @@ static bool check_fgt_bit(struct kvm *kvm, bool is_read, > sr = is_read ? HDFGRTR_EL2 : HDFGWTR_EL2; > break; > > + case HDFGRTR2_GROUP: > + sr = is_read ? HDFGRTR2_EL2 : HDFGWTR2_EL2; > + break; > + > case HAFGRTR_GROUP: > sr = HAFGRTR_EL2; > break; > @@ -2053,6 +2060,13 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) > val = __vcpu_sys_reg(vcpu, HDFGWTR_EL2); > break; > > + case HDFGRTR2_GROUP: > + if (is_read) > + val = __vcpu_sys_reg(vcpu, HDFGRTR2_EL2); > + else > + val = __vcpu_sys_reg(vcpu, HDFGWTR2_EL2); > + break; > + > case HAFGRTR_GROUP: > val = __vcpu_sys_reg(vcpu, HAFGRTR_EL2); > break; > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index 0c4de44534b7..b5944aa6d9c8 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -89,6 +89,10 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > case HDFGWTR_EL2: \ > id = HDFGRTR_GROUP; \ > break; \ > + case HDFGRTR2_EL2: \ > + case HDFGWTR2_EL2: \ > + id = HDFGRTR2_GROUP; \ > + break; \ > case HAFGRTR_EL2: \ > id = HAFGRTR_GROUP; \ > break; \ > @@ -160,6 +164,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) > CHECK_FGT_MASKS(HDFGWTR_EL2); > CHECK_FGT_MASKS(HAFGRTR_EL2); > CHECK_FGT_MASKS(HCRX_EL2); > + CHECK_FGT_MASKS(HDFGRTR2_EL2); > + CHECK_FGT_MASKS(HDFGWTR2_EL2); > > if (!cpus_have_final_cap(ARM64_HAS_FGT)) > return; > @@ -171,6 +177,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) > update_fgt_traps(hctxt, vcpu, kvm, HFGITR_EL2); > update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR_EL2); > update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR_EL2); > + update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR2_EL2); > + update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR2_EL2); > > if (cpu_has_amu()) > update_fgt_traps(hctxt, vcpu, kvm, HAFGRTR_EL2); > @@ -200,6 +208,8 @@ static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu) > __deactivate_fgt(hctxt, vcpu, kvm, HFGITR_EL2); > __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR_EL2); > __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR_EL2); > + __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR2_EL2); > + __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR2_EL2); > > if (cpu_has_amu()) > __deactivate_fgt(hctxt, vcpu, kvm, HAFGRTR_EL2); > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c > index bae8536cbf00..ebe4e3972fed 100644 > --- a/arch/arm64/kvm/nested.c > +++ b/arch/arm64/kvm/nested.c > @@ -384,6 +384,42 @@ int kvm_init_nv_sysregs(struct kvm *kvm) > res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1; > set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1); > > + /* HDFG[RW]TR2_EL2 */ > + res0 = res1 = 0; > + > + /* FEAT_TRBE_MPAM is not exposed to the guest */ > + res0 |= HDFGRTR2_EL2_nTRBMPAM_EL1; No. We are moving away from hard-coded features, so you have to explicitly check it. > + > + /* FEAT_SPE_FDS is not exposed to the guest */ > + res0 |= HDFGRTR2_EL2_nPMSDSFR_EL1; Same thing. > + > + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) > + res0 |= HDFGRTR2_EL2_nMDSTEPOP_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) > + res0 |= HDFGRTR2_EL2_nTRCITECR_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) > + res0 |= (HDFGRTR2_EL2_nSPMDEVAFF_EL1 | HDFGRTR2_EL2_nSPMID | > + HDFGRTR2_EL2_nSPMSCR_EL1 | HDFGRTR2_EL2_nSPMACCESSR_EL1 | > + HDFGRTR2_EL2_nSPMCR_EL0 | HDFGRTR2_EL2_nSPMOVS | > + HDFGRTR2_EL2_nSPMINTEN | HDFGRTR2_EL2_nSPMSELR_EL0 | > + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | HDFGRTR2_EL2_nSPMEVCNTRn_EL0 | > + HDFGRTR2_EL2_nPMSSCR_EL1 | HDFGRTR2_EL2_nPMSSDATA); > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) > + res0 |= HDFGRTR2_EL2_nMDSELR_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) > + res0 |= HDFGRTR2_EL2_nPMUACR_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) > + res0 |= HDFGRTR2_EL2_nPMICFILTR_EL0; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) > + res0 |= HDFGRTR2_EL2_nPMICNTR_EL0; > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) > + res0 |= HDFGRTR2_EL2_nPMIAR_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && > + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) > + res0 |= HDFGRTR2_EL2_nPMECR_EL1; And all of that suffers from the same issue as in https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=next&id=eb9d53d4a949c6d6d7c9f130e537f6b5687fedf9 > + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1); > + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1); How about the HDFGxTR2_EL2_RES1 bits? > + > /* Reuse the bits from the read-side and add the write-specific stuff */ > if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP)) > res0 |= (HDFGWTR_EL2_PMCR_EL0 | HDFGWTR_EL2_PMSWINC_EL0); > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index f921af014d0c..8029f408855d 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) > kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | > HAFGRTR_EL2_RES1); > > + /* FEAT_TRBE_MPAM is not exposed to the guest */ > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1); Same thing about dynamic configuration. But more importantly, you are disabling anything *but* MPAM. Does it seem right to you? > + > + /* FEAT_SPE_FDS is not exposed to the guest */ > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMSDSFR_EL1); Same thing. > + > + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSTEPOP_EL1); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRCITECR_EL1); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nSPMDEVAFF_EL1 | > + HDFGRTR2_EL2_nSPMID | > + HDFGRTR2_EL2_nSPMSCR_EL1 | > + HDFGRTR2_EL2_nSPMACCESSR_EL1 | > + HDFGRTR2_EL2_nSPMCR_EL0 | > + HDFGRTR2_EL2_nSPMOVS | > + HDFGRTR2_EL2_nSPMINTEN | > + HDFGRTR2_EL2_nSPMSELR_EL0 | > + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | > + HDFGRTR2_EL2_nSPMEVCNTRn_EL0 | > + HDFGRTR2_EL2_nPMSSCR_EL1 | > + HDFGRTR2_EL2_nPMSSDATA); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSELR_EL1); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMUACR_EL1); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMICFILTR_EL0); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMICNTR_EL0); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMIAR_EL1); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && > + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMECR_EL1); > + Overall, this looks completely broken. M.
On 6/20/24 16:36, Marc Zyngier wrote: > On Thu, 20 Jun 2024 07:58:07 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> wrote: >> >> This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id >> for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers. >> >> Cc: Marc Zyngier <maz@kernel.org> >> Cc: Oliver Upton <oliver.upton@linux.dev> >> Cc: James Morse <james.morse@arm.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: kvmarm@lists.linux.dev >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> arch/arm64/include/asm/kvm_arm.h | 8 +++++ >> arch/arm64/include/asm/kvm_host.h | 2 ++ >> arch/arm64/kvm/emulate-nested.c | 14 ++++++++ >> arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++++++ >> arch/arm64/kvm/nested.c | 36 ++++++++++++++++++++ >> arch/arm64/kvm/sys_regs.c | 45 +++++++++++++++++++++++++ >> 6 files changed, 115 insertions(+) >> >> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h >> index b2adc2c6c82a..b3fb368bcadb 100644 >> --- a/arch/arm64/include/asm/kvm_arm.h >> +++ b/arch/arm64/include/asm/kvm_arm.h >> @@ -354,6 +354,14 @@ >> #define __HFGRTR_EL2_MASK GENMASK(49, 0) >> #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK) >> >> +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 >> +#define __HDFGRTR2_EL2_MASK (GENMASK(22, 22) | GENMASK(20, 0)) > > How about bit 23? How comes you are considering all these bits as positive? It had 23 earlier looking into DDI0601 2024-03 but then referred into ARM ARM DDI 0487K.A which changed it as 22 - which was wrong, will change this again if required. > >> +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) > > Note the *nMASK* here. 'n' is for *negative*. Just look at how > __HDFGRTR_EL2_MASK and __HDFGRTR_EL2_nMASK are written. Still trying to understand what does these three masks represent for a given FGT register XXXX - __XXXX_RES0 - __XXXX_MASK - __XXXX_nMASK But from the mentioned example for HDFGRTR_EL2. #define __HDFGRTR_EL2_RES0 HDFGRTR_EL2_RES0 #define __HDFGRTR_EL2_MASK (BIT(63) | GENMASK(58, 50) | GENMASK(48, 43) | \ GENMASK(41, 40) | GENMASK(37, 22) | \ GENMASK(19, 9) | GENMASK(7, 0)) #define __HDFGRTR_EL2_nMASK ~(__HDFGRTR_EL2_RES0 | __HDFGRTR_EL2_MASK) Looking at HDFGRTR_EL2 definition inside arch/arm64/tools/sysreg HDFGRTR_EL2_RES0 = BIT(49) | GENMASK(39, 38) | GENMASK(21, 20) | BIT(8) is representing the entire mask in the register which are RES0. But then what does __HDFGRTR_EL2_MASK signify ? Positive trap bits mask ? The following bits belong in __HDFGRTR_EL2_MASK HDFGRTR_EL2.PMBIDR_EL1 (63) HDFGRTR_EL2.PMCEIDn_EL0 (58) Where as the following bits belong in __HDFGRTR_EL2_nMASK HDFGRTR_EL2.nPMSNEVFR_EL1 (61) HDFGRTR_EL2.nBRBCTL (60) Reworking proposed HDFGRTR2_EL2 and HDFGWTR2_EL2. #define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 #define __HDFGRTR2_EL2_MASK 0 #define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) #define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 #define __HDFGWTR2_EL2_MASK 0 #define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) Please note that all trap bits in both these registers are negative ones hence __HDFGRTR2_EL2_MASK/__HDFGWTR2_EL2_MASK should be 0. > >> + >> +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 >> +#define __HDFGWTR2_EL2_MASK (GENMASK(22, 19) | GENMASK(16, 7) | GENMASK(5, 0)) > > Again, how about bit 23? Same remark for the polarity. > >> +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) >> + >> /* >> * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any >> * future additions, define __HFGWTR* macros relative to __HFGRTR* ones. >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 7b44e96e7270..d6fbd6ebc32d 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -239,6 +239,8 @@ enum fgt_group_id { >> HDFGWTR_GROUP = HDFGRTR_GROUP, >> HFGITR_GROUP, >> HAFGRTR_GROUP, >> + HDFGRTR2_GROUP, >> + HDFGWTR2_GROUP = HDFGRTR2_GROUP, >> >> /* Must be last */ >> __NR_FGT_GROUP_IDS__ >> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c >> index 54090967a335..bc5ea1e60a0a 100644 >> --- a/arch/arm64/kvm/emulate-nested.c >> +++ b/arch/arm64/kvm/emulate-nested.c >> @@ -1724,6 +1724,9 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = { >> SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1), >> SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1), >> SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1), >> + >> + /* HDFGRTR2_EL2 */ >> + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 1), > > No. See the 'n' prefix on this bit? Right, that should be a 0 instead. > > Also, where are all the other bits for the HDFRxTR2 registers? This will require a number of new registers being added into tools sysreg expanding the series further, but will go ahead add all that is required. Although I had asked about this in the cover letter. - Probably an entry is needed for SYS_MDSELR_EL1 in encoding_to_fgt[] table inside the file arch/arm64/kvm/emulate-nested.c, but while trying to test features for all individual bits in HDFGRTR2_EL2, it seemed a lot of new register definitions from various features need to be added as well, thus expanding the scope further. Should all required new system registers be added for completeness ? > >> }; >> >> static union trap_config get_trap_config(u32 sysreg) >> @@ -1979,6 +1982,10 @@ static bool check_fgt_bit(struct kvm *kvm, bool is_read, >> sr = is_read ? HDFGRTR_EL2 : HDFGWTR_EL2; >> break; >> >> + case HDFGRTR2_GROUP: >> + sr = is_read ? HDFGRTR2_EL2 : HDFGWTR2_EL2; >> + break; >> + >> case HAFGRTR_GROUP: >> sr = HAFGRTR_EL2; >> break; >> @@ -2053,6 +2060,13 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) >> val = __vcpu_sys_reg(vcpu, HDFGWTR_EL2); >> break; >> >> + case HDFGRTR2_GROUP: >> + if (is_read) >> + val = __vcpu_sys_reg(vcpu, HDFGRTR2_EL2); >> + else >> + val = __vcpu_sys_reg(vcpu, HDFGWTR2_EL2); >> + break; >> + >> case HAFGRTR_GROUP: >> val = __vcpu_sys_reg(vcpu, HAFGRTR_EL2); >> break; >> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h >> index 0c4de44534b7..b5944aa6d9c8 100644 >> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h >> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h >> @@ -89,6 +89,10 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) >> case HDFGWTR_EL2: \ >> id = HDFGRTR_GROUP; \ >> break; \ >> + case HDFGRTR2_EL2: \ >> + case HDFGWTR2_EL2: \ >> + id = HDFGRTR2_GROUP; \ >> + break; \ >> case HAFGRTR_EL2: \ >> id = HAFGRTR_GROUP; \ >> break; \ >> @@ -160,6 +164,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) >> CHECK_FGT_MASKS(HDFGWTR_EL2); >> CHECK_FGT_MASKS(HAFGRTR_EL2); >> CHECK_FGT_MASKS(HCRX_EL2); >> + CHECK_FGT_MASKS(HDFGRTR2_EL2); >> + CHECK_FGT_MASKS(HDFGWTR2_EL2); >> >> if (!cpus_have_final_cap(ARM64_HAS_FGT)) >> return; >> @@ -171,6 +177,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) >> update_fgt_traps(hctxt, vcpu, kvm, HFGITR_EL2); >> update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR_EL2); >> update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR_EL2); >> + update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR2_EL2); >> + update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR2_EL2); >> >> if (cpu_has_amu()) >> update_fgt_traps(hctxt, vcpu, kvm, HAFGRTR_EL2); >> @@ -200,6 +208,8 @@ static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu) >> __deactivate_fgt(hctxt, vcpu, kvm, HFGITR_EL2); >> __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR_EL2); >> __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR_EL2); >> + __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR2_EL2); >> + __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR2_EL2); >> >> if (cpu_has_amu()) >> __deactivate_fgt(hctxt, vcpu, kvm, HAFGRTR_EL2); >> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c >> index bae8536cbf00..ebe4e3972fed 100644 >> --- a/arch/arm64/kvm/nested.c >> +++ b/arch/arm64/kvm/nested.c >> @@ -384,6 +384,42 @@ int kvm_init_nv_sysregs(struct kvm *kvm) >> res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1; >> set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1); >> >> + /* HDFG[RW]TR2_EL2 */ >> + res0 = res1 = 0; >> + >> + /* FEAT_TRBE_MPAM is not exposed to the guest */ >> + res0 |= HDFGRTR2_EL2_nTRBMPAM_EL1; > > No. We are moving away from hard-coded features, so you have to > explicitly check it. The above code was just temporary for this RFC. But you are right, these features need to be checked properly but there is a challenge. As I have mentioned in the cover letter - TRBIDR_EL1.MPAM needs to be probed for setting HDFGRTR2_EL2_nTRBMPAM_EL1 but kvm_has_feat() does not operate a non-ID register which causes build warnings. The same problem exists for probing PMSIDR_EL1.FDS which is needed for setting HDFGRTR2_EL2_nPMSDSFR_EL1 as well. Currently both the bits mentioned earlier are set, assuming the features are not present in nested virtualization. Do we need some new helpers to probe these non-ID registers as well ? How do you suggest we proceed on this - testing features in TRBIDR_EL1 and PMSIDR_EL1 ? > >> + >> + /* FEAT_SPE_FDS is not exposed to the guest */ >> + res0 |= HDFGRTR2_EL2_nPMSDSFR_EL1; > > Same thing. Got it. > >> + >> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) >> + res0 |= HDFGRTR2_EL2_nMDSTEPOP_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) >> + res0 |= HDFGRTR2_EL2_nTRCITECR_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) >> + res0 |= (HDFGRTR2_EL2_nSPMDEVAFF_EL1 | HDFGRTR2_EL2_nSPMID | >> + HDFGRTR2_EL2_nSPMSCR_EL1 | HDFGRTR2_EL2_nSPMACCESSR_EL1 | >> + HDFGRTR2_EL2_nSPMCR_EL0 | HDFGRTR2_EL2_nSPMOVS | >> + HDFGRTR2_EL2_nSPMINTEN | HDFGRTR2_EL2_nSPMSELR_EL0 | >> + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | HDFGRTR2_EL2_nSPMEVCNTRn_EL0 | >> + HDFGRTR2_EL2_nPMSSCR_EL1 | HDFGRTR2_EL2_nPMSSDATA); >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) >> + res0 |= HDFGRTR2_EL2_nMDSELR_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) >> + res0 |= HDFGRTR2_EL2_nPMUACR_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) >> + res0 |= HDFGRTR2_EL2_nPMICFILTR_EL0; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) >> + res0 |= HDFGRTR2_EL2_nPMICNTR_EL0; >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) >> + res0 |= HDFGRTR2_EL2_nPMIAR_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && >> + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) >> + res0 |= HDFGRTR2_EL2_nPMECR_EL1; > > And all of that suffers from the same issue as in > https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=next&id=eb9d53d4a949c6d6d7c9f130e537f6b5687fedf9 Alright, will set res1 instead for all these negative trap bits. > > >> + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1); >> + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1); > > How about the HDFGxTR2_EL2_RES1 bits? I assume you are suggesting something like this. - set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1); - set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1); + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1 | HDFGRTR2_EL2_RES1); + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1 | HDFGWTR2_EL2_RES1); But guess both HDFGRTR2_EL2_RES1 and HDFGWTR2_EL2_RES1 will be empty (0) as there are no res1 bit positions in either of these registers. But will change as above. > >> + >> /* Reuse the bits from the read-side and add the write-specific stuff */ >> if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP)) >> res0 |= (HDFGWTR_EL2_PMCR_EL0 | HDFGWTR_EL2_PMSWINC_EL0); >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index f921af014d0c..8029f408855d 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) >> kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | >> HAFGRTR_EL2_RES1); >> >> + /* FEAT_TRBE_MPAM is not exposed to the guest */ >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1); > > Same thing about dynamic configuration. > > But more importantly, you are disabling anything *but* MPAM. Does it > seem right to you? Did not get that, should the inverse ~ be dropped here and also for all other negative trap bits across the register ? > >> + >> + /* FEAT_SPE_FDS is not exposed to the guest */ >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMSDSFR_EL1); > > Same thing. As mentioned earlier there is a challenge in checking for the features via non-ID registers i.e TRBIDR_EL1.MPAM and PMSIDR_EL1.FDS. > >> + >> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSTEPOP_EL1); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRCITECR_EL1); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nSPMDEVAFF_EL1 | >> + HDFGRTR2_EL2_nSPMID | >> + HDFGRTR2_EL2_nSPMSCR_EL1 | >> + HDFGRTR2_EL2_nSPMACCESSR_EL1 | >> + HDFGRTR2_EL2_nSPMCR_EL0 | >> + HDFGRTR2_EL2_nSPMOVS | >> + HDFGRTR2_EL2_nSPMINTEN | >> + HDFGRTR2_EL2_nSPMSELR_EL0 | >> + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | >> + HDFGRTR2_EL2_nSPMEVCNTRn_EL0 | >> + HDFGRTR2_EL2_nPMSSCR_EL1 | >> + HDFGRTR2_EL2_nPMSSDATA); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSELR_EL1); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMUACR_EL1); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMICFILTR_EL0); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMICNTR_EL0); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMIAR_EL1); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && >> + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMECR_EL1); >> + > > Overall, this looks completely broken. I am re-working this patch as suggested. Thanks for the pointers.
On Fri, 21 Jun 2024 08:56:15 +0100, Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > On 6/20/24 16:36, Marc Zyngier wrote: > > On Thu, 20 Jun 2024 07:58:07 +0100, > > Anshuman Khandual <anshuman.khandual@arm.com> wrote: > >> > >> This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id > >> for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers. > >> > >> Cc: Marc Zyngier <maz@kernel.org> > >> Cc: Oliver Upton <oliver.upton@linux.dev> > >> Cc: James Morse <james.morse@arm.com> > >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > >> Cc: linux-arm-kernel@lists.infradead.org > >> Cc: kvmarm@lists.linux.dev > >> Cc: linux-kernel@vger.kernel.org > >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > >> --- > >> arch/arm64/include/asm/kvm_arm.h | 8 +++++ > >> arch/arm64/include/asm/kvm_host.h | 2 ++ > >> arch/arm64/kvm/emulate-nested.c | 14 ++++++++ > >> arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++++++ > >> arch/arm64/kvm/nested.c | 36 ++++++++++++++++++++ > >> arch/arm64/kvm/sys_regs.c | 45 +++++++++++++++++++++++++ > >> 6 files changed, 115 insertions(+) > >> > >> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > >> index b2adc2c6c82a..b3fb368bcadb 100644 > >> --- a/arch/arm64/include/asm/kvm_arm.h > >> +++ b/arch/arm64/include/asm/kvm_arm.h > >> @@ -354,6 +354,14 @@ > >> #define __HFGRTR_EL2_MASK GENMASK(49, 0) > >> #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK) > >> > >> +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 > >> +#define __HDFGRTR2_EL2_MASK (GENMASK(22, 22) | GENMASK(20, 0)) > > > > How about bit 23? How comes you are considering all these bits as positive? > > It had 23 earlier looking into DDI0601 2024-03 but then referred into ARM > ARM DDI 0487K.A which changed it as 22 - which was wrong, will change this > again if required. I guess we're past that point now, and we should delete the comment that limits it to K.a. Anything that is published as part of the XML drop (DDI 0602) is now fair game. > > > > >> +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) > > > > Note the *nMASK* here. 'n' is for *negative*. Just look at how > > __HDFGRTR_EL2_MASK and __HDFGRTR_EL2_nMASK are written. > > Still trying to understand what does these three masks represent > for a given FGT register XXXX > > - __XXXX_RES0 RES0 bits. > - __XXXX_MASK Positive trap bits. > - __XXXX_nMASK Negative trap bits. > > But from the mentioned example for HDFGRTR_EL2. > > #define __HDFGRTR_EL2_RES0 HDFGRTR_EL2_RES0 > #define __HDFGRTR_EL2_MASK (BIT(63) | GENMASK(58, 50) | GENMASK(48, 43) | \ > GENMASK(41, 40) | GENMASK(37, 22) | \ > GENMASK(19, 9) | GENMASK(7, 0)) > #define __HDFGRTR_EL2_nMASK ~(__HDFGRTR_EL2_RES0 | __HDFGRTR_EL2_MASK) > > Looking at HDFGRTR_EL2 definition inside arch/arm64/tools/sysreg > > HDFGRTR_EL2_RES0 = BIT(49) | GENMASK(39, 38) | GENMASK(21, 20) | BIT(8) > > is representing the entire mask in the register which are RES0. But then what > does __HDFGRTR_EL2_MASK signify ? Positive trap bits mask ? > > The following bits belong in __HDFGRTR_EL2_MASK > > HDFGRTR_EL2.PMBIDR_EL1 (63) > HDFGRTR_EL2.PMCEIDn_EL0 (58) > > Where as the following bits belong in __HDFGRTR_EL2_nMASK > > HDFGRTR_EL2.nPMSNEVFR_EL1 (61) > HDFGRTR_EL2.nBRBCTL (60) > > Reworking proposed HDFGRTR2_EL2 and HDFGWTR2_EL2. > > #define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 > #define __HDFGRTR2_EL2_MASK 0 > #define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) > > #define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 > #define __HDFGWTR2_EL2_MASK 0 > #define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) > > Please note that all trap bits in both these registers are negative ones > hence __HDFGRTR2_EL2_MASK/__HDFGWTR2_EL2_MASK should be 0. That's because you're looking at the XML, and not the ARM ARM this was written against. > > > > >> + > >> +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 > >> +#define __HDFGWTR2_EL2_MASK (GENMASK(22, 19) | GENMASK(16, 7) | GENMASK(5, 0)) > > > > Again, how about bit 23? Same remark for the polarity. > > > >> +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) > >> + > >> /* > >> * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any > >> * future additions, define __HFGWTR* macros relative to __HFGRTR* ones. > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >> index 7b44e96e7270..d6fbd6ebc32d 100644 > >> --- a/arch/arm64/include/asm/kvm_host.h > >> +++ b/arch/arm64/include/asm/kvm_host.h > >> @@ -239,6 +239,8 @@ enum fgt_group_id { > >> HDFGWTR_GROUP = HDFGRTR_GROUP, > >> HFGITR_GROUP, > >> HAFGRTR_GROUP, > >> + HDFGRTR2_GROUP, > >> + HDFGWTR2_GROUP = HDFGRTR2_GROUP, > >> > >> /* Must be last */ > >> __NR_FGT_GROUP_IDS__ > >> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c > >> index 54090967a335..bc5ea1e60a0a 100644 > >> --- a/arch/arm64/kvm/emulate-nested.c > >> +++ b/arch/arm64/kvm/emulate-nested.c > >> @@ -1724,6 +1724,9 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = { > >> SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1), > >> SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1), > >> SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1), > >> + > >> + /* HDFGRTR2_EL2 */ > >> + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 1), > > > > No. See the 'n' prefix on this bit? > > Right, that should be a 0 instead. > > > > > Also, where are all the other bits for the HDFRxTR2 registers? > > This will require a number of new registers being added into tools sysreg > expanding the series further, but will go ahead add all that is required. Please do. > Although I had asked about this in the cover letter. > > - Probably an entry is needed for SYS_MDSELR_EL1 in encoding_to_fgt[] table > inside the file arch/arm64/kvm/emulate-nested.c, but while trying to test > features for all individual bits in HDFGRTR2_EL2, it seemed a lot of new > register definitions from various features need to be added as well, thus > expanding the scope further. Should all required new system registers be > added for completeness ? Anything on which you expect KVM to interact with *must* be fully described. I don't want to have to second guess the exception routing tables when we add support for a feature. In short, this is the end of half baked feature support in KVM. When you add support for a trap register, it comes with everything it traps, recursively. [...] > >> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c > >> index bae8536cbf00..ebe4e3972fed 100644 > >> --- a/arch/arm64/kvm/nested.c > >> +++ b/arch/arm64/kvm/nested.c > >> @@ -384,6 +384,42 @@ int kvm_init_nv_sysregs(struct kvm *kvm) > >> res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1; > >> set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1); > >> > >> + /* HDFG[RW]TR2_EL2 */ > >> + res0 = res1 = 0; > >> + > >> + /* FEAT_TRBE_MPAM is not exposed to the guest */ > >> + res0 |= HDFGRTR2_EL2_nTRBMPAM_EL1; > > > > No. We are moving away from hard-coded features, so you have to > > explicitly check it. > > The above code was just temporary for this RFC. But you are right, these > features need to be checked properly but there is a challenge. As I have > mentioned in the cover letter You are wasting everybody's time with these RFCs. Either you *try* do the right thing from the first post, or you don't bother posting the patches. What's the point of posting them if you know this isn't right? Writing these reviews take time and energy, and there is no shortage of this I'd rather do instead of having to write these emails. If you have questions, just ask. You don't need to dump 10 patches on the list for that. > > - TRBIDR_EL1.MPAM needs to be probed for setting HDFGRTR2_EL2_nTRBMPAM_EL1 > but kvm_has_feat() does not operate a non-ID register which causes build > warnings. The same problem exists for probing PMSIDR_EL1.FDS which is > needed for setting HDFGRTR2_EL2_nPMSDSFR_EL1 as well. Currently both the > bits mentioned earlier are set, assuming the features are not present in > nested virtualization. Do we need some new helpers to probe these non-ID > registers as well ? > > How do you suggest we proceed on this - testing features in TRBIDR_EL1 and > PMSIDR_EL1 ? Let's look at the spec. For TRBMPAM_EL1 being implemented (from K.a): * This register is present only when FEAT_TRBE_MPAM is implemented. Otherwise, direct accesses to TRBMPAM_EL1 are UNDEFINED. * If FEAT_TRBE_MPAM is implemented, then FEAT_TRBE_EXT is implemented. * The following fields identify the presence of FEAT_TRBE_EXT: * ID_AA64DFR0_EL1.ExtTrcBuff. This allows you do shortcut it one level above the particular MPAM requirement, which is good enough (I don't expect external traces to be exposed to a VM). Therefore no need to look at TRBIDR_EL1. For PMSDSFR_EL1 being implemented (from K.a): * This register is present only when FEAT_SPE_FDS is implemented. Otherwise, direct accesses to PMSDSFR_EL1 are UNDEFINED. * If FEAT_SPE_FDS is implemented, then FEAT_SPEv1p4 is implemented. * The following field identifies the presence of FEAT_SPEv1p4: * ID_AA64DFR0_EL1.PMSVer. So again that's your cut-off point. Until we support this level of SPE in KVM, we're pretty safe (and I seriously doubt we'll get there in my lifetime, given the current pace of progress). If at some point we need to support ID registers that are not in the feature range, we'll do that (Oliver has some stuff already for CTR_EL0). But don't hardcode things. [...] > >> + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1); > >> + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1); > > > > How about the HDFGxTR2_EL2_RES1 bits? > > I assume you are suggesting something like this. > > - set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1); > - set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1); > + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1 | HDFGRTR2_EL2_RES1); > + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1 | HDFGWTR2_EL2_RES1); > > But guess both HDFGRTR2_EL2_RES1 and HDFGWTR2_EL2_RES1 will be empty (0) as there > are no res1 bit positions in either of these registers. But will change as above. I don't care about these values being 0. I want the reassuring feeling that we're not missing anything, because debugging this is hell if you have a guest that sets/clears random bits. [...] > >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > >> index f921af014d0c..8029f408855d 100644 > >> --- a/arch/arm64/kvm/sys_regs.c > >> +++ b/arch/arm64/kvm/sys_regs.c > >> @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) > >> kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | > >> HAFGRTR_EL2_RES1); > >> > >> + /* FEAT_TRBE_MPAM is not exposed to the guest */ > >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1); > > > > Same thing about dynamic configuration. > > > > But more importantly, you are disabling anything *but* MPAM. Does it > > seem right to you? > > Did not get that, should the inverse ~ be dropped here and also for all > other negative trap bits across the register ? Look at the way FGU works. A bit set to 1 means that if we have trapped because of this bit (as per the FGT table), we inject an UNDEF. > > > > >> + > >> + /* FEAT_SPE_FDS is not exposed to the guest */ > >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMSDSFR_EL1); > > > > Same thing. > > As mentioned earlier there is a challenge in checking for the features > via non-ID registers i.e TRBIDR_EL1.MPAM and PMSIDR_EL1.FDS. As I wrote, there is no problem. You always get enough ID-reg information to do something sensible. And if we need to eventually add the infrastructure for that, so be it. M.
On 6/21/24 16:54, Marc Zyngier wrote: > On Fri, 21 Jun 2024 08:56:15 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> wrote: >> >> On 6/20/24 16:36, Marc Zyngier wrote: >>> On Thu, 20 Jun 2024 07:58:07 +0100, >>> Anshuman Khandual <anshuman.khandual@arm.com> wrote: >>>> >>>> This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id >>>> for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers. >>>> >>>> Cc: Marc Zyngier <maz@kernel.org> >>>> Cc: Oliver Upton <oliver.upton@linux.dev> >>>> Cc: James Morse <james.morse@arm.com> >>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>>> Cc: linux-arm-kernel@lists.infradead.org >>>> Cc: kvmarm@lists.linux.dev >>>> Cc: linux-kernel@vger.kernel.org >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>>> --- >>>> arch/arm64/include/asm/kvm_arm.h | 8 +++++ >>>> arch/arm64/include/asm/kvm_host.h | 2 ++ >>>> arch/arm64/kvm/emulate-nested.c | 14 ++++++++ >>>> arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++++++ >>>> arch/arm64/kvm/nested.c | 36 ++++++++++++++++++++ >>>> arch/arm64/kvm/sys_regs.c | 45 +++++++++++++++++++++++++ >>>> 6 files changed, 115 insertions(+) >>>> >>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h >>>> index b2adc2c6c82a..b3fb368bcadb 100644 >>>> --- a/arch/arm64/include/asm/kvm_arm.h >>>> +++ b/arch/arm64/include/asm/kvm_arm.h >>>> @@ -354,6 +354,14 @@ >>>> #define __HFGRTR_EL2_MASK GENMASK(49, 0) >>>> #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK) >>>> >>>> +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 >>>> +#define __HDFGRTR2_EL2_MASK (GENMASK(22, 22) | GENMASK(20, 0)) >>> >>> How about bit 23? How comes you are considering all these bits as positive? >> >> It had 23 earlier looking into DDI0601 2024-03 but then referred into ARM >> ARM DDI 0487K.A which changed it as 22 - which was wrong, will change this >> again if required. > > I guess we're past that point now, and we should delete the comment > that limits it to K.a. Anything that is published as part of the XML > drop (DDI 0602) is now fair game. Sure. > >> >>> >>>> +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) >>> >>> Note the *nMASK* here. 'n' is for *negative*. Just look at how >>> __HDFGRTR_EL2_MASK and __HDFGRTR_EL2_nMASK are written. >> >> Still trying to understand what does these three masks represent >> for a given FGT register XXXX >> >> - __XXXX_RES0 > > RES0 bits. > >> - __XXXX_MASK > > Positive trap bits. > >> - __XXXX_nMASK > > Negative trap bits. Right, figured that eventually but these were not very clear at first. > >> >> But from the mentioned example for HDFGRTR_EL2. >> >> #define __HDFGRTR_EL2_RES0 HDFGRTR_EL2_RES0 >> #define __HDFGRTR_EL2_MASK (BIT(63) | GENMASK(58, 50) | GENMASK(48, 43) | \ >> GENMASK(41, 40) | GENMASK(37, 22) | \ >> GENMASK(19, 9) | GENMASK(7, 0)) >> #define __HDFGRTR_EL2_nMASK ~(__HDFGRTR_EL2_RES0 | __HDFGRTR_EL2_MASK) >> >> Looking at HDFGRTR_EL2 definition inside arch/arm64/tools/sysreg >> >> HDFGRTR_EL2_RES0 = BIT(49) | GENMASK(39, 38) | GENMASK(21, 20) | BIT(8) >> >> is representing the entire mask in the register which are RES0. But then what >> does __HDFGRTR_EL2_MASK signify ? Positive trap bits mask ? >> >> The following bits belong in __HDFGRTR_EL2_MASK >> >> HDFGRTR_EL2.PMBIDR_EL1 (63) >> HDFGRTR_EL2.PMCEIDn_EL0 (58) >> >> Where as the following bits belong in __HDFGRTR_EL2_nMASK >> >> HDFGRTR_EL2.nPMSNEVFR_EL1 (61) >> HDFGRTR_EL2.nBRBCTL (60) >> >> Reworking proposed HDFGRTR2_EL2 and HDFGWTR2_EL2. >> >> #define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 >> #define __HDFGRTR2_EL2_MASK 0 >> #define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) >> >> #define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 >> #define __HDFGWTR2_EL2_MASK 0 >> #define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) >> >> Please note that all trap bits in both these registers are negative ones >> hence __HDFGRTR2_EL2_MASK/__HDFGWTR2_EL2_MASK should be 0. > > That's because you're looking at the XML, and not the ARM ARM this was > written against. Did not follow that. Both in ddi0601/2024-03 XML and ARM ARM DDI 0487K.a there are no positive trap bits, either in HDFGRTR2_EL2 or HDFGWTR2_EL2. OR did I miss something here. > >> >>> >>>> + >>>> +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 >>>> +#define __HDFGWTR2_EL2_MASK (GENMASK(22, 19) | GENMASK(16, 7) | GENMASK(5, 0)) >>> >>> Again, how about bit 23? Same remark for the polarity. >>> >>>> +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) >>>> + >>>> /* >>>> * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any >>>> * future additions, define __HFGWTR* macros relative to __HFGRTR* ones. >>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>> index 7b44e96e7270..d6fbd6ebc32d 100644 >>>> --- a/arch/arm64/include/asm/kvm_host.h >>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>> @@ -239,6 +239,8 @@ enum fgt_group_id { >>>> HDFGWTR_GROUP = HDFGRTR_GROUP, >>>> HFGITR_GROUP, >>>> HAFGRTR_GROUP, >>>> + HDFGRTR2_GROUP, >>>> + HDFGWTR2_GROUP = HDFGRTR2_GROUP, >>>> >>>> /* Must be last */ >>>> __NR_FGT_GROUP_IDS__ >>>> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c >>>> index 54090967a335..bc5ea1e60a0a 100644 >>>> --- a/arch/arm64/kvm/emulate-nested.c >>>> +++ b/arch/arm64/kvm/emulate-nested.c >>>> @@ -1724,6 +1724,9 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = { >>>> SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1), >>>> SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1), >>>> SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1), >>>> + >>>> + /* HDFGRTR2_EL2 */ >>>> + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 1), >>> >>> No. See the 'n' prefix on this bit? >> >> Right, that should be a 0 instead. >> >>> >>> Also, where are all the other bits for the HDFRxTR2 registers? >> >> This will require a number of new registers being added into tools sysreg >> expanding the series further, but will go ahead add all that is required. > > Please do. Sure, will add the following new registers in arch/arm64/tools/sysreg format except for the ones that require a formula based enumeration. Those will be added into arch/arm64/include/asm/sysreg.h directly e.g +#define SYS_SPMEVCNTR_EL0(m) sys_reg(2, 3, 14, (0 | (m >> 3)), (m & 7)) +#define SYS_SPMEVTYPER_EL0(m) sys_reg(2, 3, 14, (2 | (m >> 3)), (m & 7)) +#define SYS_SPMEVFILTR_EL0(m) sys_reg(2, 3, 14, (4 | (m >> 3)), (m & 7)) +#define SYS_SPMEVFILT2R_EL0(m) sys_reg(2, 3, 14, (6 | (m >> 3)), (m & 7)) 9d93fc432f1a arm64/sysreg: Add remaining debug registers affected by HDFGxTR2_EL2 56b8830f0a5e arm64/sysreg: Add register fields for SPMCGCR1_EL1 ad674ae52178 arm64/sysreg: Add register fields for SPMCGCR0_EL1 8538a282d208 arm64/sysreg: Add register fields for PMZR_EL0 c88f0b8d898e arm64/sysreg: Add register fields for PMSSCR_EL1 8788ad49b7b6 arm64/sysreg: Add register fields for SPMCFGR_EL1 142de2bc3d7b arm64/sysreg: Add register fields for SPMDEVARCH_EL1 3d903ba35e8c arm64/sysreg: Add register fields for SPMIIDR_EL1 1a4db0b8b100 arm64/sysreg: Add register fields for PMICNTSVR_EL1 6bee8f139ba5 arm64/sysreg: Add register fields for SPMSELR_EL0 b208ab4cd54d arm64/sysreg: Add register fields for SPMCNTENSET_EL0 644abf522c8a arm64/sysreg: Add register fields for SPMCNTENCLR_EL0 fad9b7751359 arm64/sysreg: Add register fields for SPMINTENSET_EL1 c28299b8df76 arm64/sysreg: Add register fields for SPMINTENCLR_EL1 b9c283b27980 arm64/sysreg: Add register fields for SPMOVSSET_EL0 03dd01a26c46 arm64/sysreg: Add register fields for SPMOVSCLR_EL0 c8a2f1b688de arm64/sysreg: Add register fields for SPMCR_EL0 422ca4026aa7 arm64/sysreg: Add register fields for SPMACCESSR_EL1 d790b2570461 arm64/sysreg: Add register fields for SPMSCR_EL1 c2cdd0fecdcb arm64/sysreg: Add register fields for PMCCNTSVR_EL1 3b793f3f07b8 arm64/sysreg: Add register fields for PMUACR_EL1 a1804742ee8c arm64/sysreg: Add register fields for PMICFILTR_EL0 c88476b9c52c arm64/sysreg: Add register fields for PMICNTR_EL0 6d1520f3477b arm64/sysreg: Add register fields for PMECR_EL1 257ea3ec7950 arm64/sysreg: Add register fields for PMIAR_EL1 a29f787102f0 arm64/sysreg: Add register fields for SPMDEVAFF_EL1 3b7c4d4cf0eb arm64/sysreg: Add register fields for PMSDSFR_EL1 2a14e5dc1903 arm64/sysreg: Add register fields for TRBMPAM_EL1 > >> Although I had asked about this in the cover letter. >> >> - Probably an entry is needed for SYS_MDSELR_EL1 in encoding_to_fgt[] table >> inside the file arch/arm64/kvm/emulate-nested.c, but while trying to test >> features for all individual bits in HDFGRTR2_EL2, it seemed a lot of new >> register definitions from various features need to be added as well, thus >> expanding the scope further. Should all required new system registers be >> added for completeness ? > > Anything on which you expect KVM to interact with *must* be fully > described. I don't want to have to second guess the exception routing > tables when we add support for a feature.> > In short, this is the end of half baked feature support in KVM. When > you add support for a trap register, it comes with everything it > traps, recursively. Understood. > > [...] > >>>> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c >>>> index bae8536cbf00..ebe4e3972fed 100644 >>>> --- a/arch/arm64/kvm/nested.c >>>> +++ b/arch/arm64/kvm/nested.c >>>> @@ -384,6 +384,42 @@ int kvm_init_nv_sysregs(struct kvm *kvm) >>>> res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1; >>>> set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1); >>>> >>>> + /* HDFG[RW]TR2_EL2 */ >>>> + res0 = res1 = 0; >>>> + >>>> + /* FEAT_TRBE_MPAM is not exposed to the guest */ >>>> + res0 |= HDFGRTR2_EL2_nTRBMPAM_EL1; >>> >>> No. We are moving away from hard-coded features, so you have to >>> explicitly check it. >> >> The above code was just temporary for this RFC. But you are right, these >> features need to be checked properly but there is a challenge. As I have >> mentioned in the cover letter > > You are wasting everybody's time with these RFCs. Either you *try* do > the right thing from the first post, or you don't bother posting the > patches. What's the point of posting them if you know this isn't > right? > > Writing these reviews take time and energy, and there is no shortage > of this I'd rather do instead of having to write these emails. If you > have questions, just ask. You don't need to dump 10 patches on the > list for that. Alright, as you prefer, in future will keep the questions in separate emails instead, rather than with some RFC patches. > >> >> - TRBIDR_EL1.MPAM needs to be probed for setting HDFGRTR2_EL2_nTRBMPAM_EL1 >> but kvm_has_feat() does not operate a non-ID register which causes build >> warnings. The same problem exists for probing PMSIDR_EL1.FDS which is >> needed for setting HDFGRTR2_EL2_nPMSDSFR_EL1 as well. Currently both the >> bits mentioned earlier are set, assuming the features are not present in >> nested virtualization. Do we need some new helpers to probe these non-ID >> registers as well ? >> >> How do you suggest we proceed on this - testing features in TRBIDR_EL1 and >> PMSIDR_EL1 ? > > Let's look at the spec. > > For TRBMPAM_EL1 being implemented (from K.a): > > * This register is present only when FEAT_TRBE_MPAM is > implemented. Otherwise, direct accesses to TRBMPAM_EL1 are > UNDEFINED. > > * If FEAT_TRBE_MPAM is implemented, then FEAT_TRBE_EXT is implemented. > > * The following fields identify the presence of FEAT_TRBE_EXT: > * ID_AA64DFR0_EL1.ExtTrcBuff. > > This allows you do shortcut it one level above the particular MPAM > requirement, which is good enough (I don't expect external traces to > be exposed to a VM). Therefore no need to look at TRBIDR_EL1. Sure, will check ID_AA64DFR0_EL1.ExtTrcBuff for FEAT_TRBE_MPAM instead. > > For PMSDSFR_EL1 being implemented (from K.a): > > * This register is present only when FEAT_SPE_FDS is > implemented. Otherwise, direct accesses to PMSDSFR_EL1 are > UNDEFINED. > > * If FEAT_SPE_FDS is implemented, then FEAT_SPEv1p4 is implemented. > > * The following field identifies the presence of FEAT_SPEv1p4: > * ID_AA64DFR0_EL1.PMSVer. > > So again that's your cut-off point. Until we support this level of SPE > in KVM, we're pretty safe (and I seriously doubt we'll get there in my > lifetime, given the current pace of progress). Okay, will check ID_AA64DFR0_EL1.PMSVer for FEAT_SPE_FDS instead. Thanks for pointing out the feature check work arounds. > > If at some point we need to support ID registers that are not in the > feature range, we'll do that (Oliver has some stuff already for > CTR_EL0). But don't hardcode things. > > [...] > >>>> + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1); >>>> + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1); >>> >>> How about the HDFGxTR2_EL2_RES1 bits? >> >> I assume you are suggesting something like this. >> >> - set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1); >> - set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1); >> + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1 | HDFGRTR2_EL2_RES1); >> + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1 | HDFGWTR2_EL2_RES1); >> >> But guess both HDFGRTR2_EL2_RES1 and HDFGWTR2_EL2_RES1 will be empty (0) as there >> are no res1 bit positions in either of these registers. But will change as above. > > I don't care about these values being 0. I want the reassuring feeling > that we're not missing anything, because debugging this is hell if you > have a guest that sets/clears random bits. Understood. > > [...] > >>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>>> index f921af014d0c..8029f408855d 100644 >>>> --- a/arch/arm64/kvm/sys_regs.c >>>> +++ b/arch/arm64/kvm/sys_regs.c >>>> @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) >>>> kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | >>>> HAFGRTR_EL2_RES1); >>>> >>>> + /* FEAT_TRBE_MPAM is not exposed to the guest */ >>>> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1); >>> >>> Same thing about dynamic configuration. >>> >>> But more importantly, you are disabling anything *but* MPAM. Does it >>> seem right to you? >> >> Did not get that, should the inverse ~ be dropped here and also for all >> other negative trap bits across the register ? > > Look at the way FGU works. A bit set to 1 means that if we have > trapped because of this bit (as per the FGT table), we inject an > UNDEF. Seems like positive and negative trap bits do not make any difference here, while setting respective bits in kvm->arch.fgu[HDFGRTR2_GROUP]. In this case the inverse should be dropped for all bits. Should the lone trap bit (nPMZR_EL0) which is present in HDFGWTR2_EL2 but not in HDFGRTR2_EL2, be set in kvm->arch.fgu[HDFGWTR2_GROUP] as well ? if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) { kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMUACR_EL1; kvm->arch.fgu[HDFGWTR2_GROUP] |= HDFGWTR2_EL2_nPMZR_EL0; } > >> >>> >>>> + >>>> + /* FEAT_SPE_FDS is not exposed to the guest */ >>>> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMSDSFR_EL1); >>> >>> Same thing. >> >> As mentioned earlier there is a challenge in checking for the features >> via non-ID registers i.e TRBIDR_EL1.MPAM and PMSIDR_EL1.FDS. > > As I wrote, there is no problem. You always get enough ID-reg > information to do something sensible. And if we need to eventually add > the infrastructure for that, so be it. Okay. Thanks for the review and the related pointers.
On Tue, 25 Jun 2024 10:03:29 +0100, Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > >>>> +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) > >>> > >>> Note the *nMASK* here. 'n' is for *negative*. Just look at how > >>> __HDFGRTR_EL2_MASK and __HDFGRTR_EL2_nMASK are written. > >> > >> Still trying to understand what does these three masks represent > >> for a given FGT register XXXX > >> > >> - __XXXX_RES0 > > > > RES0 bits. > > > >> - __XXXX_MASK > > > > Positive trap bits. > > > >> - __XXXX_nMASK > > > > Negative trap bits. > > Right, figured that eventually but these were not very clear at > first. I keep hearing I'm not clear. But at this stage, I don't know what to do to make it (or myself) clearer. > > > > >> > >> But from the mentioned example for HDFGRTR_EL2. > >> > >> #define __HDFGRTR_EL2_RES0 HDFGRTR_EL2_RES0 > >> #define __HDFGRTR_EL2_MASK (BIT(63) | GENMASK(58, 50) | GENMASK(48, 43) | \ > >> GENMASK(41, 40) | GENMASK(37, 22) | \ > >> GENMASK(19, 9) | GENMASK(7, 0)) > >> #define __HDFGRTR_EL2_nMASK ~(__HDFGRTR_EL2_RES0 | __HDFGRTR_EL2_MASK) > >> > >> Looking at HDFGRTR_EL2 definition inside arch/arm64/tools/sysreg > >> > >> HDFGRTR_EL2_RES0 = BIT(49) | GENMASK(39, 38) | GENMASK(21, 20) | BIT(8) > >> > >> is representing the entire mask in the register which are RES0. But then what > >> does __HDFGRTR_EL2_MASK signify ? Positive trap bits mask ? > >> > >> The following bits belong in __HDFGRTR_EL2_MASK > >> > >> HDFGRTR_EL2.PMBIDR_EL1 (63) > >> HDFGRTR_EL2.PMCEIDn_EL0 (58) > >> > >> Where as the following bits belong in __HDFGRTR_EL2_nMASK > >> > >> HDFGRTR_EL2.nPMSNEVFR_EL1 (61) > >> HDFGRTR_EL2.nBRBCTL (60) > >> > >> Reworking proposed HDFGRTR2_EL2 and HDFGWTR2_EL2. > >> > >> #define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 > >> #define __HDFGRTR2_EL2_MASK 0 > >> #define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) > >> > >> #define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 > >> #define __HDFGWTR2_EL2_MASK 0 > >> #define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) > >> > >> Please note that all trap bits in both these registers are negative ones > >> hence __HDFGRTR2_EL2_MASK/__HDFGWTR2_EL2_MASK should be 0. > > > > That's because you're looking at the XML, and not the ARM ARM this was > > written against. > > Did not follow that. Both in ddi0601/2024-03 XML and ARM ARM DDI 0487K.a > there are no positive trap bits, either in HDFGRTR2_EL2 or HDFGWTR2_EL2. > OR did I miss something here. From this very file: <quote> /* * FGT register definitions * * RES0 and polarity masks as of DDI0487J.a, to be updated as needed. * We're not using the generated masks as they are usually ahead of * the published ARM ARM, which we use as a reference. * * Once we get to a point where the two describe the same thing, we'll * merge the definitions. One day. */ </quote> In case it wasn't written *CLEARLY* enough. > Sure, will add the following new registers in arch/arm64/tools/sysreg format > except for the ones that require a formula based enumeration. Those will be > added into arch/arm64/include/asm/sysreg.h directly e.g > > +#define SYS_SPMEVCNTR_EL0(m) sys_reg(2, 3, 14, (0 | (m >> 3)), (m & 7)) > +#define SYS_SPMEVTYPER_EL0(m) sys_reg(2, 3, 14, (2 | (m >> 3)), (m & 7)) > +#define SYS_SPMEVFILTR_EL0(m) sys_reg(2, 3, 14, (4 | (m >> 3)), (m & 7)) > +#define SYS_SPMEVFILT2R_EL0(m) sys_reg(2, 3, 14, (6 | (m >> 3)), (m & 7)) > > 9d93fc432f1a arm64/sysreg: Add remaining debug registers affected by HDFGxTR2_EL2 > 56b8830f0a5e arm64/sysreg: Add register fields for SPMCGCR1_EL1 > ad674ae52178 arm64/sysreg: Add register fields for SPMCGCR0_EL1 > 8538a282d208 arm64/sysreg: Add register fields for PMZR_EL0 > c88f0b8d898e arm64/sysreg: Add register fields for PMSSCR_EL1 > 8788ad49b7b6 arm64/sysreg: Add register fields for SPMCFGR_EL1 > 142de2bc3d7b arm64/sysreg: Add register fields for SPMDEVARCH_EL1 > 3d903ba35e8c arm64/sysreg: Add register fields for SPMIIDR_EL1 > 1a4db0b8b100 arm64/sysreg: Add register fields for PMICNTSVR_EL1 > 6bee8f139ba5 arm64/sysreg: Add register fields for SPMSELR_EL0 > b208ab4cd54d arm64/sysreg: Add register fields for SPMCNTENSET_EL0 > 644abf522c8a arm64/sysreg: Add register fields for SPMCNTENCLR_EL0 > fad9b7751359 arm64/sysreg: Add register fields for SPMINTENSET_EL1 > c28299b8df76 arm64/sysreg: Add register fields for SPMINTENCLR_EL1 > b9c283b27980 arm64/sysreg: Add register fields for SPMOVSSET_EL0 > 03dd01a26c46 arm64/sysreg: Add register fields for SPMOVSCLR_EL0 > c8a2f1b688de arm64/sysreg: Add register fields for SPMCR_EL0 > 422ca4026aa7 arm64/sysreg: Add register fields for SPMACCESSR_EL1 > d790b2570461 arm64/sysreg: Add register fields for SPMSCR_EL1 > c2cdd0fecdcb arm64/sysreg: Add register fields for PMCCNTSVR_EL1 > 3b793f3f07b8 arm64/sysreg: Add register fields for PMUACR_EL1 > a1804742ee8c arm64/sysreg: Add register fields for PMICFILTR_EL0 > c88476b9c52c arm64/sysreg: Add register fields for PMICNTR_EL0 > 6d1520f3477b arm64/sysreg: Add register fields for PMECR_EL1 > 257ea3ec7950 arm64/sysreg: Add register fields for PMIAR_EL1 > a29f787102f0 arm64/sysreg: Add register fields for SPMDEVAFF_EL1 > 3b7c4d4cf0eb arm64/sysreg: Add register fields for PMSDSFR_EL1 > 2a14e5dc1903 arm64/sysreg: Add register fields for TRBMPAM_EL1 Have you done this by hand? Or have you used an automated generator that parses the XML? If it's the former, please do the latter and compare the results. [...] > > > >>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > >>>> index f921af014d0c..8029f408855d 100644 > >>>> --- a/arch/arm64/kvm/sys_regs.c > >>>> +++ b/arch/arm64/kvm/sys_regs.c > >>>> @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) > >>>> kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | > >>>> HAFGRTR_EL2_RES1); > >>>> > >>>> + /* FEAT_TRBE_MPAM is not exposed to the guest */ > >>>> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1); > >>> > >>> Same thing about dynamic configuration. > >>> > >>> But more importantly, you are disabling anything *but* MPAM. Does it > >>> seem right to you? > >> > >> Did not get that, should the inverse ~ be dropped here and also for all > >> other negative trap bits across the register ? > > > > Look at the way FGU works. A bit set to 1 means that if we have > > trapped because of this bit (as per the FGT table), we inject an > > UNDEF. > > Seems like positive and negative trap bits do not make any difference > here, while setting respective bits in kvm->arch.fgu[HDFGRTR2_GROUP]. > In this case the inverse should be dropped for all bits. Yup, that's what "A bit set to 1" means here. We don't need to follow the convolutions of the HW here. > > Should the lone trap bit (nPMZR_EL0) which is present in HDFGWTR2_EL2 > but not in HDFGRTR2_EL2, be set in kvm->arch.fgu[HDFGWTR2_GROUP] as > well ? > > if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) { > kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMUACR_EL1; > kvm->arch.fgu[HDFGWTR2_GROUP] |= HDFGWTR2_EL2_nPMZR_EL0; > } Possibly. At the time this code was written, ARMv8.9 wasn't in the ARM ARM. M.
On 6/25/24 19:28, Marc Zyngier wrote: > On Tue, 25 Jun 2024 10:03:29 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> wrote: >> >>>>>> +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) >>>>> >>>>> Note the *nMASK* here. 'n' is for *negative*. Just look at how >>>>> __HDFGRTR_EL2_MASK and __HDFGRTR_EL2_nMASK are written. >>>> >>>> Still trying to understand what does these three masks represent >>>> for a given FGT register XXXX >>>> >>>> - __XXXX_RES0 >>> >>> RES0 bits. >>> >>>> - __XXXX_MASK >>> >>> Positive trap bits. >>> >>>> - __XXXX_nMASK >>> >>> Negative trap bits. >> >> Right, figured that eventually but these were not very clear at >> first. > > I keep hearing I'm not clear. But at this stage, I don't know what to > do to make it (or myself) clearer. > >> >>> >>>> >>>> But from the mentioned example for HDFGRTR_EL2. >>>> >>>> #define __HDFGRTR_EL2_RES0 HDFGRTR_EL2_RES0 >>>> #define __HDFGRTR_EL2_MASK (BIT(63) | GENMASK(58, 50) | GENMASK(48, 43) | \ >>>> GENMASK(41, 40) | GENMASK(37, 22) | \ >>>> GENMASK(19, 9) | GENMASK(7, 0)) >>>> #define __HDFGRTR_EL2_nMASK ~(__HDFGRTR_EL2_RES0 | __HDFGRTR_EL2_MASK) >>>> >>>> Looking at HDFGRTR_EL2 definition inside arch/arm64/tools/sysreg >>>> >>>> HDFGRTR_EL2_RES0 = BIT(49) | GENMASK(39, 38) | GENMASK(21, 20) | BIT(8) >>>> >>>> is representing the entire mask in the register which are RES0. But then what >>>> does __HDFGRTR_EL2_MASK signify ? Positive trap bits mask ? >>>> >>>> The following bits belong in __HDFGRTR_EL2_MASK >>>> >>>> HDFGRTR_EL2.PMBIDR_EL1 (63) >>>> HDFGRTR_EL2.PMCEIDn_EL0 (58) >>>> >>>> Where as the following bits belong in __HDFGRTR_EL2_nMASK >>>> >>>> HDFGRTR_EL2.nPMSNEVFR_EL1 (61) >>>> HDFGRTR_EL2.nBRBCTL (60) >>>> >>>> Reworking proposed HDFGRTR2_EL2 and HDFGWTR2_EL2. >>>> >>>> #define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 >>>> #define __HDFGRTR2_EL2_MASK 0 >>>> #define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) >>>> >>>> #define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 >>>> #define __HDFGWTR2_EL2_MASK 0 >>>> #define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) >>>> >>>> Please note that all trap bits in both these registers are negative ones >>>> hence __HDFGRTR2_EL2_MASK/__HDFGWTR2_EL2_MASK should be 0. >>> >>> That's because you're looking at the XML, and not the ARM ARM this was >>> written against. >> >> Did not follow that. Both in ddi0601/2024-03 XML and ARM ARM DDI 0487K.a >> there are no positive trap bits, either in HDFGRTR2_EL2 or HDFGWTR2_EL2. >> OR did I miss something here. > >>From this very file: > > <quote> > /* > * FGT register definitions > * > * RES0 and polarity masks as of DDI0487J.a, to be updated as needed. > * We're not using the generated masks as they are usually ahead of > * the published ARM ARM, which we use as a reference. > * > * Once we get to a point where the two describe the same thing, we'll > * merge the definitions. One day. > */ > </quote> > > In case it wasn't written *CLEARLY* enough. > >> Sure, will add the following new registers in arch/arm64/tools/sysreg format >> except for the ones that require a formula based enumeration. Those will be >> added into arch/arm64/include/asm/sysreg.h directly e.g >> >> +#define SYS_SPMEVCNTR_EL0(m) sys_reg(2, 3, 14, (0 | (m >> 3)), (m & 7)) >> +#define SYS_SPMEVTYPER_EL0(m) sys_reg(2, 3, 14, (2 | (m >> 3)), (m & 7)) >> +#define SYS_SPMEVFILTR_EL0(m) sys_reg(2, 3, 14, (4 | (m >> 3)), (m & 7)) >> +#define SYS_SPMEVFILT2R_EL0(m) sys_reg(2, 3, 14, (6 | (m >> 3)), (m & 7)) >> >> 9d93fc432f1a arm64/sysreg: Add remaining debug registers affected by HDFGxTR2_EL2 >> 56b8830f0a5e arm64/sysreg: Add register fields for SPMCGCR1_EL1 >> ad674ae52178 arm64/sysreg: Add register fields for SPMCGCR0_EL1 >> 8538a282d208 arm64/sysreg: Add register fields for PMZR_EL0 >> c88f0b8d898e arm64/sysreg: Add register fields for PMSSCR_EL1 >> 8788ad49b7b6 arm64/sysreg: Add register fields for SPMCFGR_EL1 >> 142de2bc3d7b arm64/sysreg: Add register fields for SPMDEVARCH_EL1 >> 3d903ba35e8c arm64/sysreg: Add register fields for SPMIIDR_EL1 >> 1a4db0b8b100 arm64/sysreg: Add register fields for PMICNTSVR_EL1 >> 6bee8f139ba5 arm64/sysreg: Add register fields for SPMSELR_EL0 >> b208ab4cd54d arm64/sysreg: Add register fields for SPMCNTENSET_EL0 >> 644abf522c8a arm64/sysreg: Add register fields for SPMCNTENCLR_EL0 >> fad9b7751359 arm64/sysreg: Add register fields for SPMINTENSET_EL1 >> c28299b8df76 arm64/sysreg: Add register fields for SPMINTENCLR_EL1 >> b9c283b27980 arm64/sysreg: Add register fields for SPMOVSSET_EL0 >> 03dd01a26c46 arm64/sysreg: Add register fields for SPMOVSCLR_EL0 >> c8a2f1b688de arm64/sysreg: Add register fields for SPMCR_EL0 >> 422ca4026aa7 arm64/sysreg: Add register fields for SPMACCESSR_EL1 >> d790b2570461 arm64/sysreg: Add register fields for SPMSCR_EL1 >> c2cdd0fecdcb arm64/sysreg: Add register fields for PMCCNTSVR_EL1 >> 3b793f3f07b8 arm64/sysreg: Add register fields for PMUACR_EL1 >> a1804742ee8c arm64/sysreg: Add register fields for PMICFILTR_EL0 >> c88476b9c52c arm64/sysreg: Add register fields for PMICNTR_EL0 >> 6d1520f3477b arm64/sysreg: Add register fields for PMECR_EL1 >> 257ea3ec7950 arm64/sysreg: Add register fields for PMIAR_EL1 >> a29f787102f0 arm64/sysreg: Add register fields for SPMDEVAFF_EL1 >> 3b7c4d4cf0eb arm64/sysreg: Add register fields for PMSDSFR_EL1 >> 2a14e5dc1903 arm64/sysreg: Add register fields for TRBMPAM_EL1 > > Have you done this by hand? Or have you used an automated generator > that parses the XML? If it's the former, please do the latter and > compare the results. > > [...] > >>> >>>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>>>>> index f921af014d0c..8029f408855d 100644 >>>>>> --- a/arch/arm64/kvm/sys_regs.c >>>>>> +++ b/arch/arm64/kvm/sys_regs.c >>>>>> @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) >>>>>> kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | >>>>>> HAFGRTR_EL2_RES1); >>>>>> >>>>>> + /* FEAT_TRBE_MPAM is not exposed to the guest */ >>>>>> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1); >>>>> >>>>> Same thing about dynamic configuration. >>>>> >>>>> But more importantly, you are disabling anything *but* MPAM. Does it >>>>> seem right to you? >>>> >>>> Did not get that, should the inverse ~ be dropped here and also for all >>>> other negative trap bits across the register ? >>> >>> Look at the way FGU works. A bit set to 1 means that if we have >>> trapped because of this bit (as per the FGT table), we inject an >>> UNDEF. >> >> Seems like positive and negative trap bits do not make any difference >> here, while setting respective bits in kvm->arch.fgu[HDFGRTR2_GROUP]. >> In this case the inverse should be dropped for all bits. > > Yup, that's what "A bit set to 1" means here. We don't need to follow > the convolutions of the HW here. > >> >> Should the lone trap bit (nPMZR_EL0) which is present in HDFGWTR2_EL2 >> but not in HDFGRTR2_EL2, be set in kvm->arch.fgu[HDFGWTR2_GROUP] as >> well ? >> >> if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) { >> kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMUACR_EL1; >> kvm->arch.fgu[HDFGWTR2_GROUP] |= HDFGWTR2_EL2_nPMZR_EL0; >> } > > Possibly. At the time this code was written, ARMv8.9 wasn't in the ARM > ARM. Hello Marc, As you had suggested to avoid posting the entire series (which now consists of around ~30 patches just for all required sysregs update) to get the core part of FGT2 reviewed, please find the updated last patch here. I will post the entire series once there is some agreement on this patch. Thank you. Changes from RFC V1: - Updated positive and negative masks for HDFGRTR2_EL2 and HDFGWTR2_EL2 - Both register's positive masks are 0 (all bits are nFormat) - Both register's negative masks are all bits but the RES0 ones - Please note that sysreg field definitions for both registers were added into tools are from 2024-06 XML not ARM DDI 0487K.a - Even if ARM DDI 0487K.a gets used instead of the above XML, there are no positive mask bits, only RES0 mask will expand. - Only nPMZR_EL0 gets added for HDFGWTR2_EL2 in encoding_to_fgt() - Follows the same pattern as in HDFGWTR_EL2/HDFGWTR_EL2 - All other entries are for HDFGRTR2_EL2 - Used indirect features check for FEAT_TRBE_MPAM and FEAT_SPE_FDS - Updated kvm_init_nv_sysregs() as required - Updated kvm_calculate_traps() as required - Anshuman -------->8---------------------------------------------------------------- From c1e648dcdb603ad182bcd522ca489f7deee58e86 Mon Sep 17 00:00:00 2001 From: Anshuman Khandual <anshuman.khandual@arm.com> Date: Mon, 8 Apr 2024 12:15:35 +0530 Subject: [PATCH] KVM: arm64: nv: Add new HDFGRTR2_GROUP & HDFGRTR2_GROUP based FGU handling This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers. Cc: Marc Zyngier <maz@kernel.org> Cc: Oliver Upton <oliver.upton@linux.dev> Cc: James Morse <james.morse@arm.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: kvmarm@lists.linux.dev Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- arch/arm64/include/asm/kvm_arm.h | 8 ++ arch/arm64/include/asm/kvm_host.h | 2 + arch/arm64/kvm/emulate-nested.c | 158 ++++++++++++++++++++++++ arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++ arch/arm64/kvm/nested.c | 38 ++++++ arch/arm64/kvm/sys_regs.c | 49 ++++++++ 6 files changed, 265 insertions(+) diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index d81cc746e0eb..66ebb2f8e0fa 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -353,6 +353,14 @@ #define __HFGRTR_EL2_MASK GENMASK(49, 0) #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK) +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 +#define __HDFGRTR2_EL2_MASK 0 +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) + +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 +#define __HDFGWTR2_EL2_MASK 0 +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) + /* * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any * future additions, define __HFGWTR* macros relative to __HFGRTR* ones. diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 696bb07b4722..edf78ddb099f 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -266,6 +266,8 @@ enum fgt_group_id { HDFGWTR_GROUP = HDFGRTR_GROUP, HFGITR_GROUP, HAFGRTR_GROUP, + HDFGRTR2_GROUP, + HDFGWTR2_GROUP = HDFGRTR2_GROUP, /* Must be last */ __NR_FGT_GROUP_IDS__ diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c index 05166eccea0a..bbeeb0ab758e 100644 --- a/arch/arm64/kvm/emulate-nested.c +++ b/arch/arm64/kvm/emulate-nested.c @@ -1828,6 +1828,153 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = { SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1), SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1), SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1), + + /* + * HDFGWTR_EL2 + * + * Although HDFGRTR2_EL2 and HDFGWTR2_EL2 registers largely + * overlap in their bit assignment, there are a number of bits + * that are RES0 on one side, and an actual trap bit on the + * other. The policy chosen here is to describe all the + * read-side mappings, and only the write-side mappings that + * differ from the read side, and the trap handler will pick + * the correct shadow register based on the access type. + */ + SR_FGT(SYS_PMZR_EL0, HDFGWTR2, nPMZR_EL0, 0), + + /* HDFGRTR2_EL2 */ + SR_FGT(SYS_MDSTEPOP_EL1, HDFGRTR2, nMDSTEPOP_EL1, 0), + SR_FGT(SYS_TRBMPAM_EL1, HDFGRTR2, nTRBMPAM_EL1, 0), + SR_FGT(SYS_TRCITECR_EL1, HDFGRTR2, nTRCITECR_EL1, 0), + SR_FGT(SYS_PMSDSFR_EL1, HDFGRTR2, nPMSDSFR_EL1, 0), + SR_FGT(SYS_SPMDEVAFF_EL1, HDFGRTR2, nSPMDEVAFF_EL1, 0), + + SR_FGT(SYS_SPMCGCR0_EL1, HDFGRTR2, nSPMID, 0), + SR_FGT(SYS_SPMCGCR1_EL1, HDFGRTR2, nSPMID, 0), + SR_FGT(SYS_SPMIIDR_EL1, HDFGRTR2, nSPMID, 0), + SR_FGT(SYS_SPMDEVARCH_EL1, HDFGRTR2, nSPMID, 0), + SR_FGT(SYS_SPMCFGR_EL1, HDFGRTR2, nSPMID, 0), + + SR_FGT(SYS_SPMSCR_EL1, HDFGRTR2, nSPMSCR_EL1, 0), + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0), + SR_FGT(SYS_SPMCR_EL0, HDFGRTR2, nSPMCR_EL0, 0), + SR_FGT(SYS_SPMOVSCLR_EL0, HDFGRTR2, nSPMOVS, 0), + SR_FGT(SYS_SPMOVSSET_EL0, HDFGRTR2, nSPMOVS, 0), + SR_FGT(SYS_SPMINTENCLR_EL1, HDFGRTR2, nSPMINTEN, 0), + SR_FGT(SYS_SPMINTENSET_EL1, HDFGRTR2, nSPMINTEN, 0), + SR_FGT(SYS_SPMCNTENCLR_EL0, HDFGRTR2, nSPMCNTEN, 0), + SR_FGT(SYS_SPMCNTENSET_EL0, HDFGRTR2, nSPMCNTEN, 0), + SR_FGT(SYS_SPMSELR_EL0, HDFGRTR2, nSPMSELR_EL0, 0), + + SR_FGT(SYS_SPMEVTYPER_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVTYPER_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + + SR_FGT(SYS_SPMEVFILTR_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILTR_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + + SR_FGT(SYS_SPMEVFILT2R_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + SR_FGT(SYS_SPMEVFILT2R_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), + + SR_FGT(SYS_SPMEVCNTR_EL0(0), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(1), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(2), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(3), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(4), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(5), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(6), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(7), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(8), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(9), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(10), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(11), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(12), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(13), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(14), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + SR_FGT(SYS_SPMEVCNTR_EL0(15), HDFGRTR2, nSPMEVCNTRn_EL0, 0), + + SR_FGT(SYS_PMSSCR_EL1, HDFGRTR2, nPMSSCR_EL1, 0), + SR_FGT(SYS_PMCCNTSVR_EL1, HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMICNTSVR_EL1, HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(0), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(1), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(2), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(3), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(4), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(5), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(6), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(7), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(8), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(9), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(10), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(11), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(12), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(13), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(14), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(15), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(16), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(17), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(18), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(19), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(20), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(21), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(22), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(23), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(24), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(25), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(26), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(27), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(28), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(29), HDFGRTR2, nPMSSDATA, 0), + SR_FGT(SYS_PMEVCNTSVR_EL1(30), HDFGRTR2, nPMSSDATA, 0), + + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 0), + SR_FGT(SYS_PMUACR_EL1, HDFGRTR2, nPMUACR_EL1, 0), + SR_FGT(SYS_PMICFILTR_EL0, HDFGRTR2, nPMICFILTR_EL0, 0), + SR_FGT(SYS_PMICNTR_EL0, HDFGRTR2, nPMICNTR_EL0, 0), + SR_FGT(SYS_PMIAR_EL1, HDFGRTR2, nPMIAR_EL1, 0), + SR_FGT(SYS_PMECR_EL1, HDFGRTR2, nPMECR_EL1, 0), }; static union trap_config get_trap_config(u32 sysreg) @@ -2083,6 +2230,10 @@ static bool check_fgt_bit(struct kvm *kvm, bool is_read, sr = is_read ? HDFGRTR_EL2 : HDFGWTR_EL2; break; + case HDFGRTR2_GROUP: + sr = is_read ? HDFGRTR2_EL2 : HDFGWTR2_EL2; + break; + case HAFGRTR_GROUP: sr = HAFGRTR_EL2; break; @@ -2157,6 +2308,13 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) val = __vcpu_sys_reg(vcpu, HDFGWTR_EL2); break; + case HDFGRTR2_GROUP: + if (is_read) + val = __vcpu_sys_reg(vcpu, HDFGRTR2_EL2); + else + val = __vcpu_sys_reg(vcpu, HDFGWTR2_EL2); + break; + case HAFGRTR_GROUP: val = __vcpu_sys_reg(vcpu, HAFGRTR_EL2); break; diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index f59ccfe11ab9..af6c774d9202 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -89,6 +89,10 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) case HDFGWTR_EL2: \ id = HDFGRTR_GROUP; \ break; \ + case HDFGRTR2_EL2: \ + case HDFGWTR2_EL2: \ + id = HDFGRTR2_GROUP; \ + break; \ case HAFGRTR_EL2: \ id = HAFGRTR_GROUP; \ break; \ @@ -160,6 +164,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) CHECK_FGT_MASKS(HDFGWTR_EL2); CHECK_FGT_MASKS(HAFGRTR_EL2); CHECK_FGT_MASKS(HCRX_EL2); + CHECK_FGT_MASKS(HDFGRTR2_EL2); + CHECK_FGT_MASKS(HDFGWTR2_EL2); if (!cpus_have_final_cap(ARM64_HAS_FGT)) return; @@ -171,6 +177,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) update_fgt_traps(hctxt, vcpu, kvm, HFGITR_EL2); update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR_EL2); update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR_EL2); + update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR2_EL2); + update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR2_EL2); if (cpu_has_amu()) update_fgt_traps(hctxt, vcpu, kvm, HAFGRTR_EL2); @@ -200,6 +208,8 @@ static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu) __deactivate_fgt(hctxt, vcpu, kvm, HFGITR_EL2); __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR_EL2); __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR_EL2); + __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR2_EL2); + __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR2_EL2); if (cpu_has_amu()) __deactivate_fgt(hctxt, vcpu, kvm, HAFGRTR_EL2); diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c index de789e0f1ae9..32ac5342d340 100644 --- a/arch/arm64/kvm/nested.c +++ b/arch/arm64/kvm/nested.c @@ -1146,6 +1146,44 @@ int kvm_init_nv_sysregs(struct kvm *kvm) res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1; set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1); + /* HDFG[RW]TR2_EL2 */ + res0 = res1 = 0; + + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, ExtTrcBuff, IMP)) + res1 |= HDFGRTR2_EL2_nTRBMPAM_EL1; + + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4)) + res1 |= HDFGRTR2_EL2_nPMSDSFR_EL1; + + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) + res1 |= HDFGRTR2_EL2_nMDSTEPOP_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) + res1 |= HDFGRTR2_EL2_nTRCITECR_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) + res1 |= (HDFGRTR2_EL2_nSPMDEVAFF_EL1 | HDFGRTR2_EL2_nSPMID | + HDFGRTR2_EL2_nSPMSCR_EL1 | HDFGRTR2_EL2_nSPMACCESSR_EL1 | + HDFGRTR2_EL2_nSPMCR_EL0 | HDFGRTR2_EL2_nSPMOVS | + HDFGRTR2_EL2_nSPMINTEN | HDFGRTR2_EL2_nSPMCNTEN | + HDFGRTR2_EL2_nSPMSELR_EL0 | HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | + HDFGRTR2_EL2_nSPMEVCNTRn_EL0); + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) + res1 |= (HDFGRTR2_EL2_nPMSSCR_EL1 | HDFGRTR2_EL2_nPMSSDATA); + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) + res1 |= HDFGRTR2_EL2_nMDSELR_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) + res1 |= HDFGRTR2_EL2_nPMUACR_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) + res1 |= (HDFGRTR2_EL2_nPMICFILTR_EL0 | HDFGRTR2_EL2_nPMICNTR_EL0); + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) + res1 |= HDFGRTR2_EL2_nPMIAR_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) + res1 |= HDFGRTR2_EL2_nPMECR_EL1; + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1 | HDFGRTR2_EL2_RES1); + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1 | HDFGWTR2_EL2_RES1); + /* Reuse the bits from the read-side and add the write-specific stuff */ if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP)) res0 |= (HDFGWTR_EL2_PMCR_EL0 | HDFGWTR_EL2_PMSWINC_EL0); diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 4ea714dcb660..c971febcafab 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -4610,6 +4610,55 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu) kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | HAFGRTR_EL2_RES1); + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, ExtTrcBuff, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nTRBMPAM_EL1; + + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMSDSFR_EL1; + + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nMDSTEPOP_EL1; + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nTRCITECR_EL1; + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nSPMDEVAFF_EL1 | + HDFGRTR2_EL2_nSPMID | + HDFGRTR2_EL2_nSPMSCR_EL1 | + HDFGRTR2_EL2_nSPMACCESSR_EL1 | + HDFGRTR2_EL2_nSPMCR_EL0 | + HDFGRTR2_EL2_nSPMOVS | + HDFGRTR2_EL2_nSPMINTEN | + HDFGRTR2_EL2_nSPMCNTEN | + HDFGRTR2_EL2_nSPMSELR_EL0 | + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | + HDFGRTR2_EL2_nSPMEVCNTRn_EL0; + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMSSCR_EL1 | + HDFGRTR2_EL2_nPMSSDATA; + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nMDSELR_EL1; + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) { + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMUACR_EL1; + kvm->arch.fgu[HDFGWTR2_GROUP] |= HDFGWTR2_EL2_nPMZR_EL0; + } + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) { + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMICFILTR_EL0; + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMICNTR_EL0; + } + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMIAR_EL1; + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMECR_EL1; + set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags); out: mutex_unlock(&kvm->arch.config_lock);
On Thu, 01 Aug 2024 11:46:22 +0100, Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > Hello Marc, > > As you had suggested to avoid posting the entire series (which now consists > of around ~30 patches just for all required sysregs update) to get the core > part of FGT2 reviewed, please find the updated last patch here. I will post > the entire series once there is some agreement on this patch. Thank > you. Thanks for respining it in this form. Comments below. > > Changes from RFC V1: > > - Updated positive and negative masks for HDFGRTR2_EL2 and HDFGWTR2_EL2 > - Both register's positive masks are 0 (all bits are nFormat) > - Both register's negative masks are all bits but the RES0 ones > - Please note that sysreg field definitions for both registers > were added into tools are from 2024-06 XML not ARM DDI 0487K.a > - Even if ARM DDI 0487K.a gets used instead of the above XML, > there are no positive mask bits, only RES0 mask will expand. > > - Only nPMZR_EL0 gets added for HDFGWTR2_EL2 in encoding_to_fgt() > - Follows the same pattern as in HDFGWTR_EL2/HDFGWTR_EL2 > - All other entries are for HDFGRTR2_EL2 > > - Used indirect features check for FEAT_TRBE_MPAM and FEAT_SPE_FDS > - Updated kvm_init_nv_sysregs() as required > - Updated kvm_calculate_traps() as required > > - Anshuman > > -------->8---------------------------------------------------------------- > From c1e648dcdb603ad182bcd522ca489f7deee58e86 Mon Sep 17 00:00:00 2001 > From: Anshuman Khandual <anshuman.khandual@arm.com> > Date: Mon, 8 Apr 2024 12:15:35 +0530 > Subject: [PATCH] KVM: arm64: nv: Add new HDFGRTR2_GROUP & HDFGRTR2_GROUP based > FGU handling > > This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id > for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers. > > Cc: Marc Zyngier <maz@kernel.org> > Cc: Oliver Upton <oliver.upton@linux.dev> > Cc: James Morse <james.morse@arm.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: kvmarm@lists.linux.dev > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > arch/arm64/include/asm/kvm_arm.h | 8 ++ > arch/arm64/include/asm/kvm_host.h | 2 + > arch/arm64/kvm/emulate-nested.c | 158 ++++++++++++++++++++++++ > arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++ > arch/arm64/kvm/nested.c | 38 ++++++ > arch/arm64/kvm/sys_regs.c | 49 ++++++++ > 6 files changed, 265 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > index d81cc746e0eb..66ebb2f8e0fa 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -353,6 +353,14 @@ > #define __HFGRTR_EL2_MASK GENMASK(49, 0) > #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK) > > +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 > +#define __HDFGRTR2_EL2_MASK 0 > +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) > + > +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 > +#define __HDFGWTR2_EL2_MASK 0 > +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) > + > /* > * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any > * future additions, define __HFGWTR* macros relative to __HFGRTR* ones. > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 696bb07b4722..edf78ddb099f 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -266,6 +266,8 @@ enum fgt_group_id { > HDFGWTR_GROUP = HDFGRTR_GROUP, > HFGITR_GROUP, > HAFGRTR_GROUP, > + HDFGRTR2_GROUP, > + HDFGWTR2_GROUP = HDFGRTR2_GROUP, > > /* Must be last */ > __NR_FGT_GROUP_IDS__ > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c > index 05166eccea0a..bbeeb0ab758e 100644 > --- a/arch/arm64/kvm/emulate-nested.c > +++ b/arch/arm64/kvm/emulate-nested.c > @@ -1828,6 +1828,153 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = { > SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1), > SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1), > SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1), > + > + /* > + * HDFGWTR_EL2 > + * > + * Although HDFGRTR2_EL2 and HDFGWTR2_EL2 registers largely > + * overlap in their bit assignment, there are a number of bits > + * that are RES0 on one side, and an actual trap bit on the > + * other. The policy chosen here is to describe all the > + * read-side mappings, and only the write-side mappings that > + * differ from the read side, and the trap handler will pick > + * the correct shadow register based on the access type. > + */ > + SR_FGT(SYS_PMZR_EL0, HDFGWTR2, nPMZR_EL0, 0), Be consistent with the existing order of registers. Writes are ordered after reads, and I'd like to preserve this. > + > + /* HDFGRTR2_EL2 */ > + SR_FGT(SYS_MDSTEPOP_EL1, HDFGRTR2, nMDSTEPOP_EL1, 0), > + SR_FGT(SYS_TRBMPAM_EL1, HDFGRTR2, nTRBMPAM_EL1, 0), > + SR_FGT(SYS_TRCITECR_EL1, HDFGRTR2, nTRCITECR_EL1, 0), > + SR_FGT(SYS_PMSDSFR_EL1, HDFGRTR2, nPMSDSFR_EL1, 0), > + SR_FGT(SYS_SPMDEVAFF_EL1, HDFGRTR2, nSPMDEVAFF_EL1, 0), > + > + SR_FGT(SYS_SPMCGCR0_EL1, HDFGRTR2, nSPMID, 0), > + SR_FGT(SYS_SPMCGCR1_EL1, HDFGRTR2, nSPMID, 0), > + SR_FGT(SYS_SPMIIDR_EL1, HDFGRTR2, nSPMID, 0), > + SR_FGT(SYS_SPMDEVARCH_EL1, HDFGRTR2, nSPMID, 0), > + SR_FGT(SYS_SPMCFGR_EL1, HDFGRTR2, nSPMID, 0), > + > + SR_FGT(SYS_SPMSCR_EL1, HDFGRTR2, nSPMSCR_EL1, 0), That's a pretty odd one, as it only exists on the secure side. We will never see the access, as it will UNDEF at EL1, but hey, who cares. Let's take this as documentation. > + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0), This (and I take it most of the stuff here) is also gated by MDCR_EL2.SPM, which is a coarse grained trap. That needs to be described as well. For every new register that you add here. > + SR_FGT(SYS_SPMCR_EL0, HDFGRTR2, nSPMCR_EL0, 0), > + SR_FGT(SYS_SPMOVSCLR_EL0, HDFGRTR2, nSPMOVS, 0), > + SR_FGT(SYS_SPMOVSSET_EL0, HDFGRTR2, nSPMOVS, 0), > + SR_FGT(SYS_SPMINTENCLR_EL1, HDFGRTR2, nSPMINTEN, 0), > + SR_FGT(SYS_SPMINTENSET_EL1, HDFGRTR2, nSPMINTEN, 0), > + SR_FGT(SYS_SPMCNTENCLR_EL0, HDFGRTR2, nSPMCNTEN, 0), > + SR_FGT(SYS_SPMCNTENSET_EL0, HDFGRTR2, nSPMCNTEN, 0), > + SR_FGT(SYS_SPMSELR_EL0, HDFGRTR2, nSPMSELR_EL0, 0), > + > + SR_FGT(SYS_SPMEVTYPER_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVTYPER_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + > + SR_FGT(SYS_SPMEVFILTR_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILTR_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + > + SR_FGT(SYS_SPMEVFILT2R_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + SR_FGT(SYS_SPMEVFILT2R_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), > + > + SR_FGT(SYS_SPMEVCNTR_EL0(0), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(1), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(2), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(3), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(4), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(5), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(6), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(7), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(8), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(9), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(10), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(11), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(12), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(13), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(14), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + SR_FGT(SYS_SPMEVCNTR_EL0(15), HDFGRTR2, nSPMEVCNTRn_EL0, 0), > + > + SR_FGT(SYS_PMSSCR_EL1, HDFGRTR2, nPMSSCR_EL1, 0), > + SR_FGT(SYS_PMCCNTSVR_EL1, HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMICNTSVR_EL1, HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(0), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(1), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(2), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(3), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(4), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(5), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(6), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(7), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(8), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(9), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(10), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(11), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(12), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(13), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(14), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(15), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(16), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(17), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(18), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(19), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(20), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(21), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(22), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(23), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(24), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(25), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(26), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(27), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(28), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(29), HDFGRTR2, nPMSSDATA, 0), > + SR_FGT(SYS_PMEVCNTSVR_EL1(30), HDFGRTR2, nPMSSDATA, 0), > + > + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 0), > + SR_FGT(SYS_PMUACR_EL1, HDFGRTR2, nPMUACR_EL1, 0), > + SR_FGT(SYS_PMICFILTR_EL0, HDFGRTR2, nPMICFILTR_EL0, 0), > + SR_FGT(SYS_PMICNTR_EL0, HDFGRTR2, nPMICNTR_EL0, 0), > + SR_FGT(SYS_PMIAR_EL1, HDFGRTR2, nPMIAR_EL1, 0), > + SR_FGT(SYS_PMECR_EL1, HDFGRTR2, nPMECR_EL1, 0), > }; > > static union trap_config get_trap_config(u32 sysreg) > @@ -2083,6 +2230,10 @@ static bool check_fgt_bit(struct kvm *kvm, bool is_read, > sr = is_read ? HDFGRTR_EL2 : HDFGWTR_EL2; > break; > > + case HDFGRTR2_GROUP: > + sr = is_read ? HDFGRTR2_EL2 : HDFGWTR2_EL2; > + break; > + > case HAFGRTR_GROUP: > sr = HAFGRTR_EL2; > break; > @@ -2157,6 +2308,13 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) > val = __vcpu_sys_reg(vcpu, HDFGWTR_EL2); > break; > > + case HDFGRTR2_GROUP: > + if (is_read) > + val = __vcpu_sys_reg(vcpu, HDFGRTR2_EL2); > + else > + val = __vcpu_sys_reg(vcpu, HDFGWTR2_EL2); > + break; > + > case HAFGRTR_GROUP: > val = __vcpu_sys_reg(vcpu, HAFGRTR_EL2); > break; > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index f59ccfe11ab9..af6c774d9202 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -89,6 +89,10 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > case HDFGWTR_EL2: \ > id = HDFGRTR_GROUP; \ > break; \ > + case HDFGRTR2_EL2: \ > + case HDFGWTR2_EL2: \ > + id = HDFGRTR2_GROUP; \ > + break; \ > case HAFGRTR_EL2: \ > id = HAFGRTR_GROUP; \ > break; \ > @@ -160,6 +164,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) > CHECK_FGT_MASKS(HDFGWTR_EL2); > CHECK_FGT_MASKS(HAFGRTR_EL2); > CHECK_FGT_MASKS(HCRX_EL2); > + CHECK_FGT_MASKS(HDFGRTR2_EL2); > + CHECK_FGT_MASKS(HDFGWTR2_EL2); > > if (!cpus_have_final_cap(ARM64_HAS_FGT)) > return; > @@ -171,6 +177,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) > update_fgt_traps(hctxt, vcpu, kvm, HFGITR_EL2); > update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR_EL2); > update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR_EL2); > + update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR2_EL2); > + update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR2_EL2); > > if (cpu_has_amu()) > update_fgt_traps(hctxt, vcpu, kvm, HAFGRTR_EL2); > @@ -200,6 +208,8 @@ static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu) > __deactivate_fgt(hctxt, vcpu, kvm, HFGITR_EL2); > __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR_EL2); > __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR_EL2); > + __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR2_EL2); > + __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR2_EL2); > > if (cpu_has_amu()) > __deactivate_fgt(hctxt, vcpu, kvm, HAFGRTR_EL2); > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c > index de789e0f1ae9..32ac5342d340 100644 > --- a/arch/arm64/kvm/nested.c > +++ b/arch/arm64/kvm/nested.c > @@ -1146,6 +1146,44 @@ int kvm_init_nv_sysregs(struct kvm *kvm) > res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1; > set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1); > > + /* HDFG[RW]TR2_EL2 */ > + res0 = res1 = 0; > + > + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, ExtTrcBuff, IMP)) > + res1 |= HDFGRTR2_EL2_nTRBMPAM_EL1; I know I told you do use 'res1' here at some point, but I also told you that I was wrong in [1], and that this should be definitely be 'res0'. I'm really sorry to have led you down the wrong path, but that's a pretty minor change (see commit cb52b5c8b81). [1] https://lore.kernel.org/kvmarm/86bk3c3uss.wl-maz@kernel.org/ > + > + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4)) > + res1 |= HDFGRTR2_EL2_nPMSDSFR_EL1; > + > + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) > + res1 |= HDFGRTR2_EL2_nMDSTEPOP_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) > + res1 |= HDFGRTR2_EL2_nTRCITECR_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) > + res1 |= (HDFGRTR2_EL2_nSPMDEVAFF_EL1 | HDFGRTR2_EL2_nSPMID | > + HDFGRTR2_EL2_nSPMSCR_EL1 | HDFGRTR2_EL2_nSPMACCESSR_EL1 | > + HDFGRTR2_EL2_nSPMCR_EL0 | HDFGRTR2_EL2_nSPMOVS | > + HDFGRTR2_EL2_nSPMINTEN | HDFGRTR2_EL2_nSPMCNTEN | > + HDFGRTR2_EL2_nSPMSELR_EL0 | HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | > + HDFGRTR2_EL2_nSPMEVCNTRn_EL0); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) > + res1 |= (HDFGRTR2_EL2_nPMSSCR_EL1 | HDFGRTR2_EL2_nPMSSDATA); > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) > + res1 |= HDFGRTR2_EL2_nMDSELR_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) > + res1 |= HDFGRTR2_EL2_nPMUACR_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) > + res1 |= (HDFGRTR2_EL2_nPMICFILTR_EL0 | HDFGRTR2_EL2_nPMICNTR_EL0); > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) > + res1 |= HDFGRTR2_EL2_nPMIAR_EL1; > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && > + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) > + res1 |= HDFGRTR2_EL2_nPMECR_EL1; > + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1 | HDFGRTR2_EL2_RES1); > + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1 | HDFGWTR2_EL2_RES1); > + This is missing nPMZR_EL0, which should only apply to the write register. > /* Reuse the bits from the read-side and add the write-specific stuff */ > if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP)) > res0 |= (HDFGWTR_EL2_PMCR_EL0 | HDFGWTR_EL2_PMSWINC_EL0); > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 4ea714dcb660..c971febcafab 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -4610,6 +4610,55 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu) > kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | > HAFGRTR_EL2_RES1); > > + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, ExtTrcBuff, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nTRBMPAM_EL1; > + > + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMSDSFR_EL1; > + > + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nMDSTEPOP_EL1; > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nTRCITECR_EL1; > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nSPMDEVAFF_EL1 | > + HDFGRTR2_EL2_nSPMID | > + HDFGRTR2_EL2_nSPMSCR_EL1 | > + HDFGRTR2_EL2_nSPMACCESSR_EL1 | > + HDFGRTR2_EL2_nSPMCR_EL0 | > + HDFGRTR2_EL2_nSPMOVS | > + HDFGRTR2_EL2_nSPMINTEN | > + HDFGRTR2_EL2_nSPMCNTEN | > + HDFGRTR2_EL2_nSPMSELR_EL0 | > + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | > + HDFGRTR2_EL2_nSPMEVCNTRn_EL0; > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMSSCR_EL1 | > + HDFGRTR2_EL2_nPMSSDATA; > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nMDSELR_EL1; > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) { > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMUACR_EL1; > + kvm->arch.fgu[HDFGWTR2_GROUP] |= HDFGWTR2_EL2_nPMZR_EL0; The registers are different, but the *groups* are the same: if something UNDEFs for read, it also UNDEFs for write. So you can be consistent and use the 'read' name everywhere. This is also consistent with what you add in kvm_host.h. > + } > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) { > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMICFILTR_EL0; > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMICNTR_EL0; > + } > + > + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMIAR_EL1; > + > + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && > + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) > + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMECR_EL1; > + > set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags); > out: > mutex_unlock(&kvm->arch.config_lock); I haven't looked into the fine details, but overall, this looks much better. Now, the main issues are that: - you're missing the coarse grained trapping for all the stuff you have just added. It's not a huge amount of work, but you need, for each register, to describe what traps apply to it. The fine grained stuff is most, but not all of it. There should be enough of it already to guide you through it. - this is all part of FEAT_FGT2, and that has some side effects: HFGITR2_EL2, HFGRTR2_EL2, and HFGWTR2_EL2 must also be defined. Thankfully, there is not much in them, so this should be much easier than the above. But the architecture do not give us the option to have two but not the others. Once you have all that, we should be in a reasonable place. Thanks, M.
On 8/1/24 21:33, Marc Zyngier wrote: > On Thu, 01 Aug 2024 11:46:22 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> wrote: >> >> Hello Marc, >> >> As you had suggested to avoid posting the entire series (which now consists >> of around ~30 patches just for all required sysregs update) to get the core >> part of FGT2 reviewed, please find the updated last patch here. I will post >> the entire series once there is some agreement on this patch. Thank >> you. > > Thanks for respining it in this form. Comments below. > >> >> Changes from RFC V1: >> >> - Updated positive and negative masks for HDFGRTR2_EL2 and HDFGWTR2_EL2 >> - Both register's positive masks are 0 (all bits are nFormat) >> - Both register's negative masks are all bits but the RES0 ones >> - Please note that sysreg field definitions for both registers >> were added into tools are from 2024-06 XML not ARM DDI 0487K.a >> - Even if ARM DDI 0487K.a gets used instead of the above XML, >> there are no positive mask bits, only RES0 mask will expand. >> >> - Only nPMZR_EL0 gets added for HDFGWTR2_EL2 in encoding_to_fgt() >> - Follows the same pattern as in HDFGWTR_EL2/HDFGWTR_EL2 >> - All other entries are for HDFGRTR2_EL2 >> >> - Used indirect features check for FEAT_TRBE_MPAM and FEAT_SPE_FDS >> - Updated kvm_init_nv_sysregs() as required >> - Updated kvm_calculate_traps() as required >> >> - Anshuman >> >> -------->8---------------------------------------------------------------- >> From c1e648dcdb603ad182bcd522ca489f7deee58e86 Mon Sep 17 00:00:00 2001 >> From: Anshuman Khandual <anshuman.khandual@arm.com> >> Date: Mon, 8 Apr 2024 12:15:35 +0530 >> Subject: [PATCH] KVM: arm64: nv: Add new HDFGRTR2_GROUP & HDFGRTR2_GROUP based >> FGU handling >> >> This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id >> for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers. >> >> Cc: Marc Zyngier <maz@kernel.org> >> Cc: Oliver Upton <oliver.upton@linux.dev> >> Cc: James Morse <james.morse@arm.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: kvmarm@lists.linux.dev >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> arch/arm64/include/asm/kvm_arm.h | 8 ++ >> arch/arm64/include/asm/kvm_host.h | 2 + >> arch/arm64/kvm/emulate-nested.c | 158 ++++++++++++++++++++++++ >> arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++ >> arch/arm64/kvm/nested.c | 38 ++++++ >> arch/arm64/kvm/sys_regs.c | 49 ++++++++ >> 6 files changed, 265 insertions(+) >> >> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h >> index d81cc746e0eb..66ebb2f8e0fa 100644 >> --- a/arch/arm64/include/asm/kvm_arm.h >> +++ b/arch/arm64/include/asm/kvm_arm.h >> @@ -353,6 +353,14 @@ >> #define __HFGRTR_EL2_MASK GENMASK(49, 0) >> #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK) >> >> +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 >> +#define __HDFGRTR2_EL2_MASK 0 >> +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) >> + >> +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 >> +#define __HDFGWTR2_EL2_MASK 0 >> +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) >> + >> /* >> * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any >> * future additions, define __HFGWTR* macros relative to __HFGRTR* ones. >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 696bb07b4722..edf78ddb099f 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -266,6 +266,8 @@ enum fgt_group_id { >> HDFGWTR_GROUP = HDFGRTR_GROUP, >> HFGITR_GROUP, >> HAFGRTR_GROUP, >> + HDFGRTR2_GROUP, >> + HDFGWTR2_GROUP = HDFGRTR2_GROUP, >> >> /* Must be last */ >> __NR_FGT_GROUP_IDS__ >> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c >> index 05166eccea0a..bbeeb0ab758e 100644 >> --- a/arch/arm64/kvm/emulate-nested.c >> +++ b/arch/arm64/kvm/emulate-nested.c >> @@ -1828,6 +1828,153 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = { >> SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1), >> SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1), >> SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1), >> + >> + /* >> + * HDFGWTR_EL2 >> + * >> + * Although HDFGRTR2_EL2 and HDFGWTR2_EL2 registers largely >> + * overlap in their bit assignment, there are a number of bits >> + * that are RES0 on one side, and an actual trap bit on the >> + * other. The policy chosen here is to describe all the >> + * read-side mappings, and only the write-side mappings that >> + * differ from the read side, and the trap handler will pick >> + * the correct shadow register based on the access type. >> + */ >> + SR_FGT(SYS_PMZR_EL0, HDFGWTR2, nPMZR_EL0, 0), > > Be consistent with the existing order of registers. Writes are ordered > after reads, and I'd like to preserve this. Sure, will move this just after HDFGRTR2_EL2. > >> + >> + /* HDFGRTR2_EL2 */ >> + SR_FGT(SYS_MDSTEPOP_EL1, HDFGRTR2, nMDSTEPOP_EL1, 0), >> + SR_FGT(SYS_TRBMPAM_EL1, HDFGRTR2, nTRBMPAM_EL1, 0), >> + SR_FGT(SYS_TRCITECR_EL1, HDFGRTR2, nTRCITECR_EL1, 0), >> + SR_FGT(SYS_PMSDSFR_EL1, HDFGRTR2, nPMSDSFR_EL1, 0), >> + SR_FGT(SYS_SPMDEVAFF_EL1, HDFGRTR2, nSPMDEVAFF_EL1, 0), >> + >> + SR_FGT(SYS_SPMCGCR0_EL1, HDFGRTR2, nSPMID, 0), >> + SR_FGT(SYS_SPMCGCR1_EL1, HDFGRTR2, nSPMID, 0), >> + SR_FGT(SYS_SPMIIDR_EL1, HDFGRTR2, nSPMID, 0), >> + SR_FGT(SYS_SPMDEVARCH_EL1, HDFGRTR2, nSPMID, 0), >> + SR_FGT(SYS_SPMCFGR_EL1, HDFGRTR2, nSPMID, 0), >> + >> + SR_FGT(SYS_SPMSCR_EL1, HDFGRTR2, nSPMSCR_EL1, 0), > > That's a pretty odd one, as it only exists on the secure side. We will > never see the access, as it will UNDEF at EL1, but hey, who cares. > Let's take this as documentation. Sure, will keep this unchanged. > >> + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0), > > This (and I take it most of the stuff here) is also gated by > MDCR_EL2.SPM, which is a coarse grained trap. That needs to be > described as well. For every new register that you add here. I did not find a SPM field in MDCR_EL2 either in latest ARM ARM or in the latest XML. But as per current HDFGRTR2_EL2 description the field nSPMACCESSR_EL1 is gated by FEAT_SPMU feature, which is being checked via ID_AA64DFR1_EL1.PMU when required. So could you please give some more details. > >> + SR_FGT(SYS_SPMCR_EL0, HDFGRTR2, nSPMCR_EL0, 0), >> + SR_FGT(SYS_SPMOVSCLR_EL0, HDFGRTR2, nSPMOVS, 0), >> + SR_FGT(SYS_SPMOVSSET_EL0, HDFGRTR2, nSPMOVS, 0), >> + SR_FGT(SYS_SPMINTENCLR_EL1, HDFGRTR2, nSPMINTEN, 0), >> + SR_FGT(SYS_SPMINTENSET_EL1, HDFGRTR2, nSPMINTEN, 0), >> + SR_FGT(SYS_SPMCNTENCLR_EL0, HDFGRTR2, nSPMCNTEN, 0), >> + SR_FGT(SYS_SPMCNTENSET_EL0, HDFGRTR2, nSPMCNTEN, 0), >> + SR_FGT(SYS_SPMSELR_EL0, HDFGRTR2, nSPMSELR_EL0, 0), >> + >> + SR_FGT(SYS_SPMEVTYPER_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVTYPER_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + >> + SR_FGT(SYS_SPMEVFILTR_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILTR_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + >> + SR_FGT(SYS_SPMEVFILT2R_EL0(0), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(1), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(2), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(3), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(4), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(5), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(6), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(7), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(8), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(9), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(10), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(11), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(12), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(13), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(14), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + SR_FGT(SYS_SPMEVFILT2R_EL0(15), HDFGRTR2, nSPMEVTYPERn_EL0, 0), >> + >> + SR_FGT(SYS_SPMEVCNTR_EL0(0), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(1), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(2), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(3), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(4), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(5), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(6), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(7), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(8), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(9), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(10), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(11), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(12), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(13), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(14), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + SR_FGT(SYS_SPMEVCNTR_EL0(15), HDFGRTR2, nSPMEVCNTRn_EL0, 0), >> + >> + SR_FGT(SYS_PMSSCR_EL1, HDFGRTR2, nPMSSCR_EL1, 0), >> + SR_FGT(SYS_PMCCNTSVR_EL1, HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMICNTSVR_EL1, HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(0), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(1), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(2), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(3), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(4), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(5), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(6), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(7), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(8), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(9), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(10), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(11), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(12), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(13), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(14), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(15), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(16), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(17), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(18), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(19), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(20), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(21), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(22), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(23), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(24), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(25), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(26), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(27), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(28), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(29), HDFGRTR2, nPMSSDATA, 0), >> + SR_FGT(SYS_PMEVCNTSVR_EL1(30), HDFGRTR2, nPMSSDATA, 0), >> + >> + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 0), >> + SR_FGT(SYS_PMUACR_EL1, HDFGRTR2, nPMUACR_EL1, 0), >> + SR_FGT(SYS_PMICFILTR_EL0, HDFGRTR2, nPMICFILTR_EL0, 0), >> + SR_FGT(SYS_PMICNTR_EL0, HDFGRTR2, nPMICNTR_EL0, 0), >> + SR_FGT(SYS_PMIAR_EL1, HDFGRTR2, nPMIAR_EL1, 0), >> + SR_FGT(SYS_PMECR_EL1, HDFGRTR2, nPMECR_EL1, 0), >> }; >> >> static union trap_config get_trap_config(u32 sysreg) >> @@ -2083,6 +2230,10 @@ static bool check_fgt_bit(struct kvm *kvm, bool is_read, >> sr = is_read ? HDFGRTR_EL2 : HDFGWTR_EL2; >> break; >> >> + case HDFGRTR2_GROUP: >> + sr = is_read ? HDFGRTR2_EL2 : HDFGWTR2_EL2; >> + break; >> + >> case HAFGRTR_GROUP: >> sr = HAFGRTR_EL2; >> break; >> @@ -2157,6 +2308,13 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) >> val = __vcpu_sys_reg(vcpu, HDFGWTR_EL2); >> break; >> >> + case HDFGRTR2_GROUP: >> + if (is_read) >> + val = __vcpu_sys_reg(vcpu, HDFGRTR2_EL2); >> + else >> + val = __vcpu_sys_reg(vcpu, HDFGWTR2_EL2); >> + break; >> + >> case HAFGRTR_GROUP: >> val = __vcpu_sys_reg(vcpu, HAFGRTR_EL2); >> break; >> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h >> index f59ccfe11ab9..af6c774d9202 100644 >> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h >> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h >> @@ -89,6 +89,10 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) >> case HDFGWTR_EL2: \ >> id = HDFGRTR_GROUP; \ >> break; \ >> + case HDFGRTR2_EL2: \ >> + case HDFGWTR2_EL2: \ >> + id = HDFGRTR2_GROUP; \ >> + break; \ >> case HAFGRTR_EL2: \ >> id = HAFGRTR_GROUP; \ >> break; \ >> @@ -160,6 +164,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) >> CHECK_FGT_MASKS(HDFGWTR_EL2); >> CHECK_FGT_MASKS(HAFGRTR_EL2); >> CHECK_FGT_MASKS(HCRX_EL2); >> + CHECK_FGT_MASKS(HDFGRTR2_EL2); >> + CHECK_FGT_MASKS(HDFGWTR2_EL2); >> >> if (!cpus_have_final_cap(ARM64_HAS_FGT)) >> return; >> @@ -171,6 +177,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) >> update_fgt_traps(hctxt, vcpu, kvm, HFGITR_EL2); >> update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR_EL2); >> update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR_EL2); >> + update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR2_EL2); >> + update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR2_EL2); >> >> if (cpu_has_amu()) >> update_fgt_traps(hctxt, vcpu, kvm, HAFGRTR_EL2); >> @@ -200,6 +208,8 @@ static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu) >> __deactivate_fgt(hctxt, vcpu, kvm, HFGITR_EL2); >> __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR_EL2); >> __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR_EL2); >> + __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR2_EL2); >> + __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR2_EL2); >> >> if (cpu_has_amu()) >> __deactivate_fgt(hctxt, vcpu, kvm, HAFGRTR_EL2); >> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c >> index de789e0f1ae9..32ac5342d340 100644 >> --- a/arch/arm64/kvm/nested.c >> +++ b/arch/arm64/kvm/nested.c >> @@ -1146,6 +1146,44 @@ int kvm_init_nv_sysregs(struct kvm *kvm) >> res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1; >> set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1); >> >> + /* HDFG[RW]TR2_EL2 */ >> + res0 = res1 = 0; >> + >> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, ExtTrcBuff, IMP)) >> + res1 |= HDFGRTR2_EL2_nTRBMPAM_EL1; > > I know I told you do use 'res1' here at some point, but I also told > you that I was wrong in [1], and that this should be definitely be > 'res0'. I'm really sorry to have led you down the wrong path, but > that's a pretty minor change (see commit cb52b5c8b81). > > [1] https://lore.kernel.org/kvmarm/86bk3c3uss.wl-maz@kernel.org/ Alright, will changes all that is res1 as res0. > >> + >> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4)) >> + res1 |= HDFGRTR2_EL2_nPMSDSFR_EL1; >> + >> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) >> + res1 |= HDFGRTR2_EL2_nMDSTEPOP_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) >> + res1 |= HDFGRTR2_EL2_nTRCITECR_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) >> + res1 |= (HDFGRTR2_EL2_nSPMDEVAFF_EL1 | HDFGRTR2_EL2_nSPMID | >> + HDFGRTR2_EL2_nSPMSCR_EL1 | HDFGRTR2_EL2_nSPMACCESSR_EL1 | >> + HDFGRTR2_EL2_nSPMCR_EL0 | HDFGRTR2_EL2_nSPMOVS | >> + HDFGRTR2_EL2_nSPMINTEN | HDFGRTR2_EL2_nSPMCNTEN | >> + HDFGRTR2_EL2_nSPMSELR_EL0 | HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | >> + HDFGRTR2_EL2_nSPMEVCNTRn_EL0); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) >> + res1 |= (HDFGRTR2_EL2_nPMSSCR_EL1 | HDFGRTR2_EL2_nPMSSDATA); >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) >> + res1 |= HDFGRTR2_EL2_nMDSELR_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) >> + res1 |= HDFGRTR2_EL2_nPMUACR_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) >> + res1 |= (HDFGRTR2_EL2_nPMICFILTR_EL0 | HDFGRTR2_EL2_nPMICNTR_EL0); >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) >> + res1 |= HDFGRTR2_EL2_nPMIAR_EL1; >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && >> + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) >> + res1 |= HDFGRTR2_EL2_nPMECR_EL1; >> + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1 | HDFGRTR2_EL2_RES1); >> + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1 | HDFGWTR2_EL2_RES1); >> + > > This is missing nPMZR_EL0, which should only apply to the write register. I did realize that subsequently. Hence will changes it as the following which adds HDFGWTR2_EL2_nPMZR_EL0 to res0 just before HDFGWTR2_EL2 gets updated. set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1 | HDFGRTR2_EL2_RES1); if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) res0 |= HDFGWTR2_EL2_nPMZR_EL0; set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1 | HDFGWTR2_EL2_RES1); > >> /* Reuse the bits from the read-side and add the write-specific stuff */ >> if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP)) >> res0 |= (HDFGWTR_EL2_PMCR_EL0 | HDFGWTR_EL2_PMSWINC_EL0); >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 4ea714dcb660..c971febcafab 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -4610,6 +4610,55 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu) >> kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | >> HAFGRTR_EL2_RES1); >> >> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, ExtTrcBuff, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nTRBMPAM_EL1; >> + >> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMSDSFR_EL1; >> + >> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nMDSTEPOP_EL1; >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nTRCITECR_EL1; >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nSPMDEVAFF_EL1 | >> + HDFGRTR2_EL2_nSPMID | >> + HDFGRTR2_EL2_nSPMSCR_EL1 | >> + HDFGRTR2_EL2_nSPMACCESSR_EL1 | >> + HDFGRTR2_EL2_nSPMCR_EL0 | >> + HDFGRTR2_EL2_nSPMOVS | >> + HDFGRTR2_EL2_nSPMINTEN | >> + HDFGRTR2_EL2_nSPMCNTEN | >> + HDFGRTR2_EL2_nSPMSELR_EL0 | >> + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | >> + HDFGRTR2_EL2_nSPMEVCNTRn_EL0; >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMSSCR_EL1 | >> + HDFGRTR2_EL2_nPMSSDATA; >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nMDSELR_EL1; >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) { >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMUACR_EL1; >> + kvm->arch.fgu[HDFGWTR2_GROUP] |= HDFGWTR2_EL2_nPMZR_EL0; > > The registers are different, but the *groups* are the same: if > something UNDEFs for read, it also UNDEFs for write. So you can be > consistent and use the 'read' name everywhere. Sure, will change that as follows. if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) { kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMUACR_EL1; kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGWTR2_EL2_nPMZR_EL0; } > > This is also consistent with what you add in kvm_host.h. Agreed. > >> + } >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) { >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMICFILTR_EL0; >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMICNTR_EL0; >> + } >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMIAR_EL1; >> + >> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && >> + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) >> + kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMECR_EL1; >> + >> set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags); >> out: >> mutex_unlock(&kvm->arch.config_lock); > > I haven't looked into the fine details, but overall, this looks much > better. Alright. > > Now, the main issues are that: > > - you're missing the coarse grained trapping for all the stuff you > have just added. It's not a huge amount of work, but you need, for > each register, to describe what traps apply to it. The fine grained > stuff is most, but not all of it. There should be enough of it > already to guide you through it. Coarse grained trapping for FEAT_FGT2 based fine grained registers ? Afraid, did not understand this. Could you please give some pointers on similar existing code. > > - this is all part of FEAT_FGT2, and that has some side effects: > HFGITR2_EL2, HFGRTR2_EL2, and HFGWTR2_EL2 must also be defined. > Thankfully, there is not much in them, so this should be much easier > than the above. But the architecture do not give us the option to > have two but not the others. Sure, will incorporate and fold in changes required for remaining FEAT_FGT2 register i.e HFGITR2_EL2, HFGRTR2_EL2 and HFGWTR2_EL2. In fact could write down some initial changes and it was manageable. > > Once you have all that, we should be in a reasonable place. Sure, thanks for the review.
On Fri, 02 Aug 2024 10:25:44 +0100, Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > On 8/1/24 21:33, Marc Zyngier wrote: > > On Thu, 01 Aug 2024 11:46:22 +0100, > > Anshuman Khandual <anshuman.khandual@arm.com> wrote: [...] > >> + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0), > > > > This (and I take it most of the stuff here) is also gated by > > MDCR_EL2.SPM, which is a coarse grained trap. That needs to be > > described as well. For every new register that you add here. > > I did not find a SPM field in MDCR_EL2 either in latest ARM ARM or in > the latest XML. But as per current HDFGRTR2_EL2 description the field > nSPMACCESSR_EL1 is gated by FEAT_SPMU feature, which is being checked > via ID_AA64DFR1_EL1.PMU when required. So could you please give some > more details. I misspelled it. It is MDCR_EL2.EnSPM. And you are completely missing the point. It is not about HDFGRTR2_EL2, but about SPMACCESSR_EL1 (and all its little friends). To convince yourself, just look at the pseudocode for SPMACCESSR_EL1, limited to an EL1 access: elsif PSTATE.EL == EL1 then if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then UNDEFINED; elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMACCESSR_EL1 == '0') then AArch64.SystemAccessTrap(EL2, 0x18); elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then AArch64.SystemAccessTrap(EL2, 0x18); elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then if EL3SDDUndef() then UNDEFINED; else AArch64.SystemAccessTrap(EL3, 0x18); elsif EffectiveHCR_EL2_NVx() IN {'111'} then X[t, 64] = NVMem[0x8E8]; else X[t, 64] = SPMACCESSR_EL1; Can you spot the *TWO* conditions where we take an exception to EL2 with 0x18 as the EC? - One is when HDFGxTR2_EL2.nSPMACCESSR_EL1 == '0': that's a fine grained trap. - The other is when MDCR_EL2.EnSPM == '0': that's a coarse grained trap. Both conditions need to be captured in the various tables in this file, for each and every register that you describe. [...] > > Now, the main issues are that: > > > > - you're missing the coarse grained trapping for all the stuff you > > have just added. It's not a huge amount of work, but you need, for > > each register, to describe what traps apply to it. The fine grained > > stuff is most, but not all of it. There should be enough of it > > already to guide you through it. > > Coarse grained trapping for FEAT_FGT2 based fine grained registers ? Not for FEAT_FGT2. For the registers that FEAT_FGT2 traps. Can you see the difference? > Afraid, did not understand this. Could you please give some pointers > on similar existing code. See above. And if you want some example, just took at the file you are patching already. Look at how MDCR_EL2 conditions the trapping of all the debug, PMU, AMU registers, for example. There is no shortage of them. Thanks, M.
On 8/2/24 16:29, Marc Zyngier wrote: > On Fri, 02 Aug 2024 10:25:44 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> wrote: >> >> On 8/1/24 21:33, Marc Zyngier wrote: >>> On Thu, 01 Aug 2024 11:46:22 +0100, >>> Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > [...] > >>>> + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0), >>> >>> This (and I take it most of the stuff here) is also gated by >>> MDCR_EL2.SPM, which is a coarse grained trap. That needs to be >>> described as well. For every new register that you add here. >> >> I did not find a SPM field in MDCR_EL2 either in latest ARM ARM or in >> the latest XML. But as per current HDFGRTR2_EL2 description the field >> nSPMACCESSR_EL1 is gated by FEAT_SPMU feature, which is being checked >> via ID_AA64DFR1_EL1.PMU when required. So could you please give some >> more details. > > I misspelled it. It is MDCR_EL2.EnSPM. > > And you are completely missing the point. It is not about > HDFGRTR2_EL2, but about SPMACCESSR_EL1 (and all its little friends). > > To convince yourself, just look at the pseudocode for SPMACCESSR_EL1, > limited to an EL1 access: > > elsif PSTATE.EL == EL1 then > if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then > UNDEFINED; > elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMACCESSR_EL1 == '0') then > AArch64.SystemAccessTrap(EL2, 0x18); > elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then > AArch64.SystemAccessTrap(EL2, 0x18); > elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then > if EL3SDDUndef() then > UNDEFINED; > else > AArch64.SystemAccessTrap(EL3, 0x18); > elsif EffectiveHCR_EL2_NVx() IN {'111'} then > X[t, 64] = NVMem[0x8E8]; > else > X[t, 64] = SPMACCESSR_EL1; > > > Can you spot the *TWO* conditions where we take an exception to EL2 > with 0x18 as the EC? > > - One is when HDFGxTR2_EL2.nSPMACCESSR_EL1 == '0': that's a fine > grained trap. > - The other is when MDCR_EL2.EnSPM == '0': that's a coarse grained > trap. > > Both conditions need to be captured in the various tables in this > file, for each and every register that you describe. Ahh, got it now. Because now KVM knows about SPMACCESSR_EL1 register, access to that register must also be enabled via both fine grained trap (HDFGxTR2_EL2.nSPMACCESSR_EL1) and coarse grained trap (MDCR_EL2.EnSPM). For all the registers that are being added via FEAT_FGT2 here in this patch, their corresponding CGT based path also needs to be enabled via corresponding CGT_MDCR_XXX groups. > > [...] > >>> Now, the main issues are that: >>> >>> - you're missing the coarse grained trapping for all the stuff you >>> have just added. It's not a huge amount of work, but you need, for >>> each register, to describe what traps apply to it. The fine grained >>> stuff is most, but not all of it. There should be enough of it >>> already to guide you through it. >> >> Coarse grained trapping for FEAT_FGT2 based fine grained registers ? > > Not for FEAT_FGT2. For the registers that FEAT_FGT2 traps. Can you see > the difference? Understood, for example PMIAR_EL1 register which FEAT_FGT2 now traps via SR_FGT(SYS_PMIAR_EL1, HDFGRTR2, nPMIAR_EL1, 0), also needs to have corresponding coarse grained trap. SR_TRAP(SYS_PMIAR_EL1, CGT_MDCR_TPM), Similarly corresponding SR_TRAP() needs to be covered for all registers that are now being trapped with FEAT_FGT2. Example code snippet. ........ + SR_TRAP(SYS_SPMEVFILT2R_EL0(8), CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMEVFILT2R_EL0(9), CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMEVFILT2R_EL0(10), CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMEVFILT2R_EL0(11), CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMEVFILT2R_EL0(12), CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMEVFILT2R_EL0(13), CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMEVFILT2R_EL0(14), CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMEVFILT2R_EL0(15), CGT_MDCR_EnSPM), + + SR_TRAP(SYS_SPMCGCR0_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMCGCR1_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMACCESSR_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMCFGR_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMCNTENCLR_EL0, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMCNTENSET_EL0, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMCR_EL0, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMDEVAFF_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMDEVARCH_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMIIDR_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMINTENCLR_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMINTENSET_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMOVSCLR_EL0, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMOVSSET_EL0, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMSCR_EL1, CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMSELR_EL0, CGT_MDCR_EnSPM), ........ > >> Afraid, did not understand this. Could you please give some pointers >> on similar existing code. > > See above. And if you want some example, just took at the file you are > patching already. Look at how MDCR_EL2 conditions the trapping of all > the debug, PMU, AMU registers, for example. There is no shortage of > them. Right, I understand your point now. I am planning to send out the following patches (in reply to the current thread) for your review, before sending out the entire series as V1. - Patch adding VNCR details for virtual EL2 access - Patch adding FEAT_FGT2 based FGU handling - Patch adding corresponding CGT handling for registers being added above Although, can skip that step and just send out the series directly if you so prefer. Please do suggest. Thanks for all the detailed information above. - Anshuman
On Sat, 03 Aug 2024 11:38:11 +0100, Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > > On 8/2/24 16:29, Marc Zyngier wrote: > > On Fri, 02 Aug 2024 10:25:44 +0100, > > Anshuman Khandual <anshuman.khandual@arm.com> wrote: > >> > >> On 8/1/24 21:33, Marc Zyngier wrote: > >>> On Thu, 01 Aug 2024 11:46:22 +0100, > >>> Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > > > [...] > > > >>>> + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0), > >>> > >>> This (and I take it most of the stuff here) is also gated by > >>> MDCR_EL2.SPM, which is a coarse grained trap. That needs to be > >>> described as well. For every new register that you add here. > >> > >> I did not find a SPM field in MDCR_EL2 either in latest ARM ARM or in > >> the latest XML. But as per current HDFGRTR2_EL2 description the field > >> nSPMACCESSR_EL1 is gated by FEAT_SPMU feature, which is being checked > >> via ID_AA64DFR1_EL1.PMU when required. So could you please give some > >> more details. > > > > I misspelled it. It is MDCR_EL2.EnSPM. > > > > And you are completely missing the point. It is not about > > HDFGRTR2_EL2, but about SPMACCESSR_EL1 (and all its little friends). > > > > To convince yourself, just look at the pseudocode for SPMACCESSR_EL1, > > limited to an EL1 access: > > > > elsif PSTATE.EL == EL1 then > > if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then > > UNDEFINED; > > elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMACCESSR_EL1 == '0') then > > AArch64.SystemAccessTrap(EL2, 0x18); > > elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then > > AArch64.SystemAccessTrap(EL2, 0x18); > > elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then > > if EL3SDDUndef() then > > UNDEFINED; > > else > > AArch64.SystemAccessTrap(EL3, 0x18); > > elsif EffectiveHCR_EL2_NVx() IN {'111'} then > > X[t, 64] = NVMem[0x8E8]; > > else > > X[t, 64] = SPMACCESSR_EL1; > > > > > > Can you spot the *TWO* conditions where we take an exception to EL2 > > with 0x18 as the EC? > > > > - One is when HDFGxTR2_EL2.nSPMACCESSR_EL1 == '0': that's a fine > > grained trap. > > - The other is when MDCR_EL2.EnSPM == '0': that's a coarse grained > > trap. > > > > Both conditions need to be captured in the various tables in this > > file, for each and every register that you describe. > > Ahh, got it now. Because now KVM knows about SPMACCESSR_EL1 register, > access to that register must also be enabled via both fine grained trap > (HDFGxTR2_EL2.nSPMACCESSR_EL1) and coarse grained trap (MDCR_EL2.EnSPM). > > For all the registers that are being added via FEAT_FGT2 here in this > patch, their corresponding CGT based path also needs to be enabled via > corresponding CGT_MDCR_XXX groups. Exactly. > > > > > [...] > > > >>> Now, the main issues are that: > >>> > >>> - you're missing the coarse grained trapping for all the stuff you > >>> have just added. It's not a huge amount of work, but you need, for > >>> each register, to describe what traps apply to it. The fine grained > >>> stuff is most, but not all of it. There should be enough of it > >>> already to guide you through it. > >> > >> Coarse grained trapping for FEAT_FGT2 based fine grained registers ? > > > > Not for FEAT_FGT2. For the registers that FEAT_FGT2 traps. Can you see > > the difference? > > Understood, for example PMIAR_EL1 register which FEAT_FGT2 now traps via > > SR_FGT(SYS_PMIAR_EL1, HDFGRTR2, nPMIAR_EL1, 0), > > also needs to have corresponding coarse grained trap. > > SR_TRAP(SYS_PMIAR_EL1, CGT_MDCR_TPM), Yup. > > Similarly corresponding SR_TRAP() needs to be covered for all registers > that are now being trapped with FEAT_FGT2. > > Example code snippet. > > ........ > + SR_TRAP(SYS_SPMEVFILT2R_EL0(8), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(9), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(10), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(11), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(12), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(13), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(14), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(15), CGT_MDCR_EnSPM), I think it is a bit more complicated than just that. Again, look at the pseudocode: elsif PSTATE.EL == EL1 then if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then UNDEFINED; elsif HaveEL(EL3) && EL3SDDUndefPriority() && SPMACCESSR_EL3<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then UNDEFINED; elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0') then AArch64.SystemAccessTrap(EL2, 0x18); elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then AArch64.SystemAccessTrap(EL2, 0x18); elsif EL2Enabled() && SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then AArch64.SystemAccessTrap(EL2, 0x18); elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then if EL3SDDUndef() then UNDEFINED; else AArch64.SystemAccessTrap(EL3, 0x18); elsif HaveEL(EL3) && SPMACCESSR_EL3<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then if EL3SDDUndef() then UNDEFINED; else AArch64.SystemAccessTrap(EL3, 0x18); elsif !IsSPMUCounterImplemented(UInt(SPMSELR_EL0.SYSPMUSEL), (UInt(SPMSELR_EL0.BANK) * 16) + m) then X[t, 64] = Zeros(64); else X[t, 64] = SPMEVFILT2R_EL0[UInt(SPMSELR_EL0.SYSPMUSEL), (UInt(SPMSELR_EL0.BANK) * 16) + m]; which shows that an SPMEVFILT2Rn_EL0 access from EL1 traps to EL2 if: - either HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0', (check) - or MDCR_EL2.EnSPM == '0', (check) - or SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' and that last condition requires some more handling as you need to evaluate both SPMSELR_EL0.SYSPMUSEL and the corresponding field of SPMACCESSR_EL2 to make a decision. It's not majorly complicated, but it isn't solved by simply setting a static attribute. > + > + SR_TRAP(SYS_SPMCGCR0_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCGCR1_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMACCESSR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCFGR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCNTENCLR_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCNTENSET_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCR_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMDEVAFF_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMDEVARCH_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMIIDR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMINTENCLR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMINTENSET_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMOVSCLR_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMOVSSET_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMSCR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMSELR_EL0, CGT_MDCR_EnSPM), So please look at the pseudocode for each and every register you add. If there is a trap to EL2 that is described, you must have the corresponding handling. None of that is optional. > ........ > > > > >> Afraid, did not understand this. Could you please give some pointers > >> on similar existing code. > > > > See above. And if you want some example, just took at the file you are > > patching already. Look at how MDCR_EL2 conditions the trapping of all > > the debug, PMU, AMU registers, for example. There is no shortage of > > them. > > Right, I understand your point now. I am planning to send out the following > patches (in reply to the current thread) for your review, before sending out > the entire series as V1. > > - Patch adding VNCR details for virtual EL2 access > - Patch adding FEAT_FGT2 based FGU handling > - Patch adding corresponding CGT handling for registers being added above > > Although, can skip that step and just send out the series directly if you so > prefer. Please do suggest. Thanks for all the detailed information above. My suggestion is that you take a good look at the pseudocode and implement all the existing requirements. Yes, this is undesirably complicated, but I'm not the one making the rules here... Once you think you have all these requirements in place, please post the series. But not before that. If you have any question, just ask. Thanks, M.
On 8/4/24 16:35, Marc Zyngier wrote: > On Sat, 03 Aug 2024 11:38:11 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> wrote: >> >> On 8/2/24 16:29, Marc Zyngier wrote: >>> On Fri, 02 Aug 2024 10:25:44 +0100, >>> Anshuman Khandual <anshuman.khandual@arm.com> wrote: >>>> On 8/1/24 21:33, Marc Zyngier wrote: >>>>> On Thu, 01 Aug 2024 11:46:22 +0100, >>>>> Anshuman Khandual <anshuman.khandual@arm.com> wrote: >>> [...] >>> >>>>>> + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0), >>>>> This (and I take it most of the stuff here) is also gated by >>>>> MDCR_EL2.SPM, which is a coarse grained trap. That needs to be >>>>> described as well. For every new register that you add here. >>>> I did not find a SPM field in MDCR_EL2 either in latest ARM ARM or in >>>> the latest XML. But as per current HDFGRTR2_EL2 description the field >>>> nSPMACCESSR_EL1 is gated by FEAT_SPMU feature, which is being checked >>>> via ID_AA64DFR1_EL1.PMU when required. So could you please give some >>>> more details. >>> I misspelled it. It is MDCR_EL2.EnSPM. >>> >>> And you are completely missing the point. It is not about >>> HDFGRTR2_EL2, but about SPMACCESSR_EL1 (and all its little friends). >>> >>> To convince yourself, just look at the pseudocode for SPMACCESSR_EL1, >>> limited to an EL1 access: >>> >>> elsif PSTATE.EL == EL1 then >>> if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then >>> UNDEFINED; >>> elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMACCESSR_EL1 == '0') then >>> AArch64.SystemAccessTrap(EL2, 0x18); >>> elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then >>> AArch64.SystemAccessTrap(EL2, 0x18); >>> elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then >>> if EL3SDDUndef() then >>> UNDEFINED; >>> else >>> AArch64.SystemAccessTrap(EL3, 0x18); >>> elsif EffectiveHCR_EL2_NVx() IN {'111'} then >>> X[t, 64] = NVMem[0x8E8]; >>> else >>> X[t, 64] = SPMACCESSR_EL1; >>> >>> >>> Can you spot the *TWO* conditions where we take an exception to EL2 >>> with 0x18 as the EC? >>> >>> - One is when HDFGxTR2_EL2.nSPMACCESSR_EL1 == '0': that's a fine >>> grained trap. >>> - The other is when MDCR_EL2.EnSPM == '0': that's a coarse grained >>> trap. >>> >>> Both conditions need to be captured in the various tables in this >>> file, for each and every register that you describe. >> Ahh, got it now. Because now KVM knows about SPMACCESSR_EL1 register, >> access to that register must also be enabled via both fine grained trap >> (HDFGxTR2_EL2.nSPMACCESSR_EL1) and coarse grained trap (MDCR_EL2.EnSPM). >> >> For all the registers that are being added via FEAT_FGT2 here in this >> patch, their corresponding CGT based path also needs to be enabled via >> corresponding CGT_MDCR_XXX groups. > Exactly. > >>> [...] >>> >>>>> Now, the main issues are that: >>>>> >>>>> - you're missing the coarse grained trapping for all the stuff you >>>>> have just added. It's not a huge amount of work, but you need, for >>>>> each register, to describe what traps apply to it. The fine grained >>>>> stuff is most, but not all of it. There should be enough of it >>>>> already to guide you through it. >>>> Coarse grained trapping for FEAT_FGT2 based fine grained registers ? >>> Not for FEAT_FGT2. For the registers that FEAT_FGT2 traps. Can you see >>> the difference? >> Understood, for example PMIAR_EL1 register which FEAT_FGT2 now traps via >> >> SR_FGT(SYS_PMIAR_EL1, HDFGRTR2, nPMIAR_EL1, 0), >> >> also needs to have corresponding coarse grained trap. >> >> SR_TRAP(SYS_PMIAR_EL1, CGT_MDCR_TPM), > Yup. > >> Similarly corresponding SR_TRAP() needs to be covered for all registers >> that are now being trapped with FEAT_FGT2. >> >> Example code snippet. >> >> ........ >> + SR_TRAP(SYS_SPMEVFILT2R_EL0(8), CGT_MDCR_EnSPM), >> + SR_TRAP(SYS_SPMEVFILT2R_EL0(9), CGT_MDCR_EnSPM), >> + SR_TRAP(SYS_SPMEVFILT2R_EL0(10), CGT_MDCR_EnSPM), >> + SR_TRAP(SYS_SPMEVFILT2R_EL0(11), CGT_MDCR_EnSPM), >> + SR_TRAP(SYS_SPMEVFILT2R_EL0(12), CGT_MDCR_EnSPM), >> + SR_TRAP(SYS_SPMEVFILT2R_EL0(13), CGT_MDCR_EnSPM), >> + SR_TRAP(SYS_SPMEVFILT2R_EL0(14), CGT_MDCR_EnSPM), >> + SR_TRAP(SYS_SPMEVFILT2R_EL0(15), CGT_MDCR_EnSPM), > I think it is a bit more complicated than just that. Again, look at > the pseudocode: > > elsif PSTATE.EL == EL1 then > if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then > UNDEFINED; > elsif HaveEL(EL3) && EL3SDDUndefPriority() && SPMACCESSR_EL3<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then > UNDEFINED; > elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0') then > AArch64.SystemAccessTrap(EL2, 0x18); > elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then > AArch64.SystemAccessTrap(EL2, 0x18); > elsif EL2Enabled() && SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then > AArch64.SystemAccessTrap(EL2, 0x18); > elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then > if EL3SDDUndef() then > UNDEFINED; > else > AArch64.SystemAccessTrap(EL3, 0x18); > elsif HaveEL(EL3) && SPMACCESSR_EL3<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then > if EL3SDDUndef() then > UNDEFINED; > else > AArch64.SystemAccessTrap(EL3, 0x18); > elsif !IsSPMUCounterImplemented(UInt(SPMSELR_EL0.SYSPMUSEL), (UInt(SPMSELR_EL0.BANK) * 16) + m) then > X[t, 64] = Zeros(64); > else > X[t, 64] = SPMEVFILT2R_EL0[UInt(SPMSELR_EL0.SYSPMUSEL), (UInt(SPMSELR_EL0.BANK) * 16) + m]; > > which shows that an SPMEVFILT2Rn_EL0 access from EL1 traps to EL2 if: > > - either HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0', (check) > - or MDCR_EL2.EnSPM == '0', (check) > - or SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' > > and that last condition requires some more handling as you need to > evaluate both SPMSELR_EL0.SYSPMUSEL and the corresponding field of > SPMACCESSR_EL2 to make a decision. It's not majorly complicated, but > it isn't solved by simply setting a static attribute. So IIUC you are suggesting to handle SYS_SPMEVFILT2R_EL0() registers via complex condition checks where the CGT_XXX can be directed to a function callback instead ? For example, something like the following (untested). --- a/arch/arm64/kvm/emulate-nested.c +++ b/arch/arm64/kvm/emulate-nested.c @@ -116,6 +116,7 @@ enum cgt_group_id { __COMPLEX_CONDITIONS__, CGT_CNTHCTL_EL1PCTEN = __COMPLEX_CONDITIONS__, CGT_CNTHCTL_EL1PTEN, + CGT_TEST, CGT_CPTR_TTA, @@ -486,6 +487,23 @@ static enum trap_behaviour check_cptr_tta(struct kvm_vcpu *vcpu) return BEHAVE_HANDLE_LOCALLY; } +static enum trap_behaviour check_test(struct kvm_vcpu *vcpu) +{ + u64 spmaccessr_el2 = __vcpu_sys_reg(vcpu, SPMACCESSR_EL2); + u64 spmselr_el2 = __vcpu_sys_reg(vcpu, SPMSELR_EL0); + u64 mdcr_el2 = __vcpu_sys_reg(vcpu, MDCR_EL2); + int syspmusel, spmaccessr_idx; + + if (!(mdcr_el2 & MDCR_EL2_EnSPM)) { + syspmusel = FIELD_GET(SPMSELR_EL0_SYSPMUSEL_MASK, spmselr_el2); + spmaccessr_idx = syspmusel * 2; + + if (((spmaccessr_el2 >> spmaccessr_idx) & 0x3) == 0x0) + return BEHAVE_FORWARD_ANY; + } + return BEHAVE_HANDLE_LOCALLY; +} + #define CCC(id, fn) \ [id - __COMPLEX_CONDITIONS__] = fn @@ -493,6 +511,7 @@ static const complex_condition_check ccc[] = { CCC(CGT_CNTHCTL_EL1PCTEN, check_cnthctl_el1pcten), CCC(CGT_CNTHCTL_EL1PTEN, check_cnthctl_el1pten), CCC(CGT_CPTR_TTA, check_cptr_tta), + CCC(CGT_TEST, check_test), }; /* @@ -1163,7 +1182,7 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = { SR_TRAP(SYS_SPMEVFILTR_EL0(14), CGT_MDCR_EnSPM), SR_TRAP(SYS_SPMEVFILTR_EL0(15), CGT_MDCR_EnSPM), - SR_TRAP(SYS_SPMEVFILT2R_EL0(0), CGT_MDCR_EnSPM), + SR_TRAP(SYS_SPMEVFILT2R_EL0(0), CGT_TEST), SR_TRAP(SYS_SPMEVFILT2R_EL0(1), CGT_MDCR_EnSPM), SR_TRAP(SYS_SPMEVFILT2R_EL0(2), CGT_MDCR_EnSPM), SR_TRAP(SYS_SPMEVFILT2R_EL0(3), CGT_MDCR_EnSPM),
On 2024-08-13 07:53, Anshuman Khandual wrote: > On 8/4/24 16:35, Marc Zyngier wrote: >> which shows that an SPMEVFILT2Rn_EL0 access from EL1 traps to EL2 if: >> >> - either HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0', (check) >> - or MDCR_EL2.EnSPM == '0', (check) >> - or SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + >> 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' >> >> and that last condition requires some more handling as you need to >> evaluate both SPMSELR_EL0.SYSPMUSEL and the corresponding field of >> SPMACCESSR_EL2 to make a decision. It's not majorly complicated, but >> it isn't solved by simply setting a static attribute. > > So IIUC you are suggesting to handle SYS_SPMEVFILT2R_EL0() registers > via > complex condition checks where the CGT_XXX can be directed to a > function > callback instead ? For example, something like the following > (untested). [...] Something like that, yes. M.
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index b2adc2c6c82a..b3fb368bcadb 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -354,6 +354,14 @@ #define __HFGRTR_EL2_MASK GENMASK(49, 0) #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK) +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0 +#define __HDFGRTR2_EL2_MASK (GENMASK(22, 22) | GENMASK(20, 0)) +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK) + +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0 +#define __HDFGWTR2_EL2_MASK (GENMASK(22, 19) | GENMASK(16, 7) | GENMASK(5, 0)) +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK) + /* * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any * future additions, define __HFGWTR* macros relative to __HFGRTR* ones. diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 7b44e96e7270..d6fbd6ebc32d 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -239,6 +239,8 @@ enum fgt_group_id { HDFGWTR_GROUP = HDFGRTR_GROUP, HFGITR_GROUP, HAFGRTR_GROUP, + HDFGRTR2_GROUP, + HDFGWTR2_GROUP = HDFGRTR2_GROUP, /* Must be last */ __NR_FGT_GROUP_IDS__ diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c index 54090967a335..bc5ea1e60a0a 100644 --- a/arch/arm64/kvm/emulate-nested.c +++ b/arch/arm64/kvm/emulate-nested.c @@ -1724,6 +1724,9 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = { SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1), SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1), SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1), + + /* HDFGRTR2_EL2 */ + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 1), }; static union trap_config get_trap_config(u32 sysreg) @@ -1979,6 +1982,10 @@ static bool check_fgt_bit(struct kvm *kvm, bool is_read, sr = is_read ? HDFGRTR_EL2 : HDFGWTR_EL2; break; + case HDFGRTR2_GROUP: + sr = is_read ? HDFGRTR2_EL2 : HDFGWTR2_EL2; + break; + case HAFGRTR_GROUP: sr = HAFGRTR_EL2; break; @@ -2053,6 +2060,13 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) val = __vcpu_sys_reg(vcpu, HDFGWTR_EL2); break; + case HDFGRTR2_GROUP: + if (is_read) + val = __vcpu_sys_reg(vcpu, HDFGRTR2_EL2); + else + val = __vcpu_sys_reg(vcpu, HDFGWTR2_EL2); + break; + case HAFGRTR_GROUP: val = __vcpu_sys_reg(vcpu, HAFGRTR_EL2); break; diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index 0c4de44534b7..b5944aa6d9c8 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -89,6 +89,10 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) case HDFGWTR_EL2: \ id = HDFGRTR_GROUP; \ break; \ + case HDFGRTR2_EL2: \ + case HDFGWTR2_EL2: \ + id = HDFGRTR2_GROUP; \ + break; \ case HAFGRTR_EL2: \ id = HAFGRTR_GROUP; \ break; \ @@ -160,6 +164,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) CHECK_FGT_MASKS(HDFGWTR_EL2); CHECK_FGT_MASKS(HAFGRTR_EL2); CHECK_FGT_MASKS(HCRX_EL2); + CHECK_FGT_MASKS(HDFGRTR2_EL2); + CHECK_FGT_MASKS(HDFGWTR2_EL2); if (!cpus_have_final_cap(ARM64_HAS_FGT)) return; @@ -171,6 +177,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu) update_fgt_traps(hctxt, vcpu, kvm, HFGITR_EL2); update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR_EL2); update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR_EL2); + update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR2_EL2); + update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR2_EL2); if (cpu_has_amu()) update_fgt_traps(hctxt, vcpu, kvm, HAFGRTR_EL2); @@ -200,6 +208,8 @@ static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu) __deactivate_fgt(hctxt, vcpu, kvm, HFGITR_EL2); __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR_EL2); __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR_EL2); + __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR2_EL2); + __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR2_EL2); if (cpu_has_amu()) __deactivate_fgt(hctxt, vcpu, kvm, HAFGRTR_EL2); diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c index bae8536cbf00..ebe4e3972fed 100644 --- a/arch/arm64/kvm/nested.c +++ b/arch/arm64/kvm/nested.c @@ -384,6 +384,42 @@ int kvm_init_nv_sysregs(struct kvm *kvm) res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1; set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1); + /* HDFG[RW]TR2_EL2 */ + res0 = res1 = 0; + + /* FEAT_TRBE_MPAM is not exposed to the guest */ + res0 |= HDFGRTR2_EL2_nTRBMPAM_EL1; + + /* FEAT_SPE_FDS is not exposed to the guest */ + res0 |= HDFGRTR2_EL2_nPMSDSFR_EL1; + + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) + res0 |= HDFGRTR2_EL2_nMDSTEPOP_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) + res0 |= HDFGRTR2_EL2_nTRCITECR_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) + res0 |= (HDFGRTR2_EL2_nSPMDEVAFF_EL1 | HDFGRTR2_EL2_nSPMID | + HDFGRTR2_EL2_nSPMSCR_EL1 | HDFGRTR2_EL2_nSPMACCESSR_EL1 | + HDFGRTR2_EL2_nSPMCR_EL0 | HDFGRTR2_EL2_nSPMOVS | + HDFGRTR2_EL2_nSPMINTEN | HDFGRTR2_EL2_nSPMSELR_EL0 | + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | HDFGRTR2_EL2_nSPMEVCNTRn_EL0 | + HDFGRTR2_EL2_nPMSSCR_EL1 | HDFGRTR2_EL2_nPMSSDATA); + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) + res0 |= HDFGRTR2_EL2_nMDSELR_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) + res0 |= HDFGRTR2_EL2_nPMUACR_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) + res0 |= HDFGRTR2_EL2_nPMICFILTR_EL0; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) + res0 |= HDFGRTR2_EL2_nPMICNTR_EL0; + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) + res0 |= HDFGRTR2_EL2_nPMIAR_EL1; + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) + res0 |= HDFGRTR2_EL2_nPMECR_EL1; + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1); + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1); + /* Reuse the bits from the read-side and add the write-specific stuff */ if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP)) res0 |= (HDFGWTR_EL2_PMCR_EL0 | HDFGWTR_EL2_PMSWINC_EL0); diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index f921af014d0c..8029f408855d 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 | HAFGRTR_EL2_RES1); + /* FEAT_TRBE_MPAM is not exposed to the guest */ + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1); + + /* FEAT_SPE_FDS is not exposed to the guest */ + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMSDSFR_EL1); + + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSTEPOP_EL1); + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRCITECR_EL1); + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nSPMDEVAFF_EL1 | + HDFGRTR2_EL2_nSPMID | + HDFGRTR2_EL2_nSPMSCR_EL1 | + HDFGRTR2_EL2_nSPMACCESSR_EL1 | + HDFGRTR2_EL2_nSPMCR_EL0 | + HDFGRTR2_EL2_nSPMOVS | + HDFGRTR2_EL2_nSPMINTEN | + HDFGRTR2_EL2_nSPMSELR_EL0 | + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | + HDFGRTR2_EL2_nSPMEVCNTRn_EL0 | + HDFGRTR2_EL2_nPMSSCR_EL1 | + HDFGRTR2_EL2_nPMSSDATA); + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSELR_EL1); + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMUACR_EL1); + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMICFILTR_EL0); + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMICNTR_EL0); + + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMIAR_EL1); + + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) && + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP)) + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMECR_EL1); + set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags); out: mutex_unlock(&kvm->arch.config_lock);
This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers. Cc: Marc Zyngier <maz@kernel.org> Cc: Oliver Upton <oliver.upton@linux.dev> Cc: James Morse <james.morse@arm.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: kvmarm@lists.linux.dev Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- arch/arm64/include/asm/kvm_arm.h | 8 +++++ arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kvm/emulate-nested.c | 14 ++++++++ arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++++++ arch/arm64/kvm/nested.c | 36 ++++++++++++++++++++ arch/arm64/kvm/sys_regs.c | 45 +++++++++++++++++++++++++ 6 files changed, 115 insertions(+)