Re: [RFC 10/10] KVM: arm64: nv: Add new HDFGRTR2_GROUP & HDFGRTR2_GROUP based FGU handling
From: Anshuman Khandual <hidden>
Date: 2024-08-13 06:53:37
Also in:
kvmarm, lkml
On 8/4/24 16:35, Marc Zyngier wrote:
On Sat, 03 Aug 2024 11:38:11 +0100, Anshuman Khandual [off-list ref] wrote:quoted
On 8/2/24 16:29, Marc Zyngier wrote:quoted
On Fri, 02 Aug 2024 10:25:44 +0100, Anshuman Khandual [off-list ref] wrote:quoted
On 8/1/24 21:33, Marc Zyngier wrote:quoted
On Thu, 01 Aug 2024 11:46:22 +0100, Anshuman Khandual [off-list ref] wrote:[...]quoted
quoted
quoted
+ 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.quoted
quoted
[...]quoted
quoted
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.quoted
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),