Message ID | 20221017093003.547009-1-ake@igel.co.jp |
---|---|
State | New |
Headers | show |
Series | target/arm: honor HCR_E2H for AT S1E2R and AT S1E2W address translation | expand |
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
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~
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
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;
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(-)