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, Jean > > > > > > --- > > > > 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 > > Yes, I agree. I'm not sure your proposed fix is the right one, > though. I need to re-work through what I did in fcc0b0418fff > to remind myself of what the various fields in a S1Translate > struct are supposed to be, but I think .in_secure (and now > .in_space) are supposed to always match .in_mmu_idx, and > that's not necessarily the same as what the local is_secure > holds. (is_secure is the original ptw's in_secure, which > matches that ptw's .in_mmu_idx, not its .in_ptw_idx.) > So probably the right thing for the .in_secure check is > to change to "(s2_mmu_idx == ARMMMUIdx_Stage2_S || > s2_mmu_idx == ARMMMUIdx_Phys_S)". Less sure about .in_space, > because that conditional is a bit more complicated. > > thanks > -- PMM
