Re: [PATCH] target/arm: Two fixes for secure ptw

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 3 Nov 2022 at 13:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 2 Nov 2022 at 05:47, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > Reversed the sense of non-secure in get_phys_addr_lpae,
> > and failed to initialize attrs.secure for ARMMMUIdx_Phys_S.
> >
> > Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >  target/arm/ptw.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> > index 58a7bbda50..df3573f150 100644
> > --- a/target/arm/ptw.c
> > +++ b/target/arm/ptw.c
> > @@ -1357,7 +1357,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
> >      descaddr |= (address >> (stride * (4 - level))) & indexmask;
> >      descaddr &= ~7ULL;
> >      nstable = extract32(tableattrs, 4, 1);
> > -    if (!nstable) {
> > +    if (nstable) {
> >          /*
> >           * Stage2_S -> Stage2 or Phys_S -> Phys_NS
> >           * Assert that the non-secure idx are even, and relative order.
> > @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> >      bool is_secure = ptw->in_secure;
> >      ARMMMUIdx s1_mmu_idx;
> >
> > +    /*
> > +     * The page table entries may downgrade secure to non-secure, but
> > +     * cannot upgrade an non-secure translation regime's attributes
> > +     * to secure.
> > +     */
> > +    result->f.attrs.secure = is_secure;
> > +
> >      switch (mmu_idx) {
> >      case ARMMMUIdx_Phys_S:
> >      case ARMMMUIdx_Phys_NS:
> > @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> >          break;
> >      }
> >
> > -    /*
> > -     * The page table entries may downgrade secure to non-secure, but
> > -     * cannot upgrade an non-secure translation regime's attributes
> > -     * to secure.
> > -     */
> > -    result->f.attrs.secure = is_secure;
> >      result->f.attrs.user = regime_is_user(env, mmu_idx);
>
> Do we also need to move this setting of attrs.user ?
> get_phys_addr_disabled() doesn't set that either.

I've applied this to target-arm.next for the moment anyway, since
it is definitely fixing an attrs.secure related bug. I can replace
that with a v2 or we can do a follow-on patch, depending whether
you get to this before or after I send out a pullreq.

thanks
-- PMM