Re: [PATCH 12/22] target/arm: NSTable is RES0 for the RME EL3 regime

[Richard Henderson]

On 2/10/23 01:36, Peter Maydell wrote:
> On Tue, 24 Jan 2023 at 00:01, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > Test in_space instead of in_secure so that we don't switch
> > out of Root space.  Handle the output space change immediately,
> > rather than try and combine the NSTable and NS bits later.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   target/arm/ptw.c | 31 ++++++++++++++-----------------
> >   1 file changed, 14 insertions(+), 17 deletions(-)
> >
> > diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> > index c1b0b8e610..ddafb1f329 100644
> > --- a/target/arm/ptw.c
> > +++ b/target/arm/ptw.c
> > @@ -1240,7 +1240,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> > S1Translate *ptw,
> >   {
> >       ARMCPU *cpu = env_archcpu(env);
> >       ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
> > -    bool is_secure = ptw->in_secure;
> >       int32_t level;
> >       ARMVAParameters param;
> >       uint64_t ttbr;
> > @@ -1256,7 +1255,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> > S1Translate *ptw,
> >       uint64_t descaddrmask;
> >       bool aarch64 = arm_el_is_aa64(env, el);
> >       uint64_t descriptor, new_descriptor;
> > -    bool nstable;
> >
> >       /* TODO: This code does not support shareability levels. */
> >       if (aarch64) {
> > @@ -1417,29 +1415,29 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> > S1Translate *ptw,
> >           descaddrmask = MAKE_64BIT_MASK(0, 40);
> >       }
> >       descaddrmask &= ~indexmask_grainsize;
> > -
> > -    /*
> > -     * Secure accesses start with the page table in secure memory and
> > -     * can be downgraded to non-secure at any step. Non-secure accesses
> > -     * remain non-secure. We implement this by just ORing in the NSTable/NS
> > -     * bits at each step.
> > -     */
> > -    tableattrs = is_secure ? 0 : (1 << 4);
> > +    tableattrs = 0;
> >
> >    next_level:
> >       descaddr |= (address >> (stride * (4 - level))) & indexmask;
> >       descaddr &= ~7ULL;
> > -    nstable = extract32(tableattrs, 4, 1);
> > -    if (nstable && ptw->in_secure) {
> > -        /*
> > -         * Stage2_S -> Stage2 or Phys_S -> Phys_NS
> > -         * Assert that the non-secure idx are even, and relative order.
> > -         */
> > +
> > +    /*
> > +     * Process the NSTable bit from the previous level.  This changes
> > +     * the table address space and the output space from Secure to
> > +     * NonSecure.  With RME, the EL3 translation regime does not change
> > +     * from Root to NonSecure.
> > +     */
>
> To check my understanding, this means that the bit that the spec
> describes as FEAT_RME changing the behaviour of NSTable in the EL3
> stage 1 translation regime is implemented by us by having the
> in_space for EL3 be different for FEAT_RME and not-FEAT_RME ?

Correct -- space is Secure for non-RME EL3, and Root for RME EL3.

> >       attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 
> > 14));
> >       if (!regime_is_stage2(mmu_idx)) {
> > -        attrs |= nstable << 5; /* NS */
>
> This removes the code where we copy the NSTable bit across to attrs,
> but there's still code below here that assumes it can get the combined
> NS bit from bit 5 of attrs, isn't there? (It passes it to get_S1prot().)

Oops.  This gets fixed in patch 14.  Some reordering needed...


r~