Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr()

[Greg Bellows <greg.bellows@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 2943 bytes --]

On Tue, Jan 27, 2015 at 12:12 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 January 2015 at 17:57, Greg Bellows <greg.bellows@linaro.org> wrote:
> >
> >
> > On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <
> peter.maydell@linaro.org>
> > wrote:
> >> +/* Return the exception level which controls this address translation
> >> regime */
> >> +static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
> >> +{
> >> +    switch (mmu_idx) {
> >> +    case ARMMMUIdx_S2NS:
> >> +    case ARMMMUIdx_S1E2:
> >> +        return 2;
> >> +    case ARMMMUIdx_S1E3:
> >> +        return 3;
> >> +    case ARMMMUIdx_S1SE0:
> >> +        return arm_el_is_aa64(env, 3) ? 1 : 3;
> >> +    case ARMMMUIdx_S1SE1:
> >
> >
> > I think this should be handled the same way as S1SE0 as Secure EL1 maps
> to
> > EL3 when EL3 is AArch32.
>
> If EL3 is AArch32 then you'll never be using this MMU index.
> By definition the S1SE1 index is for code executing at
> Secure EL1, and there isn't any of that unless EL3 is 64 bit.
> (Secure EL1 doesn't "map to" anything, it just doesn't
> exist/have any code running in it.)
>

​Ah yes and thanks for the pointer to the origin of mmu index I have now
connected where we prevent that index with this code.


>
> >> +    case ARMMMUIdx_S1NSE0:
> >> +    case ARMMMUIdx_S1NSE1:
> >> +        return 1;
> >> +    default:
> >> +        g_assert_not_reached();
> >> +    }
> >> +}
> >> +
> >> +/* Return the SCTLR value which controls this address translation
> regime
> >> */
> >> +static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx
> mmu_idx)
> >> +{
> >> +    return env->cp15.sctlr_el[regime_el(env, mmu_idx)];
> >
> >
> > Given the above regime_el(), for S1SE1, this would return the non-secure
> > SCTLR bank on a secure translation.  Same below for TCR and all uses
> > thereafter.
>
> That's correct, because S1SE1 implies "secure EL1 under a 64 bit
> EL3", in which case there is no system register banking and
> both Secure and NonSecure use the same underlying register.
> Compare the way that A32_BANKED_CURRENT_REG_GET/SET always
> use the NS version if arm_el_is_aa64(env, 3).
>
> >> +static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
> >> +{
> >> +    switch (mmu_idx) {
> >> +    case ARMMMUIdx_S1SE0:
> >> +    case ARMMMUIdx_S1NSE0:
> >
> >
> > The get_phys_addr path filters out ARMMMUIdx_S12NSE0 so calls to this
> > function in this context don't matter, but what if it is called outside
> this
> > context?  How should it handle this index?
>
> g_assert_not_reached(),  but it didn't seem worth cluttering
> the switch with a bunch of extra labels just to assert that
> they weren't reachable.
>
>
​I see how it could clutter things, but given that the routine is generic
we probably should just like we do in regime_el().​



> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 4439 bytes --]