Message ID | 20250130182309.717346-1-peter.maydell@linaro.org (mailing list archive) |
---|---|
Headers | show |
Series | target/arm: Clean up some corner cases of sysreg traps | expand |
Ping for review on patches 2, 3, 9, 10, 12, 14, please? thanks -- PMM On Thu, 30 Jan 2025 at 18:23, Peter Maydell <peter.maydell@linaro.org> wrote: > > 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(-) > > --