Re: [Qemu-devel] [PATCH 04/11] target-arm: Define correct mmu_idx values and pass them in TB flags

[Greg Bellows]

On Jan 23, 2015 7:12 PM, "Peter Maydell" <address@hidden> wrote:
>
> On 23 January 2015 at 21:44, Greg Bellows <address@hidden> wrote:
> > On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell
> > <address@hidden> wrote:
> >> +typedef enum ARMMMUIdx {
> >> +    ARMMMUIdx_S12NSE0 = 0,
> >> +    ARMMMUIdx_S12NSE1 = 1,
> >> +    ARMMMUIdx_S1E2 = 2,
> >> +    ARMMMUIdx_S1E3 = 3,
> >> +    ARMMMUIdx_S1SE0 = 4,
> >> +    ARMMMUIdx_S1SE1 = 5,
> >> +    ARMMMUIdx_S2NS = 6,
> >> +    /* Indexes below here don't have TLBs and are used only for AT system
> >> +     * instructions or for the first stage of an S12 page table walk.
> >> +     */
> >> +    ARMMMUIdx_S1NSE0 = 7,
> >> +    ARMMMUIdx_S1NSE1 = 8,
> >> +} ARMMMUIdx;
> >> +
> >>  #define MMU_MODE0_SUFFIX _user
> >>  #define MMU_MODE1_SUFFIX _kernel
> >>  #define MMU_USER_IDX 0
> >> +
> >> +/* Return the exception level we're running at if this is our mmu_idx */
> >> +static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
> >> +{
> >> +    assert(mmu_idx < ARMMMUIdx_S2NS);
> >> +    return mmu_idx & 3;
> >> +}
> >> +
> >> +/* Determine the current mmu_idx to use for normal loads/stores */
> >>  static inline int cpu_mmu_index (CPUARMState *env)
> >>  {
> >> -    return arm_current_el(env);
> >> +    int el = arm_current_el(env);
> >> +
> >> +    if (el < 3 && arm_is_secure_below_el3(env)) {
> >
> > We bypass the secure check for EL3 but not EL2.  We should either
> > circumvent both EL2 & 3 or neither.
>
> Not sure what you mean here. The point of this code is to return
> a suitable MMU idx for the current CPU state. If we are in
> EL2 or EL3 then just "el" is correct (being ARMMMUIdx_S1E2 and
> ARMMMUIdx_S1E3). We can't be in this condition with el == 2

I understand what the code is doing, my point is that you rely on arm_is_secure_below_el3 to deal with EL2 when you could have just as easily checked for el<2.  Not a big deal though.

> because if we're in EL2 then the NS bit must be set (there's
> no such thing as Secure EL2) and so arm_is_secure_below_el3()
> will have returned false. So we know here that el is 0 or 1,
> and this addition below:
>
> >> +        return ARMMMUIdx_S1SE0 + el;
>
> means we end up with either _S1SE0 or _S1SE1, as required.
>
> (the condition could equally well be written
>    if (el < 2 && arm_is_secure_below_el3(env))
>  as the two are true in exactly the same set of cases. < 3 seemed
>  marginally better to me, since it's expressing "if we're secure
> and not in EL3" which is the set of cases where we need to
> change the mmu_idx from the 0/1/2/3 values.)
>
> >> +    }
> >> +    return el;
>
> Anything else (_S12NSE0, _S12NSE1, _S1E2, _S1E3) is this return.
>
> >>  }
>
> thanks
> -- PMM