[PATCH] target/arm: honor HCR_E2H for AT S1E2R and AT S1E2W address translation

[PATCH] target/arm: honor HCR_E2H for AT S1E2R and AT S1E2W address translation

[Ake Koomsin]

When HCR_E2H is set, AT S1E2R and AT S1E2W should translate an address
based on both TTBR0_EL2 and TTBR1_EL2.

Signed-off-by: Ake Koomsin <ake@igel.co.jp>
---
 target/arm/helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index dde64a487a..147f96e752 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3486,7 +3486,12 @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
             }
             break;
         case 4: /* AT S1E2R, AT S1E2W */
-            mmu_idx = ARMMMUIdx_E2;
+            if (arm_hcr_el2_eff(env) & HCR_E2H) {
+                mmu_idx = env->pstate & PSTATE_PAN ?
+                    ARMMMUIdx_E20_2_PAN : ARMMMUIdx_E20_2;
+            } else {
+                mmu_idx = ARMMMUIdx_E2;
+            }
             break;
         case 6: /* AT S1E3R, AT S1E3W */
             mmu_idx = ARMMMUIdx_E3;
-- 
2.25.1

Re: [PATCH] target/arm: honor HCR_E2H for AT S1E2R and AT S1E2W address translation

[Peter Maydell]

On Mon, 17 Oct 2022 at 10:30, Ake Koomsin <ake@igel.co.jp> wrote:
>
> When HCR_E2H is set, AT S1E2R and AT S1E2W should translate an address
> based on both TTBR0_EL2 and TTBR1_EL2.
>
> Signed-off-by: Ake Koomsin <ake@igel.co.jp>
> ---
>  target/arm/helper.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index dde64a487a..147f96e752 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3486,7 +3486,12 @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
>              }
>              break;
>          case 4: /* AT S1E2R, AT S1E2W */
> -            mmu_idx = ARMMMUIdx_E2;
> +            if (arm_hcr_el2_eff(env) & HCR_E2H) {
> +                mmu_idx = env->pstate & PSTATE_PAN ?
> +                    ARMMMUIdx_E20_2_PAN : ARMMMUIdx_E20_2;
> +            } else {
> +                mmu_idx = ARMMMUIdx_E2;
> +            }
>              break;

I agree that the AT insns should be handling E2H, but I'm not sure this
is the right fix, and with Richard's recent refactorings I've
kind of lost track of what all our MMUIdxes do.

In the pseudocode, E2H is handled by changing the behaviour not
just of the S1E2 ops, but also of the S1E1 ops. If E2H is set:
 * the S1E2 ops use the EL2&0 regime, but continue to ignore
   PSTATE.PAN
 * the S1E1 ops also use the EL2&0 regime, with the S1E1RP and
   S1E1WP ops looking at PSTATE.PAN and the others not

Richard -- do we want to just do the same thing, or do
our MMUIdx uses differ from the architectural translation
regimes in a way that means we need to do something else?

thanks
-- PMM

Re: [PATCH] target/arm: honor HCR_E2H for AT S1E2R and AT S1E2W address translation

[Richard Henderson]

On 10/25/22 22:25, Peter Maydell wrote:
> In the pseudocode, E2H is handled by changing the behaviour not
> just of the S1E2 ops, but also of the S1E1 ops. If E2H is set:
>   * the S1E2 ops use the EL2&0 regime, but continue to ignore
>     PSTATE.PAN
>   * the S1E1 ops also use the EL2&0 regime, with the S1E1RP and
>     S1E1WP ops looking at PSTATE.PAN and the others not
> 
> Richard -- do we want to just do the same thing, or do
> our MMUIdx uses differ from the architectural translation
> regimes in a way that means we need to do something else?

We want to do the same thing.  The *_PAN MMUIdx are intended to be exactly like the 
architecture, when PAN is enabled and not ignored.


r~

Re: [PATCH] target/arm: honor HCR_E2H for AT S1E2R and AT S1E2W address translation

[ake]

On Tue, 25 Oct 2022 13:25:22 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 17 Oct 2022 at 10:30, Ake Koomsin <ake@igel.co.jp> wrote:
> >
> > When HCR_E2H is set, AT S1E2R and AT S1E2W should translate an
> > address based on both TTBR0_EL2 and TTBR1_EL2.
> >
> > Signed-off-by: Ake Koomsin <ake@igel.co.jp>
> > ---
> >  target/arm/helper.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index dde64a487a..147f96e752 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -3486,7 +3486,12 @@ static void ats_write64(CPUARMState *env,
> > const ARMCPRegInfo *ri, }
> >              break;
> >          case 4: /* AT S1E2R, AT S1E2W */
> > -            mmu_idx = ARMMMUIdx_E2;
> > +            if (arm_hcr_el2_eff(env) & HCR_E2H) {
> > +                mmu_idx = env->pstate & PSTATE_PAN ?
> > +                    ARMMMUIdx_E20_2_PAN : ARMMMUIdx_E20_2;
> > +            } else {
> > +                mmu_idx = ARMMMUIdx_E2;
> > +            }
> >              break;
> 
> I agree that the AT insns should be handling E2H, but I'm not sure
> this is the right fix, and with Richard's recent refactorings I've
> kind of lost track of what all our MMUIdxes do.
> 
> In the pseudocode, E2H is handled by changing the behaviour not
> just of the S1E2 ops, but also of the S1E1 ops. If E2H is set:
>  * the S1E2 ops use the EL2&0 regime, but continue to ignore
>    PSTATE.PAN
>  * the S1E1 ops also use the EL2&0 regime, with the S1E1RP and
>    S1E1WP ops looking at PSTATE.PAN and the others not
> 
> Richard -- do we want to just do the same thing, or do
> our MMUIdx uses differ from the architectural translation
> regimes in a way that means we need to do something else?
> 
> thanks
> -- PMM

I will re-read the AT instruction pseudocode and will come back with a
better fix. It might take a while as I am not familiar with the code
base. Thank you very much for comments.

---
Ake Koomsin