Re: [Qemu-devel] [PATCH v2 12/22] target/openrisc: Fix tlb flushing in mtspr

[Stafford Horne]

On Fri, Jun 22, 2018 at 3:40 PM Stafford Horne <address@hidden> wrote:
>
> On Mon, Jun 18, 2018 at 08:40:36AM -1000, Richard Henderson wrote:
> > The previous code was confused, avoiding the flush of the old entry
> > if the new entry is invalid.  We need to flush the old page if the
> > old entry is valid and the new page if the new entry is valid.
> >
> > This bug was masked by over-flushing elsewhere.
> >
> > Signed-off-by: Richard Henderson <address@hidden>
> > ---
> >  target/openrisc/sys_helper.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
> > index 8ad7a7d898..e00aaa332e 100644
> > --- a/target/openrisc/sys_helper.c
> > +++ b/target/openrisc/sys_helper.c
> > @@ -32,6 +32,7 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong 
> > spr, target_ulong rb)
> >  #ifndef CONFIG_USER_ONLY
> >      OpenRISCCPU *cpu = openrisc_env_get_cpu(env);
> >      CPUState *cs = CPU(cpu);
> > +    target_ulong mr;
> >      int idx;
> >
> >      switch (spr) {
> > @@ -84,12 +85,15 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong 
> > spr, target_ulong rb)
> >
> >      case TO_SPR(1, 512) ... TO_SPR(1, 512+DTLB_SIZE-1): /* DTLBW0MR 0-127 
> > */
> >          idx = spr - TO_SPR(1, 512);
> > -        if (!(rb & 1)) {
> > -            tlb_flush_page(cs, env->tlb.dtlb[idx].mr & TARGET_PAGE_MASK);
> > +        mr = env->tlb.dtlb[idx].mr;
> > +        if (mr & 1) {
> > +            tlb_flush_page(cs, mr & TARGET_PAGE_MASK);
> > +        }
> > +        if (rb & 1) {
> > +            tlb_flush_page(cs, rb & TARGET_PAGE_MASK);
>
> Hi Richard,
>
> On openrisc we write 0 to the TLB SPR to flush the old entry out of the TLB, I
> don't see much documentation on this, but this is what is done in the kernel.
> This patch is causing the linux kernel to not boot.
>
> I thought flush is to get rid of the old mapping from the tlb, if we need to 
> do
> it for new mappings too should we do.  Why do we need to flush new mappings?
>
>     /* flush old page if it was valid or we are invalidating */
>     if ((mr & 1) || !(rb & 1)) {
>         tlb_flush_page(cs, mr & TARGET_PAGE_MASK);
>     }
>     /* flush new page if its being entered into tlb */
>     if (rb & 1) {
>         tlb_flush_page(cs, rb & TARGET_PAGE_MASK);
>     }
>
> I didn't write the original code, but I think it was right as is.
>

Ignore this for now your code makes more sense, maybe we don't need to
flush the new page though?  This is causing a failure but something
more interesting is failing with the next patch "target/openrisc: Fix
cpu_mmu_index".

-Stafford