[PATCH] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode

[PATCH] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode

[Marc Zyngier]

When running in hVHE mode, EL1 accesses are performed with the EL12
accessor, as we run with HCR_EL2.E2H=1.

Unfortunately, both PMSCR_EL1 and TRFCR_EL1 are used with the
EL1 accessor, meaning that we actually affect the EL2 state. Duh.

Switch to using the {read,write}_sysreg_el1() helpers that will do
the right thing in all circumstances.

Note that the 'Fixes:' tag doesn't represent the point where the bug
was introduced (there is no such point), but the first practical point
where the hVHE feature is usable.

Cc: James Clark <james.clark@arm.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Fixes: 38cba55008e5 ("KVM: arm64: Force HCR_E2H in guest context when ARM64_KVM_HVHE is set")
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/hyp/nvhe/debug-sr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
index 8103f8c695b4..6d57b5c86a91 100644
--- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
+++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
@@ -31,8 +31,8 @@ static void __debug_save_spe(u64 *pmscr_el1)
 		return;
 
 	/* Yes; save the control register and disable data generation */
-	*pmscr_el1 = read_sysreg_s(SYS_PMSCR_EL1);
-	write_sysreg_s(0, SYS_PMSCR_EL1);
+	*pmscr_el1 = read_sysreg_el1(SYS_PMSCR);
+	write_sysreg_el1(0, SYS_PMSCR);
 	isb();
 
 	/* Now drain all buffered data to memory */
@@ -48,7 +48,7 @@ static void __debug_restore_spe(u64 pmscr_el1)
 	isb();
 
 	/* Re-enable data generation */
-	write_sysreg_s(pmscr_el1, SYS_PMSCR_EL1);
+	write_sysreg_el1(pmscr_el1, SYS_PMSCR);
 }
 
 static void __debug_save_trace(u64 *trfcr_el1)
@@ -63,8 +63,8 @@ static void __debug_save_trace(u64 *trfcr_el1)
 	 * Since access to TRFCR_EL1 is trapped, the guest can't
 	 * modify the filtering set by the host.
 	 */
-	*trfcr_el1 = read_sysreg_s(SYS_TRFCR_EL1);
-	write_sysreg_s(0, SYS_TRFCR_EL1);
+	*trfcr_el1 = read_sysreg_el1(SYS_TRFCR);
+	write_sysreg_el1(0, SYS_TRFCR);
 	isb();
 	/* Drain the trace buffer to memory */
 	tsb_csync();
@@ -76,7 +76,7 @@ static void __debug_restore_trace(u64 trfcr_el1)
 		return;
 
 	/* Restore trace filter controls */
-	write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
+	write_sysreg_el1(trfcr_el1, SYS_TRFCR);
 }
 
 void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode

[Oliver Upton]

Hey,

On Thu, Feb 29, 2024 at 02:54:17PM +0000, Marc Zyngier wrote:
> When running in hVHE mode, EL1 accesses are performed with the EL12
> accessor, as we run with HCR_EL2.E2H=1.
> 
> Unfortunately, both PMSCR_EL1 and TRFCR_EL1 are used with the
> EL1 accessor, meaning that we actually affect the EL2 state. Duh.
> 
> Switch to using the {read,write}_sysreg_el1() helpers that will do
> the right thing in all circumstances.

I was wondering if there was a way to surface these screw-ups at compile
time, but there's nothing elegant that comes to mind. Guess we need to
be very careful reviewing "nVHE" changes going forward.

> Note that the 'Fixes:' tag doesn't represent the point where the bug
> was introduced (there is no such point), but the first practical point
> where the hVHE feature is usable.
> 
> Cc: James Clark <james.clark@arm.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Fixes: 38cba55008e5 ("KVM: arm64: Force HCR_E2H in guest context when ARM64_KVM_HVHE is set")
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode

[Marc Zyngier]

On Thu, 29 Feb 2024 17:37:08 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hey,
> 
> On Thu, Feb 29, 2024 at 02:54:17PM +0000, Marc Zyngier wrote:
> > When running in hVHE mode, EL1 accesses are performed with the EL12
> > accessor, as we run with HCR_EL2.E2H=1.
> > 
> > Unfortunately, both PMSCR_EL1 and TRFCR_EL1 are used with the
> > EL1 accessor, meaning that we actually affect the EL2 state. Duh.
> > 
> > Switch to using the {read,write}_sysreg_el1() helpers that will do
> > the right thing in all circumstances.
> 
> I was wondering if there was a way to surface these screw-ups at compile
> time, but there's nothing elegant that comes to mind. Guess we need to
> be very careful reviewing "nVHE" changes going forward.

My take on this is that there should hardly be any read_sysreg_s() in
the KVM code at all. We should always use read_sysreg_el*() so that
there is no ambiguity about the state we're dealing with (that's, of
course, only valid for registers that have both an EL1 and an EL2
counterpart -- registers that are shared across ELs must still use the
read_sysreg_s() accessor).

It would also free the drive-by hacker from having to understand the
subtleties of the E2H redirection. The macros do the right thing
everywhere (they are context aware), and they should be the first port
of call.

> 
> > Note that the 'Fixes:' tag doesn't represent the point where the bug
> > was introduced (there is no such point), but the first practical point
> > where the hVHE feature is usable.
> > 
> > Cc: James Clark <james.clark@arm.com>
> > Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> > Fixes: 38cba55008e5 ("KVM: arm64: Force HCR_E2H in guest context when ARM64_KVM_HVHE is set")
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks. What should we do about it? Fix for 6.8, or part of the 6.9
drop? hVHE+tracing is a pretty niche thing, and I don't have any other
fix for the time being...

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode

[Oliver Upton]

On Thu, Feb 29, 2024 at 06:24:37PM +0000, Marc Zyngier wrote:
> On Thu, 29 Feb 2024 17:37:08 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > I was wondering if there was a way to surface these screw-ups at compile
> > time, but there's nothing elegant that comes to mind. Guess we need to
> > be very careful reviewing "nVHE" changes going forward.
> 
> My take on this is that there should hardly be any read_sysreg_s() in
> the KVM code at all. We should always use read_sysreg_el*() so that
> there is no ambiguity about the state we're dealing with (that's, of
> course, only valid for registers that have both an EL1 and an EL2
> counterpart -- registers that are shared across ELs must still use the
> read_sysreg_s() accessor).

Agreed, I was thinking something along the lines of an accessor that
expresses our intent to access EL2 state, but you can't really add
compile-time assertions behind that.

Perhaps it makes the code slightly more readable, but at that point
we're just rolling a turd in glitter.

> It would also free the drive-by hacker from having to understand the
> subtleties of the E2H redirection. The macros do the right thing
> everywhere (they are context aware), and they should be the first port
> of call.

Right, I think the mechanism for poking at true EL1 state achieves a
good abstraction.

> > Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> 
> Thanks. What should we do about it? Fix for 6.8, or part of the 6.9
> drop? hVHE+tracing is a pretty niche thing, and I don't have any other
> fix for the time being...

Ah, we are pretty late in the cycle, I should've asked :) Happy to pick
this up for 6.9 then.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode

[Oliver Upton]

On Thu, 29 Feb 2024 14:54:17 +0000, Marc Zyngier wrote:
> When running in hVHE mode, EL1 accesses are performed with the EL12
> accessor, as we run with HCR_EL2.E2H=1.
> 
> Unfortunately, both PMSCR_EL1 and TRFCR_EL1 are used with the
> EL1 accessor, meaning that we actually affect the EL2 state. Duh.
> 
> Switch to using the {read,write}_sysreg_el1() helpers that will do
> the right thing in all circumstances.
> 
> [...]

Applied to kvmarm/next, thanks!

[1/1] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode
      https://git.kernel.org/kvmarm/kvmarm/c/9a3bfb27ef65

--
Best,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel