| Message ID | 20240229145417.3606279-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode | expand |
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>
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.
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.
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
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)
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(-)