Re: [PATCH v1 1/1] target/arm: Drop access_el3_aa32ns()

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 12:01:07PM +0100, Peter Maydell wrote:
> On Tue, 28 Apr 2020 at 17:03, Edgar E. Iglesias
> <address@hidden> wrote:
> >
> > From: "Edgar E. Iglesias" <address@hidden>
> >
> > Calling access_el3_aa32ns() works for AArch32 only cores
> > but it does not handle 32-bit EL2 on top of 64-bit EL3
> > for mixed 32/64-bit cores.
> >
> > Fold access_el3_aa32ns() into access_el3_aa32ns_aa64any()
> > and replace all direct uses of the aa32 only version with
> > access_el3_aa32ns_aa64any().
> >
> > Fixes: 68e9c2fe65 ("target-arm: Add VTCR_EL2")
> > Reported-by: Laurent Desnogues <address@hidden>
> > Signed-off-by: Edgar E. Iglesias <address@hidden>
> 
> So, this is definitely a bug, but I think we could be
> clearer about what we're fixing.
> 
> For all these registers, the way the Arm ARM pseudocode phrases
> this access check is:
>  * for the AArch64 view of the register, no check
>  * for the AArch32 view of the register:
>       ...
>    elsif PSTATE.EL == EL2 then
>       return VTTBR;
>    elsif PSTATE.EL == EL3 then
>       if SCR.NS == '0' then
>           UNDEFINED;
>       else
>           return VTTBR;
> (similarly for the write path). We don't implement the HSTR.T2
> traps, so for us these registers are all .access = PL2_RW and
> we just UNDEF for all EL0/EL1 accesses.
> 
> So what we're really trying to check for is "current EL is EL3
> and we are AArch32 and SCR.NS == '0'". Because it's not possible
> to be in AArch32 Hyp with SCR.NS == 0, the check we make in
> your function is an equivalent test, but we could improve
> the comments:
> > ---
> >  target/arm/helper.c | 34 ++++++++++------------------------
> >  1 file changed, 10 insertions(+), 24 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 7e9ea5d20f..888f5f2314 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -504,29 +504,15 @@ void init_cpreg_list(ARMCPU *cpu)
> >  /*
> >   * Some registers are not accessible if EL3.NS=0 and EL3 is using AArch32 
> > but
> >   * they are accessible when EL3 is using AArch64 regardless of EL3.NS.
> 
> This could be rewritten as:
>    Some registers are not accessible from AArch32 EL3 if SCR.NS == 0.


Done in v2.


> 
> > - *
> > - * access_el3_aa32ns: Used to check AArch32 register views.
> > - * access_el3_aa32ns_aa64any: Used to check both AArch32/64 register views.
> >   */
> > -static CPAccessResult access_el3_aa32ns(CPUARMState *env,
> > -                                        const ARMCPRegInfo *ri,
> > -                                        bool isread)
> > -{
> > -    bool secure = arm_is_secure_below_el3(env);
> > -
> > -    assert(!arm_el_is_aa64(env, 3));
> > -    if (secure) {
> > -        return CP_ACCESS_TRAP_UNCATEGORIZED;
> > -    }
> > -    return CP_ACCESS_OK;
> > -}
> > -
> >  static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
> >                                                  const ARMCPRegInfo *ri,
> >                                                  bool isread)
> >  {
> > -    if (!arm_el_is_aa64(env, 3)) {
> > -        return access_el3_aa32ns(env, ri, isread);
> > +    bool secure = arm_is_secure_below_el3(env);
> > +
> > +    if (!arm_el_is_aa64(env, 3) && secure) {
> 
> We could either rephrase this as
>        if (!is_a64(env) && arm_current_el(env) == 3 &&
>            arm_is_secure_below_el3(env)) {

Went for this logic in v2.


> 
> or just have a comment
>        /*
>         * This access function is only used with .access = PL2_RW
>         * registers, so we are in AArch32 EL3 with SCR.NS == 0
>         * if and only if EL3 is AArch32 and SCR.NS == 0, because
>         * if SCR.NS == 0 we cannot be in EL2.
>         */
> 
> depending on how much you proritize a more efficient test
> over a more clearly correct test :-)
> 
> > +        return CP_ACCESS_TRAP_UNCATEGORIZED;
> >      }
> >      return CP_ACCESS_OK;
> >  }
> 
> Also, once we don't have a distinction between two different
> flavours of this access function we should use the simpler
> "access_el2_aa32ns", rather than ending up using the longer
> name for the one version of the function we're keeping.

Done in v2.

Thanks,
Edgar