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

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

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