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

Posted by Ake Koomsin 1 year, 7 months ago

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

Posted by Peter Maydell 1 year, 6 months ago

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

Posted by ake 1 year, 6 months ago

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

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

Posted by Richard Henderson 1 year, 6 months ago

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~