PPC: Fix large page support in TCG

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

On 09.03.2012, at 15:30, Nathan Whitehorn wrote:

> 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
> 
> <0005-Fix-large-page-support-in-TCG.-The-old-code-would-ov.patch>

Ok, thanks. I fixed the checkpatch errors:

WARNING: braces {} are necessary for all arms of this statement
#86: FILE: target-ppc/helper.c:677:
+    if (target_page_bits != TARGET_PAGE_BITS)
[...]

ERROR: code indent should never use tabs
#87: FILE: target-ppc/helper.c:678:
+^Ictx->raddr |= (ctx->eaddr & (( 1 << target_page_bits ) - 1))$

ERROR: space prohibited after that open parenthesis '('
#87: FILE: target-ppc/helper.c:678:
+	ctx->raddr |= (ctx->eaddr & (( 1 << target_page_bits ) - 1))

ERROR: space prohibited before that close parenthesis ')'
#87: FILE: target-ppc/helper.c:678:
+	ctx->raddr |= (ctx->eaddr & (( 1 << target_page_bits ) - 1))

ERROR: code indent should never use tabs
#88: FILE: target-ppc/helper.c:679:
+^I^I& TARGET_PAGE_MASK;$

total: 4 errors, 1 warnings, 23 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

and applied the patch to ppc-next.


Alex