RE: ARM: ptw.c:S1_ptw_translate

[Sid Manning]

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Wednesday, January 4, 2023 11:42 PM
> To: Sid Manning <sidneym@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@linaro.org; Mark Burton <mburton@qti.qualcomm.com>
> Subject: Re: ARM: ptw.c:S1_ptw_translate
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On 1/4/23 08:55, Sid Manning wrote:
> > ptw.c:S1_ptw_translate
> >
> > After migrating to v7.2.0, an issue was found where we were not
> > getting the correct virtual address from a load insn.  Reading the
> > address used in the load insn from the debugger resulted in the
> > execution of the insn getting the correct value but simply stepping over the
> insn did not.
> >
> > This is the instruction:
> >
> > ldr           x0, [x1, #24]
> >
> > The debug path varies based on the regime and if regime is NOT stage
> > two out_phys is set to addr if the regime is stage 2 then out_phys is
> > set to s2.f.phys_addr.  In the non-debug path out_phys is always set to 
> > full-
> >phys_addr.
> >
> > I got around this by only using full->phys_addr if regime_is_stage2 was
> true:
> >
> > diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> >
> > index 3745ac9723..87bc6754a6 100644
> >
> > --- a/target/arm/ptw.c
> >
> > +++ b/target/arm/ptw.c
> >
> > @@ -266,7 +266,12 @@ static bool S1_ptw_translate(CPUARMState *env,
> > S1Translate *ptw,
> >
> >           if (unlikely(flags & TLB_INVALID_MASK)) {
> >
> >               goto fail;
> >
> >           }
> >
> > -        ptw->out_phys = full->phys_addr;
> >
> > +
> >
> > +        if (regime_is_stage2(s2_mmu_idx)) {
> >
> > +            ptw->out_phys = full->phys_addr;
> >
> > +        } else {
> >
> > +            ptw->out_phys = addr;
> >
> > +        }
> >
> >           ptw->out_rw = full->prot & PAGE_WRITE;
> >
> >           pte_attrs = full->pte_attrs;
> >
> >           pte_secure = full->attrs.secure;
> >
> > This change got me the answer I wanted but I’m not familiar enough
> > with the code to know if this is correct or not.
> 
> This is incorrect.  If you are getting the wrong value here, then something 
> has
> gone wrong elsewhere, as the s2_mmu_idx result was logged.
> 
> Do you have a test case you can share?

This happens while booting QNX so I can't share it.  I don't have the source 
code either just the object code.  A number of cores are being started and the 
address happens to be what will eventually become the stack.

I'll see what I can come up with to better characterize is problem.

Thanks,
> 
> 
> r~