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

[Greg Bellows]

On Tue, Jan 27, 2015 at 12:12 PM, Peter Maydell <address@hidden> wrote:

On 27 January 2015 at 17:57, Greg Bellows <address@hidden> wrote:
>
>
> On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <address@hidden>
> 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