On Wed, 9 Dec 2020 10:35:44 +0100
Stephane Duverger <stephane.duver...@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.duver...@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;
