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

[Nathan Whitehorn <nwhitehorn@freebsd.org>]

[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]

On 03/09/12 07:13, Alexander Graf wrote:
> 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
>
Weird. I've provided it as an attachment, which should hopefully work 
this time.
-Nathan


[-- Attachment #2: 0005-Fix-large-page-support-in-TCG.-The-old-code-would-ov.patch --]
[-- Type: text/plain, Size: 1567 bytes --]

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.

Signed-off-by: Nathan Whitehorn <nwhitehorn@freebsd.org>
---
 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