On Thu, Jul 06, 2023 at 03:28:32PM +0100, Peter Maydell wrote: > On Thu, 6 Jul 2023 at 15:12, Jean-Philippe Brucker > <[email protected]> wrote: > > > > Arm TF-A fails to boot via semihosting following a recent change to the > > MMU code. Semihosting attempts to read parameters passed by TF-A in > > secure RAM via cpu_memory_rw_debug(). While performing the S1 > > translation, we call S1_ptw_translate() on the page table descriptor > > address, with an MMU index of ARMMMUIdx_Phys_S. At the moment > > S1_ptw_translate() doesn't interpret this as a secure access, and as a > > result we attempt to read the page table descriptor from the non-secure > > address space, which fails. > > > > Fixes: fe4a5472ccd6 ("target/arm: Use get_phys_addr_with_struct in > > S1_ptw_translate") > > Signed-off-by: Jean-Philippe Brucker <[email protected]> > > --- > > I'm not entirely sure why the semihosting parameters are accessed > > through stage-1 translation rather than directly as physical addresses, > > but I'm not familiar with semihosting. > > The semihosting ABI says the guest code should pass "a pointer > to the parameter block". It doesn't say explicitly, but the > straightforward interpretation is "a pointer that the guest > itself could dereference to read/write the values", which means > a virtual address, not a physical one. It would be pretty > painful for the guest to have to figure out "what is the > physaddr for this virtual address" to pass it to the semihosting > call. > > Do you have a repro case for this bug? Did it work > before commit fe4a5472ccd6 ?
Yes I bisected to fe4a5472ccd6 by trying to run TF-A, following instructions here: https://github.com/ARM-software/arm-trusted-firmware/blob/master/docs/plat/qemu.rst Building TF-A (HEAD 8e31faa05): make -j CROSS_COMPILE=aarch64-linux-gnu- BL33=edk2/Build/ArmVirtQemuKernel-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd PLAT=qemu DEBUG=1 LOG_LEVEL=40 all fip Installing it to QEMU runtime dir: ln -sf tf-a/build/qemu/debug/bl1.bin build/qemu-cca/run/ ln -sf tf-a/build/qemu/debug/bl2.bin build/qemu-cca/run/ ln -sf tf-a/build/qemu/debug/bl31.bin build/qemu-cca/run/ ln -sf edk2/Build/ArmVirtQemuKernel-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd build/qemu-cca/run/bl33.bin Running QEMU with HEAD=fe4a5472cc: qemu-system-aarch64 -M virt,secure=on -cpu max -m 1G -nographic -bios bl1.bin -semihosting-config enable=on,target=native -d guest_errors NOTICE: Booting Trusted Firmware NOTICE: BL1: v2.9(debug):v2.9.0-280-g8e31faa05 NOTICE: BL1: Built : 16:23:20, Jul 6 2023 INFO: BL1: RAM 0xe0ee000 - 0xe0f6000 INFO: BL1: Loading BL2 WARNING: Firmware Image Package header check failed. Invalid read at addr 0xE0EF900, size 8, region '(null)', reason: rejected WARNING: Failed to obtain reference to image id=1 (-2) ERROR: Failed to load BL2 firmware. with HEAD=4a7d7702cd: ... INFO: BL1: Loading BL2 WARNING: Firmware Image Package header check failed. INFO: Loading image id=1 at address 0xe06b000 INFO: Image id=1 loaded: 0xe06b000 - 0xe073201 NOTICE: BL1: Booting BL2 INFO: Entry point address = 0xe06b000 INFO: SPSR = 0x3c5 ... (Note that there is an issue with TF-A missing ENABLE_FEAT_FGT for qemu at the moment, which prevents booting Linux with -cpu max. I'll send the fix to TF-A after this, but this reproducer should at least boot edk2.) > > --- > > target/arm/ptw.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > > index 9aaff1546a..e3a738c28e 100644 > > --- a/target/arm/ptw.c > > +++ b/target/arm/ptw.c > > @@ -465,10 +465,8 @@ static bool S1_ptw_translate(CPUARMState *env, > > S1Translate *ptw, > > S1Translate s2ptw = { > > .in_mmu_idx = s2_mmu_idx, > > .in_ptw_idx = ptw_idx_for_stage_2(env, s2_mmu_idx), > > - .in_secure = s2_mmu_idx == ARMMMUIdx_Stage2_S, > > - .in_space = (s2_mmu_idx == ARMMMUIdx_Stage2_S ? ARMSS_Secure > > - : space == ARMSS_Realm ? ARMSS_Realm > > - : ARMSS_NonSecure), > > + .in_secure = is_secure, > > + .in_space = space, > > If the problem is fe4a5472ccd6 then this seems an odd change to > be making, because in_secure and in_space were set that way > before that commit too... I think that commit merged both sides of the "regime_is_stage2(s2_mmu_idx)" test, but only kept testing for secure through ARMMMUIdx_Stage2_S, and removed the test through ARMMMUIdx_Phys_S Thanks, Jean
