Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] booke206: fix MAS update on tlb miss

[KONRAD Frederic]

On 08/03/2017 03:13 PM, Thomas Huth wrote:
> On 03.08.2017 14:08, KONRAD Frederic wrote:
> > On 08/03/2017 01:37 PM, Thomas Huth wrote:
> > > On 01.08.2017 10:44, KONRAD Frederic wrote:
> > > > When a tlb instruction miss happen, rw is set to 0 at the bottom
> > > > of cpu_ppc_handle_mmu_fault which cause the MAS update function to miss
> > > > the SAS and TS bit in MAS6, MAS1 in booke206_update_mas_tlb_miss.
> > > >
> > > > Just calling booke206_update_mas_tlb_miss with rw = 2 solve the issue.
> > > >
> > > > Signed-off-by: KONRAD Frederic <address@hidden>
> > > > ---
> > > >    target/ppc/mmu_helper.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> > > > index b7b9088..f06b938 100644
> > > > --- a/target/ppc/mmu_helper.c
> > > > +++ b/target/ppc/mmu_helper.c
> > > > @@ -1551,7 +1551,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState
> > > > *env, target_ulong address,
> > > >                        env->spr[SPR_40x_ESR] = 0x00000000;
> > > >                        break;
> > > >                    case POWERPC_MMU_BOOKE206:
> > > > -                    booke206_update_mas_tlb_miss(env, address, rw);
> > > > +                    booke206_update_mas_tlb_miss(env, address, 2);
> > Hi Thomas,
> >
> > > Couldn't that code path be called for normal data read miss (instead of
> > > instruction miss), too?
> >
> > I don't think so because we have access_type == ACCESS_CODE and
> > the code in cpu_ppc_handle_mmu_fault explicitely split the CODE
> > and DATA cases.
>
> Ah, right, I missed that if-statement. So never mind about my comment!
>
> > > Anyway, could we please use MMU_INST_FETCH instead of magic values like
> > > 2 here?
> >
> > I agree it's not nice to have a magic value like this.. But it's
> > used all over the code there and david took the patch.
> >
> > So I suggest I send a second patch to fix all the instances of
> > that magic value.
>
> Sounds like a good idea!

Hi Thomas,

Looking more in details at this magic value, it seems rw is
not considered as an access type but more as an is_write boolean.

The whole mmu code use "int access_type" which is kind of
redundant / confusing if we change rw to be MMUAccessType.

Fred

>   Thanks,
>    Thomas