Re: [Qemu-devel] [PATCH v3 22/22] RFC: target-arm: Use a 1:1 mapping between EL and MMU index

[Edgar E. Iglesias]

On Tue, May 20, 2014 at 09:47:47AM +0000, Aggeler  Fabian wrote:
> I guess this makes sense. Shouldn’t we implement two more MMUs to separate 
> S-EL0/EL0 and S-EL1/EL1
> at least for ARMv8 with EL3 running in Aarch64 state?

Maybe with future patches. My understanding is that on aarch64 the world
switch between S/NS requires EL3 firmware to reprogram the TTBR regs.
Currently in QEMU, the re-programming of TTBR will flush the TLBs. We would
need to do something about that before adding MMU tables for aarch64 Secure
EL0/1 does any good. I think it's better to keep it simple for now and leave
this as a possible future optimization.

Another possible future optimization is to add some kind of dynamic allocation
of a limited set of MMU tables for different ASIDs and VMIDs. For emulated
virtualization, it might help quite a bit.


> For ARMv7 and ARMv8 with EL3 in Aarch32 S-PL1 
> is mapped to PL3, so we only need one additional MMU for S-PL0. If you agree 
> I could add this change in 
> the Security Extension patches after this patch makes it into the tree.

Yes, I avoided this patch in my v1 because I thought we would need a non
linear mapping for aarch32 S/NS anyway. But I agree that a combination is
good. Keeping a 1:1 mapping between EL -> MMU idx and have additional MMU
tables for specific features like S/NS.

Thanks,
Edgar

> 
> Best,
> Fabian
> 
> On 19 May 2014, at 11:23, Edgar E. Iglesias <address@hidden> wrote:
> 
> > From: "Edgar E. Iglesias" <address@hidden>
> > 
> > Signed-off-by: Edgar E. Iglesias <address@hidden>
> > ---
> > target-arm/cpu.h       | 26 ++++----------------------
> > target-arm/translate.h |  2 +-
> > 2 files changed, 5 insertions(+), 23 deletions(-)
> > 
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 566f9ed..3b7ef32 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -1084,32 +1084,14 @@ static inline CPUARMState *cpu_init(const char 
> > *cpu_model)
> > #define cpu_list arm_cpu_list
> > 
> > /* MMU modes definitions */
> > -#define MMU_MODE0_SUFFIX _kernel
> > -#define MMU_MODE1_SUFFIX _user
> > -#define MMU_USER_IDX 1
> > -
> > -static inline int arm_el_to_mmu_idx(int current_el)
> > -{
> > -#ifdef CONFIG_USER_ONLY
> > -    return MMU_USER_IDX;
> > -#else
> > -    switch (current_el) {
> > -    case 0:
> > -        return MMU_USER_IDX;
> > -    case 1:
> > -        return 0;
> > -    default:
> > -        /* Unsupported EL.  */
> > -        assert(0);
> > -        return 0;
> > -    }
> > -#endif
> > -}
> > +#define MMU_MODE0_SUFFIX _user
> > +#define MMU_MODE1_SUFFIX _kernel
> > +#define MMU_USER_IDX 0
> > 
> > static inline int cpu_mmu_index (CPUARMState *env)
> > {
> >     int cur_el = arm_current_pl(env);
> > -    return arm_el_to_mmu_idx(cur_el);
> > +    return cur_el;
> > }
> > 
> > #include "exec/cpu-all.h"
> > diff --git a/target-arm/translate.h b/target-arm/translate.h
> > index db6f0af..31a0104 100644
> > --- a/target-arm/translate.h
> > +++ b/target-arm/translate.h
> > @@ -54,7 +54,7 @@ static inline int arm_dc_feature(DisasContext *dc, int 
> > feature)
> > 
> > static inline int get_mem_index(DisasContext *s)
> > {
> > -    return arm_el_to_mmu_idx(s->current_pl);
> > +    return s->current_pl;
> > }
> > 
> > /* target-specific extra values for is_jmp */
> > -- 
> > 1.8.3.2
> > 
>