mbox series

[00/14] target/arm: Clean up some corner cases of sysreg traps

Message ID 20250130182309.717346-1-peter.maydell@linaro.org
Headers show
Series target/arm: Clean up some corner cases of sysreg traps | expand

Message

Peter Maydell Jan. 30, 2025, 6:22 p.m. UTC 
While reviewing Alex's recent secure timer patchset, I noticed a
bug where it was using CP_ACCESS_TRAP when CP_ACCESS_TRAP_UNCATEGORIZED
was wanted, and that we were making the same mistake elsewhere in
our existing code. This series started out as an attempt to fix
that and also rearrange the API so that it's harder to introduce
other instances of this bug in future. In the process I noticed
a different bug, where we weren't handling traps to AArch32
Monitor mode correctly, so the series fixes those as well.

The first four patches are fixes for the places where we were
using CP_ACCESS_TRAP when we wanted CP_ACCESS_TRAP_UNCATEGORIZED.
These are all situations where an attempt to access a sysreg
should UNDEF and we were incorrectly reporting it with a
syndrome value for a system register access trap. I've cc'd
these to stable as they are technically bugs, but really the impact
s very limited because I can't see a reason why any code except
test code would deliberately attempt a sysreg access that they
knew would take an exception and then look at the syndrome
value for it.

Patches 5, 6 and 7 together fix bugs in our handling of sysreg
traps that should go to AArch32 Monitor mode:
 * we were incorrectly triggering an UNDEF exception for these
   rather than a Monitor Trap, so the exception would go to
   the wrong vector table and the wrong CPU mode
 * in most cases we weren't trapping accesses from EL3
   non-Monitor modes to Monitor mode
This affects only:
 * trapping of ERRIDR via SCR.TERR
 * trapping of the debug channel registers via SDCR.TDCC
 * trapping of GICv3 registers via SCR.IRQ and SCR.FIQ
because most "trap sysreg access to EL3" situations are either for
AArch64 only registers or for trap bits that are AArch64 only.

Patch 8 is a trivial one removing an unnecessary clause in
an if() condition in the GIC cpuif access functions.

Patches 9-13 are the API change that tries to make the bug harder to
write: we add CP_ACCESS_TRAP_EL1 for "trap a sysreg access direct to
EL1". After all the bugfixes in the first half of the series, the
remaining uses of CP_ACCESS_TRAP are all in contexts where we know the
current EL is 0, so we can directly replace them with
CP_ACCESS_TRAP_EL1, and remove CP_ACCESS_TRAP entirely. We also rename
CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED, to be a clearer
parallel with the pseudocode's use of "UNDEFINED" in this situation.
The resulting
API is that an accessfn can only return:
 CP_ACCESS_OK for a success
 CP_ACCESS_UNDEFINED for an UNDEF
 CP_ACCESS_TRAP_EL{1,2,3} for a sysreg trap direct to an EL
and everything else is invalid. UNCATEGORIZED traps never go to a
specified target EL, and sysreg-traps always go to a specified target
EL, matching the pseudocode which either uses "UNDEFINED" or some kind
of SystemAccessTrap(ELx, ...).

Patch 14 fixes some issues with the WFI/WFX trap code, some of
which are like the above sysreg access check bugs (not using
Monitor Trap, not honouring traps in EL3 not-Monitor-mode),
plus one which I noticed while working on the code (it doesn't
use arm_sctlr() so will look at the wrong SCTLR when in EL2&0).

I've cc'd the relevant patches to stable, but I don't think
it's very likely that anybody ever ran into these bugs, because
they're all cases of "we did the wrong thing if code at an EL
below EL3 tried to do something it shouldn't". I don't think that
EL3 code generally uses the trap bits for trap-and-emulate
of functionality, only to prevent the lower ELs messing with
state it should not, and everything here with the exception of
SCR.IRQ and SCR.FIQ is pretty obscure anyway.

(Tested somewhat lightly, because I don't have any test images
that test AArch32 EL3 really.)

thanks
-- PMM

Peter Maydell (14):
  target/arm: Report correct syndrome for UNDEFINED CNTPS_*_EL1 from EL2
    and NS EL1
  target/arm: Report correct syndrome for UNDEFINED AT ops with wrong
    NSE,NS
  target/arm: Report correct syndrome for UNDEFINED S1E2 AT ops at EL3
  target/arm: Report correct syndrome for UNDEFINED LOR sysregs when
    NS=0
  target/arm: Make CP_ACCESS_TRAPs to AArch32 EL3 be Monitor traps
  hw/intc/arm_gicv3_cpuif: Don't downgrade monitor traps for AArch32 EL3
  target/arm: Honour SDCR.TDCC and SCR.TERR in AArch32 EL3 non-Monitor
    modes
  hw/intc/arm_gicv3_cpuif(): Remove redundant tests of is_a64()
  target/arm: Support CP_ACCESS_TRAP_EL1 as a CPAccessResult
  target/arm: Use CP_ACCESS_TRAP_EL1 for traps that are always to EL1
  target/arm: Use TRAP_UNCATEGORIZED for XScale CPAR traps
  target/arm: Remove CP_ACCESS_TRAP handling
  target/arm: Rename CP_ACCESS_TRAP_UNCATEGORIZED to CP_ACCESS_UNDEFINED
  target/arm: Correct errors in WFI/WFE trapping

 target/arm/cpregs.h        | 15 +++++---
 target/arm/cpu.h           |  6 +++
 hw/intc/arm_gicv3_cpuif.c  | 15 ++------
 target/arm/debug_helper.c  |  5 ++-
 target/arm/helper.c        | 75 ++++++++++++++++++++++----------------
 target/arm/tcg/op_helper.c | 71 ++++++++++++++++++++++--------------
 6 files changed, 107 insertions(+), 80 deletions(-)