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

[Stephane Duverger <stephane.duverger@free.fr>]

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