Patchwork PPC: Fix large page support in TCG

login
register
mail settings
Submitter Nathan Whitehorn
Date March 3, 2012, 4:39 p.m.
Message ID <4F524946.1050001@freebsd.org>
Download mbox | patch
Permalink /patch/144450/
State New
Headers show

Comments

Nathan Whitehorn - March 3, 2012, 4:39 p.m.
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
Andreas Färber - March 3, 2012, 6:07 p.m.
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
Alexander Graf - March 7, 2012, 3:41 p.m.
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
David Gibson - March 8, 2012, 1:25 a.m.
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;
>  }
>
Nathan Whitehorn - March 8, 2012, 3:24 p.m.
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
David Gibson - March 9, 2012, 3:42 a.m.
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>
Alexander Graf - March 9, 2012, 1:13 p.m.
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

Patch

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);