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(-)
>
> --