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

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

[Stephane Duverger]

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.

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

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;
-- 
2.25.1

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;

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

[Stephane Duverger]

On Wed, Dec 09, 2020 at 02:40:45PM +0100, Greg Kurz wrote:
> 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.

Good to know, I understand your concern :)

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

I run a proprietary embedded OS inside qemu with a board I
developped. It uses a 32 bits PPC with mmu-hash32 implementation. At
some point, I add a slow path memory access throught
helper_be_stl_mmu() that triggered:

mmu-hash32.c:ppc_hash32_direct_store()
{
    switch (env->access_type) {
    ...
    default:
        cpu_abort(cs, "ERROR: instruction should not need "
                  "address translation\n");
}

How come did we lost 'access_type' ? In turn, it was related to:

translate.c:gen_set_access_type()
{
    if (ctx->need_access_type && ctx->access_type != access_type) {
        tcg_gen_movi_i32(cpu_access_type, access_type);
        ctx->access_type = access_type;
    }
}

At TCG level, the 'need_access_type' prevented updating the
'access_type'. And so I ended up in
translate.c:ppc_tr_init_disas_context() and discovered the bug.

Unfortunately, it will be difficult to provide you with a test case as
it depends on the current MMU state configured by the running
firmware.

Maybe you might be able to craft something that triggers a slow-path
memory access through:

ppc_hash32_handle_mmu_fault()
...
    /* 3. Look up the Segment Register */
    sr = env->sr[eaddr >> 28];

    /* 4. Handle direct store segments */
    if (sr & SR32_T) {
        if (ppc_hash32_direct_store(cpu, sr, eaddr, rwx,
                                    &raddr, &prot) == 0) {
...


> 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).

Great. I didn't look few lines below nor other locations using
POWERPC_MMU_64B. Just spotted my initial issue. Maybe it would be more
consistent to trash my patch and submit something fixing both issues.

Feel free to fusion my finding with your and sign-off a new candidate
for the sake of the glory of our dear PPC softmmu :)