Re: [PATCH] ppc/translate: Fix need_access_type for non MMU_64

[Greg Kurz]

On Wed, 9 Dec 2020 10:35:44 +0100
Stephane Duverger <stephane.duverger@free.fr> wrote:

> The 64bits MMU variants have POWERPC_MMU_64 flag and POWERPC_MMU_64B
> is a specific one (POWERPC_MMU_32B with flag POWERPC_MMU_64). As a
> consequence, the original test ignored POWERPC_MMU_32B too.

Hrm... POWERPC_MMU_64B is just the generic MMU model for pre-PowerISA-2.03
64-bit CPUs (ie. PowerPC 970 in QEMU). I don't think the 0x00000001 in
POWERPC_MMU_64B has anything to do with POWERPC_MMU_32B actually, it is
just fortuitous.

FYI the MMU model enum was initially introduced by commit a750fc0b9184:

+    POWERPC_MMU_UNKNOWN    = 0,
+    /* Standard 32 bits PowerPC MMU                            */
+    POWERPC_MMU_32B,
+    /* Standard 64 bits PowerPC MMU                            */
+    POWERPC_MMU_64B,

=> POWERPC_MMU_64B isn't POWERPC_MMU_32B with a flag

> 
> The commit 5f2a625452 targeted hash64 mmu version. And indeed the
> 'mmu-hash64.c' does not use access_type. But 'mmu-hash32.c' does.
> 

But I agree that the 0x00000001 causes the check to be wrong for
CPUs using the POWERPC_MMU_32B MMU model. So your change is clearly
the way to go but the changelog should rather state that it doesn't
make sense to use an MMU model enum as a mask. The POWERPC_MMU_64
flag is to be used to detect 64-bit MMU models, as it is done
everywhere else.

How did you spot this ? Would you have an example that illustrates
how this can break things to share ?

Also, this could have:

Fixes: 5f2a6254522b ("ppc: Don't set access_type on all load/stores on hash64")

Finally, we also have a similar nit a few lines below in the same
function:

    ctx->lazy_tlb_flush = env->mmu_model == POWERPC_MMU_32B
        || env->mmu_model == POWERPC_MMU_601
        || (env->mmu_model & POWERPC_MMU_64B);

This happens to be working because POWERPC_MMU_32B is checked first but
the last check should also be (env->mmu_model & POWERPC_MMU_64).

> Signed-off-by: Stephane Duverger <stephane.duverger@free.fr>
> ---
>  target/ppc/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 54cac0e6a7..b4d0699ce3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7892,7 +7892,7 @@ static void ppc_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>      ctx->insns_flags = env->insns_flags;
>      ctx->insns_flags2 = env->insns_flags2;
>      ctx->access_type = -1;
> -    ctx->need_access_type = !(env->mmu_model & POWERPC_MMU_64B);
> +    ctx->need_access_type = !(env->mmu_model & POWERPC_MMU_64);
>      ctx->le_mode = !!(env->hflags & (1 << MSR_LE));
>      ctx->default_tcg_memop_mask = ctx->le_mode ? MO_LE : MO_BE;
>      ctx->flags = env->flags;