| Message ID | 4F524946.1050001@freebsd.org |
|---|---|
| State | New |
| Headers | show |
Am 03.03.2012 17:39, schrieb Nathan Whitehorn: > 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 <nwhitehorn@freebsd.org> cc'ing Alex and qemu-ppc. /-F > --- > 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; > } > > -- > 1.7.9
On 03/03/2012 07:07 PM, Andreas Färber wrote: > Am 03.03.2012 17:39, schrieb Nathan Whitehorn: >> 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<nwhitehorn@freebsd.org> > cc'ing Alex and qemu-ppc. David? Could you please ack? Alex > /-F > >> --- >> 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; >> } >> >> -- >> 1.7.9
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 <nwhitehorn@freebsd.org> 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? > --- > 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; > } >
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 <nwhitehorn@freebsd.org> > > 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
On Thu, Mar 08, 2012 at 09:24:53AM -0600, Nathan Whitehorn wrote: > > 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 <nwhitehorn@freebsd.org> > > > >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. Ah, yes, I see it now. Good catch. Acked-by: David Gibson <david@gibson.drobpear.id.au>
On 09.03.2012, at 04:42, David Gibson wrote: > On Thu, Mar 08, 2012 at 09:24:53AM -0600, Nathan Whitehorn wrote: >> >> 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 <nwhitehorn@freebsd.org> >>> >>> 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. > > Ah, yes, I see it now. Good catch. > > Acked-by: David Gibson <david@gibson.drobpear.id.au> Hrm - the patch doesn't apply for me. Could you please resend as something that's applyable? :) Also, please make sure to always CC qemu-ppc on ppc patches, otherwise there's a good chance they slip off my radar. Alex
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);
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 <nwhitehorn@freebsd.org> --- target-ppc/helper.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-) 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; } -- 1.7.9