Re: [PATCH v3 00/42] KVM: arm64: Revamp Fine Grained Trap handling

[Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx>]

On 4/29/2025 1:04 PM, Marc Zyngier wrote:

> > I am trying nv-next branch and I believe these FGT related changes are
> > merged. With this, selftest arm64/set_id_regs is failing. From initial
> > debug it seems, the register access of SYS_CTR_EL0, SYS_MIDR_EL1,
> > SYS_REVIDR_EL1 and SYS_AIDR_EL1 from guest_code is resulting in trap
> > to EL2 (HCR_ID1,ID2 are set) and is getting forwarded back to EL1,
> > since EL1 sync handler is not installed in the test code, resulting in
> > hang(endless guest_exit/entry).
>
> Let's start by calling bullshit on the test itself:
>
> root@semi-fraudulent:/home/maz# grep AA64PFR0 /sys/kernel/debug/kvm/2008-4/idregs
>   SYS_ID_AA64PFR0_EL1:	0000000020110000
>
> It basically disable anything 64bit at EL{0,1,2,3]. Frankly, all these
> tests are pure garbage. I'm baffled that anyone expects this crap to
> give any meaningful result.
>
> > It is due to function "triage_sysreg_trap" is returning true.
> >
> > When guest_code is in EL1 (default case) it is due to return in below if.
> >
> >   if (tc.fgt != __NO_FGT_GROUP__ &&
> >              (vcpu->kvm->arch.fgu[tc.fgt] & BIT(tc.bit))) {
> >                  kvm_inject_undefined(vcpu);
> >                  return true;
> >          }
>
> That explains why we end-up here. The 64bit ISA is "disabled", a bunch
> of trap bits are advertised as depending on it, so the corresponding
> FGU bits are set to "emulate" the requested behaviour.

OK, was comparing fgu for this test case and VM boot.
For this test, all HFGRTR bits were set in fgu.
thanks, I did not notice that guest was disabled for AArch64.

> Works as intended, and this proves once more that what we call testing
> is just horseshit.
>
> In retrospect, we should do a few things:
>
> - Prevent writes to ID_AA64PFR0_EL1 disabling the 64bit ISA, breaking
>    this stupid test for good.
>
> - Flag all the FGT bits depending on FEAT_AA64EL1 as NEVER_FGU,
>    because that shouldn't happen, by construction (there is no
>    architecture revision where these sysregs are UNDEFined).

Yes we should.

> - Mark all these test as unmaintained and deprecated, recognising that
>    they are utterly pointless (optional).

Just wondering, should I continue to modify this test to run in vEL2?

> Full patch below.
>
> 	M.
>
> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> index d4e1218b004dd..666070d4ccd7f 100644
> --- a/arch/arm64/kvm/config.c
> +++ b/arch/arm64/kvm/config.c
> @@ -295,34 +295,34 @@ static const struct reg_bits_to_feat_map hfgrtr_feat_map[] = {
>   		   HFGRTR_EL2_APDBKey		|
>   		   HFGRTR_EL2_APDAKey,
>   		   feat_pauth),
> -	NEEDS_FEAT(HFGRTR_EL2_VBAR_EL1		|
> -		   HFGRTR_EL2_TTBR1_EL1		|
> -		   HFGRTR_EL2_TTBR0_EL1		|
> -		   HFGRTR_EL2_TPIDR_EL0		|
> -		   HFGRTR_EL2_TPIDRRO_EL0	|
> -		   HFGRTR_EL2_TPIDR_EL1		|
> -		   HFGRTR_EL2_TCR_EL1		|
> -		   HFGRTR_EL2_SCTLR_EL1		|
> -		   HFGRTR_EL2_REVIDR_EL1	|
> -		   HFGRTR_EL2_PAR_EL1		|
> -		   HFGRTR_EL2_MPIDR_EL1		|
> -		   HFGRTR_EL2_MIDR_EL1		|
> -		   HFGRTR_EL2_MAIR_EL1		|
> -		   HFGRTR_EL2_ISR_EL1		|
> -		   HFGRTR_EL2_FAR_EL1		|
> -		   HFGRTR_EL2_ESR_EL1		|
> -		   HFGRTR_EL2_DCZID_EL0		|
> -		   HFGRTR_EL2_CTR_EL0		|
> -		   HFGRTR_EL2_CSSELR_EL1	|
> -		   HFGRTR_EL2_CPACR_EL1		|
> -		   HFGRTR_EL2_CONTEXTIDR_EL1	|
> -		   HFGRTR_EL2_CLIDR_EL1		|
> -		   HFGRTR_EL2_CCSIDR_EL1	|
> -		   HFGRTR_EL2_AMAIR_EL1		|
> -		   HFGRTR_EL2_AIDR_EL1		|
> -		   HFGRTR_EL2_AFSR1_EL1		|
> -		   HFGRTR_EL2_AFSR0_EL1,
> -		   FEAT_AA64EL1),
> +	NEEDS_FEAT_FLAG(HFGRTR_EL2_VBAR_EL1	|
> +			HFGRTR_EL2_TTBR1_EL1	|
> +			HFGRTR_EL2_TTBR0_EL1	|
> +			HFGRTR_EL2_TPIDR_EL0	|
> +			HFGRTR_EL2_TPIDRRO_EL0	|
> +			HFGRTR_EL2_TPIDR_EL1	|
> +			HFGRTR_EL2_TCR_EL1	|
> +			HFGRTR_EL2_SCTLR_EL1	|
> +			HFGRTR_EL2_REVIDR_EL1	|
> +			HFGRTR_EL2_PAR_EL1	|
> +			HFGRTR_EL2_MPIDR_EL1	|
> +			HFGRTR_EL2_MIDR_EL1	|
> +			HFGRTR_EL2_MAIR_EL1	|
> +			HFGRTR_EL2_ISR_EL1	|
> +			HFGRTR_EL2_FAR_EL1	|
> +			HFGRTR_EL2_ESR_EL1	|
> +			HFGRTR_EL2_DCZID_EL0	|
> +			HFGRTR_EL2_CTR_EL0	|
> +			HFGRTR_EL2_CSSELR_EL1	|
> +			HFGRTR_EL2_CPACR_EL1	|
> +			HFGRTR_EL2_CONTEXTIDR_EL1|
> +			HFGRTR_EL2_CLIDR_EL1	|
> +			HFGRTR_EL2_CCSIDR_EL1	|
> +			HFGRTR_EL2_AMAIR_EL1	|
> +			HFGRTR_EL2_AIDR_EL1	|
> +			HFGRTR_EL2_AFSR1_EL1	|
> +			HFGRTR_EL2_AFSR0_EL1,
> +			NEVER_FGU, FEAT_AA64EL1),
>   };
>   
>   static const struct reg_bits_to_feat_map hfgwtr_feat_map[] = {
> @@ -368,25 +368,25 @@ static const struct reg_bits_to_feat_map hfgwtr_feat_map[] = {
>   		   HFGWTR_EL2_APDBKey		|
>   		   HFGWTR_EL2_APDAKey,
>   		   feat_pauth),
> -	NEEDS_FEAT(HFGWTR_EL2_VBAR_EL1		|
> -		   HFGWTR_EL2_TTBR1_EL1		|
> -		   HFGWTR_EL2_TTBR0_EL1		|
> -		   HFGWTR_EL2_TPIDR_EL0		|
> -		   HFGWTR_EL2_TPIDRRO_EL0	|
> -		   HFGWTR_EL2_TPIDR_EL1		|
> -		   HFGWTR_EL2_TCR_EL1		|
> -		   HFGWTR_EL2_SCTLR_EL1		|
> -		   HFGWTR_EL2_PAR_EL1		|
> -		   HFGWTR_EL2_MAIR_EL1		|
> -		   HFGWTR_EL2_FAR_EL1		|
> -		   HFGWTR_EL2_ESR_EL1		|
> -		   HFGWTR_EL2_CSSELR_EL1	|
> -		   HFGWTR_EL2_CPACR_EL1		|
> -		   HFGWTR_EL2_CONTEXTIDR_EL1	|
> -		   HFGWTR_EL2_AMAIR_EL1		|
> -		   HFGWTR_EL2_AFSR1_EL1		|
> -		   HFGWTR_EL2_AFSR0_EL1,
> -		   FEAT_AA64EL1),
> +	NEEDS_FEAT_FLAG(HFGWTR_EL2_VBAR_EL1	|
> +			HFGWTR_EL2_TTBR1_EL1	|
> +			HFGWTR_EL2_TTBR0_EL1	|
> +			HFGWTR_EL2_TPIDR_EL0	|
> +			HFGWTR_EL2_TPIDRRO_EL0	|
> +			HFGWTR_EL2_TPIDR_EL1	|
> +			HFGWTR_EL2_TCR_EL1	|
> +			HFGWTR_EL2_SCTLR_EL1	|
> +			HFGWTR_EL2_PAR_EL1	|
> +			HFGWTR_EL2_MAIR_EL1	|
> +			HFGWTR_EL2_FAR_EL1	|
> +			HFGWTR_EL2_ESR_EL1	|
> +			HFGWTR_EL2_CSSELR_EL1	|
> +			HFGWTR_EL2_CPACR_EL1	|
> +			HFGWTR_EL2_CONTEXTIDR_EL1|
> +			HFGWTR_EL2_AMAIR_EL1	|
> +			HFGWTR_EL2_AFSR1_EL1	|
> +			HFGWTR_EL2_AFSR0_EL1,
> +			NEVER_FGU, FEAT_AA64EL1),
>   };
>   
>   static const struct reg_bits_to_feat_map hdfgrtr_feat_map[] = {
> @@ -443,17 +443,17 @@ static const struct reg_bits_to_feat_map hdfgrtr_feat_map[] = {
>   		   FEAT_TRBE),
>   	NEEDS_FEAT_FLAG(HDFGRTR_EL2_OSDLR_EL1, NEVER_FGU,
>   			FEAT_DoubleLock),
> -	NEEDS_FEAT(HDFGRTR_EL2_OSECCR_EL1	|
> -		   HDFGRTR_EL2_OSLSR_EL1	|
> -		   HDFGRTR_EL2_DBGPRCR_EL1	|
> -		   HDFGRTR_EL2_DBGAUTHSTATUS_EL1|
> -		   HDFGRTR_EL2_DBGCLAIM		|
> -		   HDFGRTR_EL2_MDSCR_EL1	|
> -		   HDFGRTR_EL2_DBGWVRn_EL1	|
> -		   HDFGRTR_EL2_DBGWCRn_EL1	|
> -		   HDFGRTR_EL2_DBGBVRn_EL1	|
> -		   HDFGRTR_EL2_DBGBCRn_EL1,
> -		   FEAT_AA64EL1)
> +	NEEDS_FEAT_FLAG(HDFGRTR_EL2_OSECCR_EL1	|
> +			HDFGRTR_EL2_OSLSR_EL1	|
> +			HDFGRTR_EL2_DBGPRCR_EL1	|
> +			HDFGRTR_EL2_DBGAUTHSTATUS_EL1|
> +			HDFGRTR_EL2_DBGCLAIM	|
> +			HDFGRTR_EL2_MDSCR_EL1	|
> +			HDFGRTR_EL2_DBGWVRn_EL1	|
> +			HDFGRTR_EL2_DBGWCRn_EL1	|
> +			HDFGRTR_EL2_DBGBVRn_EL1	|
> +			HDFGRTR_EL2_DBGBCRn_EL1,
> +			NEVER_FGU, FEAT_AA64EL1)
>   };
>   
>   static const struct reg_bits_to_feat_map hdfgwtr_feat_map[] = {
> @@ -503,16 +503,16 @@ static const struct reg_bits_to_feat_map hdfgwtr_feat_map[] = {
>   		   FEAT_TRBE),
>   	NEEDS_FEAT_FLAG(HDFGWTR_EL2_OSDLR_EL1,
>   			NEVER_FGU, FEAT_DoubleLock),
> -	NEEDS_FEAT(HDFGWTR_EL2_OSECCR_EL1	|
> -		   HDFGWTR_EL2_OSLAR_EL1	|
> -		   HDFGWTR_EL2_DBGPRCR_EL1	|
> -		   HDFGWTR_EL2_DBGCLAIM		|
> -		   HDFGWTR_EL2_MDSCR_EL1	|
> -		   HDFGWTR_EL2_DBGWVRn_EL1	|
> -		   HDFGWTR_EL2_DBGWCRn_EL1	|
> -		   HDFGWTR_EL2_DBGBVRn_EL1	|
> -		   HDFGWTR_EL2_DBGBCRn_EL1,
> -		   FEAT_AA64EL1),
> +	NEEDS_FEAT_FLAG(HDFGWTR_EL2_OSECCR_EL1	|
> +			HDFGWTR_EL2_OSLAR_EL1	|
> +			HDFGWTR_EL2_DBGPRCR_EL1	|
> +			HDFGWTR_EL2_DBGCLAIM	|
> +			HDFGWTR_EL2_MDSCR_EL1	|
> +			HDFGWTR_EL2_DBGWVRn_EL1	|
> +			HDFGWTR_EL2_DBGWCRn_EL1	|
> +			HDFGWTR_EL2_DBGBVRn_EL1	|
> +			HDFGWTR_EL2_DBGBCRn_EL1,
> +			NEVER_FGU, FEAT_AA64EL1),
>   	NEEDS_FEAT(HDFGWTR_EL2_TRFCR_EL1, FEAT_TRF),
>   };
>   
> @@ -556,38 +556,38 @@ static const struct reg_bits_to_feat_map hfgitr_feat_map[] = {
>   		   HFGITR_EL2_ATS1E1RP,
>   		   FEAT_PAN2),
>   	NEEDS_FEAT(HFGITR_EL2_DCCVADP, FEAT_DPB2),
> -	NEEDS_FEAT(HFGITR_EL2_DCCVAC		|
> -		   HFGITR_EL2_SVC_EL1		|
> -		   HFGITR_EL2_SVC_EL0		|
> -		   HFGITR_EL2_ERET		|
> -		   HFGITR_EL2_TLBIVAALE1	|
> -		   HFGITR_EL2_TLBIVALE1		|
> -		   HFGITR_EL2_TLBIVAAE1		|
> -		   HFGITR_EL2_TLBIASIDE1	|
> -		   HFGITR_EL2_TLBIVAE1		|
> -		   HFGITR_EL2_TLBIVMALLE1	|
> -		   HFGITR_EL2_TLBIVAALE1IS	|
> -		   HFGITR_EL2_TLBIVALE1IS	|
> -		   HFGITR_EL2_TLBIVAAE1IS	|
> -		   HFGITR_EL2_TLBIASIDE1IS	|
> -		   HFGITR_EL2_TLBIVAE1IS	|
> -		   HFGITR_EL2_TLBIVMALLE1IS	|
> -		   HFGITR_EL2_ATS1E0W		|
> -		   HFGITR_EL2_ATS1E0R		|
> -		   HFGITR_EL2_ATS1E1W		|
> -		   HFGITR_EL2_ATS1E1R		|
> -		   HFGITR_EL2_DCZVA		|
> -		   HFGITR_EL2_DCCIVAC		|
> -		   HFGITR_EL2_DCCVAP		|
> -		   HFGITR_EL2_DCCVAU		|
> -		   HFGITR_EL2_DCCISW		|
> -		   HFGITR_EL2_DCCSW		|
> -		   HFGITR_EL2_DCISW		|
> -		   HFGITR_EL2_DCIVAC		|
> -		   HFGITR_EL2_ICIVAU		|
> -		   HFGITR_EL2_ICIALLU		|
> -		   HFGITR_EL2_ICIALLUIS,
> -		   FEAT_AA64EL1),
> +	NEEDS_FEAT_FLAG(HFGITR_EL2_DCCVAC	|
> +			HFGITR_EL2_SVC_EL1	|
> +			HFGITR_EL2_SVC_EL0	|
> +			HFGITR_EL2_ERET		|
> +			HFGITR_EL2_TLBIVAALE1	|
> +			HFGITR_EL2_TLBIVALE1	|
> +			HFGITR_EL2_TLBIVAAE1	|
> +			HFGITR_EL2_TLBIASIDE1	|
> +			HFGITR_EL2_TLBIVAE1	|
> +			HFGITR_EL2_TLBIVMALLE1	|
> +			HFGITR_EL2_TLBIVAALE1IS	|
> +			HFGITR_EL2_TLBIVALE1IS	|
> +			HFGITR_EL2_TLBIVAAE1IS	|
> +			HFGITR_EL2_TLBIASIDE1IS	|
> +			HFGITR_EL2_TLBIVAE1IS	|
> +			HFGITR_EL2_TLBIVMALLE1IS|
> +			HFGITR_EL2_ATS1E0W	|
> +			HFGITR_EL2_ATS1E0R	|
> +			HFGITR_EL2_ATS1E1W	|
> +			HFGITR_EL2_ATS1E1R	|
> +			HFGITR_EL2_DCZVA	|
> +			HFGITR_EL2_DCCIVAC	|
> +			HFGITR_EL2_DCCVAP	|
> +			HFGITR_EL2_DCCVAU	|
> +			HFGITR_EL2_DCCISW	|
> +			HFGITR_EL2_DCCSW	|
> +			HFGITR_EL2_DCISW	|
> +			HFGITR_EL2_DCIVAC	|
> +			HFGITR_EL2_ICIVAU	|
> +			HFGITR_EL2_ICIALLU	|
> +			HFGITR_EL2_ICIALLUIS,
> +			NEVER_FGU, FEAT_AA64EL1),
>   };
>   
>   static const struct reg_bits_to_feat_map hafgrtr_feat_map[] = {
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 157de0ace6e7e..28dc778d0d9bb 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1946,6 +1946,12 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>   	if ((hw_val & mpam_mask) == (user_val & mpam_mask))
>   		user_val &= ~ID_AA64PFR0_EL1_MPAM_MASK;
>   
> +	/* Fail the guest's request to disable the AA64 ISA at EL{0,1,2} */
> +	if (!FIELD_GET(ID_AA64PFR0_EL1_EL0, user_val) ||
> +	    !FIELD_GET(ID_AA64PFR0_EL1_EL1, user_val) ||
> +	    (vcpu_has_nv(vcpu) && !FIELD_GET(ID_AA64PFR0_EL1_EL2, user_val)))
> +		return -EINVAL;
> +
>   	return set_id_reg(vcpu, rd, user_val);
>   }
>   
> diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> index 322b9d3b01255..57708de2075df 100644
> --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c
> +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> @@ -129,10 +129,10 @@ static const struct reg_ftr_bits ftr_id_aa64pfr0_el1[] = {
>   	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, DIT, 0),
>   	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, SEL2, 0),
>   	REG_FTR_BITS(FTR_EXACT, ID_AA64PFR0_EL1, GIC, 0),
> -	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL3, 0),
> -	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL2, 0),
> -	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL1, 0),
> -	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL0, 0),
> +	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL3, 1),
> +	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL2, 1),
> +	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL1, 1),
> +	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL0, 1),
>   	REG_FTR_END,
>   };
>   
This diff fixes the hang seen while running this test(test ran gracefully).
Tried to run this test in vEL2 and it passing for majority of the registers and failing for the few, looking in to it.

--
Thanks,
Gk