Re: [PATCH] target/arm: Fix ptw parameters in S1_ptw_translate() for debug contexts

[Peter Maydell]

On Thu, 6 Jul 2023 at 17:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 6 Jul 2023 at 17:10, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > On Thu, Jul 06, 2023 at 04:42:02PM +0100, Peter Maydell wrote:
> > > > > 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
> > >
> > > Could you put the necessary binary blobs up somewhere, to save
> > > me trying to rebuild TF-A ?
> >
> > Uploaded to:
> > https://jpbrucker.net/tmp/2023-07-06-repro-qemu-tfa.tar.gz
>
> Thanks, I've got that and can repro the failure. I probably won't
> be able to get a patch sorted before Monday, I'm afraid.

Tentative patch, which works on the test case:

--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -449,7 +449,7 @@ static bool S2_attrs_are_device(uint64_t hcr, uint8_t attrs)
 static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
                              hwaddr addr, ARMMMUFaultInfo *fi)
 {
-    ARMSecuritySpace space = ptw->in_space;
+    ARMSecuritySpace s2_space;
     bool is_secure = ptw->in_secure;
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
     ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx;
@@ -457,6 +457,9 @@ static bool S1_ptw_translate(CPUARMState *env,
S1Translate *ptw,

     ptw->out_virt = addr;

+    s2_space = regime_is_stage2(s2_mmu_idx) ?
+        ptw->in_space : arm_phys_to_space(s2_mmu_idx);
+
     if (unlikely(ptw->in_debug)) {
         /*
          * From gdbstub, do not use softmmu so that we don't modify the
@@ -465,10 +468,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 = arm_space_is_secure(s2_space),
+            .in_space = s2_space,
             .in_debug = true,
         };
         GetPhysAddrResult s2 = { };

But I need to check whether just using the ptw->in_space
as the stage 2 walk space is correct, which will have to
wait til Monday.

-- PMM