[PATCH] KVM: arm64: nv: Fix RESx behaviour of disabled FGTs with negative polarity

[PATCH] KVM: arm64: nv: Fix RESx behaviour of disabled FGTs with negative polarity

[Marc Zyngier]

The Fine Grained Trap extension is pretty messy as it doesn't
consistently use the same polarity for all trap bits. A bunch
of them, added later in the life of the architecture, have a
*negative* priority.

So if these bits are disabled, they must be RES1 and not RES0.
But that's not what the code implements, making the traps for
these negative trap bits being always on instead of disabled.

Fix the relevant bits, and stick a brown paper bag on my head
for the rest of the day...

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/nested.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index bae8536cbf00..0acb60273482 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -328,21 +328,21 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 			 HFGxTR_EL2_ERXPFGF_EL1 | HFGxTR_EL2_ERXPFGCTL_EL1 |
 			 HFGxTR_EL2_ERXPFGCDN_EL1 | HFGxTR_EL2_ERXADDR_EL1);
 	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, LS64, LS64_ACCDATA))
-		res0 |= HFGxTR_EL2_nACCDATA_EL1;
+		res1 |= HFGxTR_EL2_nACCDATA_EL1;
 	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP))
-		res0 |= (HFGxTR_EL2_nGCS_EL0 | HFGxTR_EL2_nGCS_EL1);
+		res1 |= (HFGxTR_EL2_nGCS_EL0 | HFGxTR_EL2_nGCS_EL1);
 	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, SME, IMP))
-		res0 |= (HFGxTR_EL2_nSMPRI_EL1 | HFGxTR_EL2_nTPIDR2_EL0);
+		res1 |= (HFGxTR_EL2_nSMPRI_EL1 | HFGxTR_EL2_nTPIDR2_EL0);
 	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, THE, IMP))
-		res0 |= HFGxTR_EL2_nRCWMASK_EL1;
+		res1 |= HFGxTR_EL2_nRCWMASK_EL1;
 	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1PIE, IMP))
-		res0 |= (HFGxTR_EL2_nPIRE0_EL1 | HFGxTR_EL2_nPIR_EL1);
+		res1 |= (HFGxTR_EL2_nPIRE0_EL1 | HFGxTR_EL2_nPIR_EL1);
 	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1POE, IMP))
-		res0 |= (HFGxTR_EL2_nPOR_EL0 | HFGxTR_EL2_nPOR_EL1);
+		res1 |= (HFGxTR_EL2_nPOR_EL0 | HFGxTR_EL2_nPOR_EL1);
 	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S2POE, IMP))
-		res0 |= HFGxTR_EL2_nS2POR_EL1;
+		res1 |= HFGxTR_EL2_nS2POR_EL1;
 	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, AIE, IMP))
-		res0 |= (HFGxTR_EL2_nMAIR2_EL1 | HFGxTR_EL2_nAMAIR2_EL1);
+		res1 |= (HFGxTR_EL2_nMAIR2_EL1 | HFGxTR_EL2_nAMAIR2_EL1);
 	set_sysreg_masks(kvm, HFGRTR_EL2, res0 | __HFGRTR_EL2_RES0, res1);
 	set_sysreg_masks(kvm, HFGWTR_EL2, res0 | __HFGWTR_EL2_RES0, res1);
 
@@ -378,10 +378,10 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 			 HDFGRTR_EL2_TRBPTR_EL1 | HDFGRTR_EL2_TRBSR_EL1 |
 			 HDFGRTR_EL2_TRBTRG_EL1);
 	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP))
-		res0 |= (HDFGRTR_EL2_nBRBIDR | HDFGRTR_EL2_nBRBCTL |
+		res1 |= (HDFGRTR_EL2_nBRBIDR | HDFGRTR_EL2_nBRBCTL |
 			 HDFGRTR_EL2_nBRBDATA);
 	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P2))
-		res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1;
+		res1 |= HDFGRTR_EL2_nPMSNEVFR_EL1;
 	set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1);
 
 	/* Reuse the bits from the read-side and add the write-specific stuff */
@@ -417,9 +417,9 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 		res0 |= (HFGITR_EL2_CFPRCTX | HFGITR_EL2_DVPRCTX |
 			 HFGITR_EL2_CPPRCTX);
 	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP))
-		res0 |= (HFGITR_EL2_nBRBINJ | HFGITR_EL2_nBRBIALL);
+		res1 |= (HFGITR_EL2_nBRBINJ | HFGITR_EL2_nBRBIALL);
 	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP))
-		res0 |= (HFGITR_EL2_nGCSPUSHM_EL1 | HFGITR_EL2_nGCSSTR_EL1 |
+		res1 |= (HFGITR_EL2_nGCSPUSHM_EL1 | HFGITR_EL2_nGCSSTR_EL1 |
 			 HFGITR_EL2_nGCSEPP);
 	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, SPECRES, COSP_RCTX))
 		res0 |= HFGITR_EL2_COSPRCTX;
-- 
2.39.2

Re: [PATCH] KVM: arm64: nv: Fix RESx behaviour of disabled FGTs with negative polarity

[Oliver Upton]

On Fri, 14 Jun 2024 13:58:58 +0100, Marc Zyngier wrote:
> The Fine Grained Trap extension is pretty messy as it doesn't
> consistently use the same polarity for all trap bits. A bunch
> of them, added later in the life of the architecture, have a
> *negative* priority.
> 
> So if these bits are disabled, they must be RES1 and not RES0.
> But that's not what the code implements, making the traps for
> these negative trap bits being always on instead of disabled.
> 
> [...]

Applied to kvmarm/next, thanks!

[1/1] KVM: arm64: nv: Fix RESx behaviour of disabled FGTs with negative polarity
      https://git.kernel.org/kvmarm/kvmarm/c/eb9d53d4a949

--
Best,
Oliver

Re: [PATCH] KVM: arm64: nv: Fix RESx behaviour of disabled FGTs with negative polarity

[Marc Zyngier]

On Fri, 14 Jun 2024 21:24:51 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Fri, 14 Jun 2024 13:58:58 +0100, Marc Zyngier wrote:
> > The Fine Grained Trap extension is pretty messy as it doesn't
> > consistently use the same polarity for all trap bits. A bunch
> > of them, added later in the life of the architecture, have a
> > *negative* priority.
> > 
> > So if these bits are disabled, they must be RES1 and not RES0.
> > But that's not what the code implements, making the traps for
> > these negative trap bits being always on instead of disabled.
> > 
> > [...]
> 
> Applied to kvmarm/next, thanks!
> 
> [1/1] KVM: arm64: nv: Fix RESx behaviour of disabled FGTs with negative polarity
>       https://git.kernel.org/kvmarm/kvmarm/c/eb9d53d4a949

[+ Anshuman, as I've pointed him to this patch in the past]

OK, I think I have come to my senses, and came to the conclusion that:

- I am officially losing the plot (blame the political climate)

- this "fix" is total b*ll*cks and must be dropped/reverted

Let remember how this whole thing works. A "negative" trap bit has two
essential properties:

- it is writable

- it has an effect when set to 0

If the bit isn't implemented, it is RES0. Only RES0. Not RES1, which
this patch enforces. None of the FGT bits are ever RES1. So at least
on this front, this patch is broken and results in observable nonsense
on the guest side.

But there is more! We are already capable of distinguishing a bit that
traps because it is set to 0 from a bit that is RES0. check_fgt_bit()
already has all the logic, which is evaluated on any trap.

So we already have the proper filtering in place (a RES0 bit won't
result in a trap forwarded to a nested guest), the original code was
correct, and forcing FGT bits to RES1 is just a stupid regression.

Oliver, can you please drop or revert this patch from the kvmarm/next
branch please?

Thanks and sorry for the noise.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: nv: Fix RESx behaviour of disabled FGTs with negative polarity

[Oliver Upton]

Hey,

On Fri, Jul 05, 2024 at 01:08:51PM +0100, Marc Zyngier wrote:
> OK, I think I have come to my senses, and came to the conclusion that:
> 
> - I am officially losing the plot (blame the political climate)
> 
> - this "fix" is total b*ll*cks and must be dropped/reverted

Ouch, and I let it sail straight by me.

> Oliver, can you please drop or revert this patch from the kvmarm/next
> branch please?
> 
> Thanks and sorry for the noise.

Done, and sorry I didn't spot this in review.

commit:	cb52b5c8b81bfbc34df13537d82cd1849725d6c7

-- 
Thanks,
Oliver