On Mon, May 04, 2020 at 12:01:07PM +0100, Peter Maydell wrote:
> On Tue, 28 Apr 2020 at 17:03, Edgar E. Iglesias
> <edgar.igles...@gmail.com> wrote:
> >
> > From: "Edgar E. Iglesias" <edgar.igles...@xilinx.com>
> >
> > 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 <laurent.desnog...@gmail.com>
> > Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
>
> 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
