Re: [Qemu-devel] [PATCH] PPC: Fix large page support in TCG

[Nathan Whitehorn]

On Mar 7, 2012, at 7:25 PM, David Gibson wrote:

> On Sat, Mar 03, 2012 at 10:39:34AM -0600, Nathan Whitehorn wrote:
> > Fix large page support in TCG. The old code would overwrite the
> > large page table entry with the fake 4 KB
> > one generated here whenever the ref/change bits were updated,
> > causing it to point to the wrong area of memory. Instead of creating
> > a fake PTE, just update the real address at the end.
> >
> > Signed-off-by: Nathan Whitehorn <address@hidden>
>
> Hrm.  This looks like a cleaner way of handling things, but I don't
> really follow what exactly was going wrong in the old way.  Can you
> spell out in more detail where the modified pte1 value caused
> problems?

The problem was that pte1 would get extra bits added into it in  
_find_pte() to produce a new, fake 4KB page table entry. When the ref/ 
change bits were updated, pte1 would be written back to the page table  
-- *including* the bits added to make a fake 4K page. At the next  
access, since this function does not clear the low bits of large pages  
(which is probably itself a bug) when it interprets them, the  
generated address would be the large page base, ored with the large  
page remainder for this access, ored with the large page remainder  
from the *previous* access, etc. and you would get a progressively  
more bogus address in the end.
-Nathan

> > ---
> > target-ppc/helper.c |   11 +++++------
> > 1 files changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> > index 928fbcf..0f5ad2e 100644
> > --- a/target-ppc/helper.c
> > +++ b/target-ppc/helper.c
> > @@ -597,12 +597,6 @@ static inline int _find_pte(CPUState *env,
> > mmu_ctx_t *ctx,
> > int is_64b, int h,
> >                 pte1 = ldq_phys(env->htab_base + pteg_off + (i *  
> > 16) + 8);
> >             }
> >
> > -            /* We have a TLB that saves 4K pages, so let's
> > -             * split a huge page to 4k chunks */
> > -            if (target_page_bits != TARGET_PAGE_BITS)
> > -                pte1 |= (ctx->eaddr & (( 1 << target_page_bits ) -  
> > 1))
> > - & TARGET_PAGE_MASK;
> > -
> >             r = pte64_check(ctx, pte0, pte1, h, rw, type);
> >             LOG_MMU("Load pte from " TARGET_FMT_lx " => "
> > TARGET_FMT_lx " "
> >                     TARGET_FMT_lx " %d %d %d " TARGET_FMT_lx "\n",
> > @@ -678,6 +672,11 @@ static inline int _find_pte(CPUState *env,
> > mmu_ctx_t *ctx,
> > int is_64b, int h,
> >         }
> >     }
> >
> > +    /* We have a TLB that saves 4K pages, so let's
> > +     * split a huge page to 4k chunks */
> > +    if (target_page_bits != TARGET_PAGE_BITS)
> > +       ctx->raddr |= (ctx->eaddr & (( 1 << target_page_bits ) - 1))
> > + & TARGET_PAGE_MASK;
> >     return ret;
> > }
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_  
> _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson