Re: [PATCH v2 31/66] target/arm: Fix S2 disabled check in S1_ptw_translate

[Peter Maydell]

On Mon, 22 Aug 2022 at 17:23, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Pass the correct stage2 mmu_idx to regime_translation_disabled,
> which we computed afterward.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/ptw.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index dbe5852af6..680139b478 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -211,11 +211,10 @@ static hwaddr S1_ptw_translate(CPUARMState *env, 
> ARMMMUIdx mmu_idx,
>                                 ARMMMUFaultInfo *fi)
>  {
>      bool is_secure = *is_secure_ptr;
> +    ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
>
>      if (arm_mmu_idx_is_stage1_of_2(mmu_idx) &&
> -        !regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
> -        ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S
> -                                         : ARMMMUIdx_Stage2;
> +        !regime_translation_disabled(env, s2_mmu_idx, is_secure)) {
>          GetPhysAddrResult s2 = {};
>          int ret;


This doesn't actually change the behaviour, though, right?
regime_translation_disabled() at this point in the patchset doesn't
do anything that makes a distinction between Stage2_S and Stage2 AFAICT.
So this is just making the code clearer; we fixed the actual bug in patch 19.

With a tweaked commit message,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Alternatively, pull this patch earlier so it's before patch 19 and
is the one fixing the bug; then patch 19 is only adding the
is_secure argument to regime_translation_disabled() and doesn't
fix the bug in passing. That would be neater but maybe the
patchset reshuffle is too painful so I don't insist on it.

thanks
-- PMM