diff mbox

[v3,22/22] RFC: target-arm: Use a 1:1 mapping between EL and MMU index

Message ID 1400491383-6725-23-git-send-email-edgar.iglesias@gmail.com
State New
Headers show

Commit Message

Edgar E. Iglesias May 19, 2014, 9:23 a.m. UTC 
From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
---
 target-arm/cpu.h       | 26 ++++----------------------
 target-arm/translate.h |  2 +-
 2 files changed, 5 insertions(+), 23 deletions(-)

Comments

Peter Crosthwaite May 20, 2014, 9:07 a.m. UTC | #1 
On Mon, May 19, 2014 at 7:23 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> ---
>  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
>
>

I'm a fan of this change. Should it squash into earlier in the series?

Regards,
Peter
Aggeler Fabian May 20, 2014, 9:47 a.m. UTC | #2 
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? 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.

Best,
Fabian

On 19 May 2014, at 11:23, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:

> From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> ---
> 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
>
Edgar E. Iglesias May 20, 2014, 1:28 p.m. UTC | #3 
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 <edgar.iglesias@gmail.com> wrote:
> 
> > From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > ---
> > 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
> > 
>
Peter Maydell May 20, 2014, 1:47 p.m. UTC | #4 
On 20 May 2014 14:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 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.

I think the right way to do that is to have QEMU's TLB
structure include some sort of general equivalent to the
ASID/VMID mechanism (presumably other target CPUs have some
equivalent). Then we can honour 'flush by ASID' as well.
(We make a forlorn gesture in this direction with the
completely ignored 'flush_global' parameter to tlb_flush().)

This is all definitely 'maybe future' stuff though.)

thanks
-- PMM
Alexander Graf May 20, 2014, 1:51 p.m. UTC | #5 
On 20.05.14 15:47, Peter Maydell wrote:
> On 20 May 2014 14:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>> 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.
> I think the right way to do that is to have QEMU's TLB
> structure include some sort of general equivalent to the
> ASID/VMID mechanism (presumably other target CPUs have some
> equivalent). Then we can honour 'flush by ASID' as well.
> (We make a forlorn gesture in this direction with the
> completely ignored 'flush_global' parameter to tlb_flush().)

I tried to implement this a few years ago, but utterly failed. If 
anyone's interested I can probably dig out the patches I had :).


Alex
Edgar E. Iglesias May 20, 2014, 1:55 p.m. UTC | #6 
On Tue, May 20, 2014 at 02:47:49PM +0100, Peter Maydell wrote:
> On 20 May 2014 14:28, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > 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.
> 
> I think the right way to do that is to have QEMU's TLB
> structure include some sort of general equivalent to the
> ASID/VMID mechanism (presumably other target CPUs have some
> equivalent). Then we can honour 'flush by ASID' as well.
> (We make a forlorn gesture in this direction with the
> completely ignored 'flush_global' parameter to tlb_flush().)

Makes sense.

Cheers,
Edgar

> 
> This is all definitely 'maybe future' stuff though.)
> 
> thanks
> -- PMM
diff mbox

Patch

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 */