Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[John Clark]

>>             /* Check from TLB entry */
>> -            /* XXX: there is a problem here or in the TLB fill code... */
>> +            /* There is no longer a need to force PAGE_EXEC permission here */
>> +            /* because of the tlb->attr fix in helper_4xx_tlbwe_lo() */
> 
> I guess that comment is superfluous, as readers several years from now don't care what was broken back in the day :).

Yes, I suppose so :)

>> @@ -3939,7 +3939,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>>     tlb = &env->tlb[entry].tlbe;
>>     ret = tlb->EPN;
>>     if (tlb->prot & PAGE_VALID)
>> -        ret |= 0x400;
>> +        ret |= 0x40;    /* V bit is 0x40, not 0x400 */
> 
> Ouch. Mind to make it a define?

Sure, I was surprised that there wasn't a define for that when I found it.

>>     size = booke_page_size_to_tlb(tlb->size);
>>     if (size < 0 || size > 0x7)
>>         size = 1;
>> @@ -3948,7 +3948,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>>     return ret;
>> }
>>
>> -target_ulong helper_4xx_tlbre_hi (target_ulong entry)
>> +target_ulong helper_4xx_tlbre_lo (target_ulong entry)
> 
> Huh?

To summarize, 'tlbre' has two forms: one to retrieve the high bits of
a TLB entry (TLBHI), and one to retrieve the low bits (TLBLO) of a TLB
entry.  This code had the TLBLO form returning the bits corresponding
to TLBHI and vice versa, hence the name change.  You can verify this
if you like with this IBM PowerPC 405 core user manual on page 362:

https://www-01.ibm.com/chips/techlib/techlib.nsf/techdocs/D060DB54BD4DC4F2872569D2004A30D6/$file/ppc405fx_um.pdf

Thanks.

- John

[Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[John Clark]

Hello,

I found I had to make a few minor changes to the MMU code for the
PowerPC 40x emulation to get NetBSD to run on a virtual PowerPC 405
core with qemu-system-ppcemb. The 'tlbre' instruction was not working,
and permission checking for a TLB entry was not as strict as it should
be. Diffs are included below.

Thank you.

- John Clark

diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 3bc8a34..a8c1802 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -1172,9 +1172,9 @@ static int mmu40x_get_physical_address (CPUState *env, mmu_ctx_t *ctx,
         case 0x1:
         check_perms:
             /* Check from TLB entry */
-            /* XXX: there is a problem here or in the TLB fill code... */
+            /* There is no longer a need to force PAGE_EXEC permission here */
+            /* because of the tlb->attr fix in helper_4xx_tlbwe_lo() */
             ctx->prot = tlb->prot;
-            ctx->prot |= PAGE_EXEC;
             ret = check_prot(ctx->prot, rw, access_type);
             if (ret == -2)
                 env->spr[SPR_40x_ESR] = 0;
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 3e6db85..54356e8 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -3929,7 +3929,7 @@ static inline int booke_page_size_to_tlb(target_ulong page_size)
 }
 
 /* Helpers for 4xx TLB management */
-target_ulong helper_4xx_tlbre_lo (target_ulong entry)
+target_ulong helper_4xx_tlbre_hi (target_ulong entry)
 {
     ppcemb_tlb_t *tlb;
     target_ulong ret;
@@ -3939,7 +3939,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
     tlb = &env->tlb[entry].tlbe;
     ret = tlb->EPN;
     if (tlb->prot & PAGE_VALID)
-        ret |= 0x400;
+        ret |= 0x40;    /* V bit is 0x40, not 0x400 */
     size = booke_page_size_to_tlb(tlb->size);
     if (size < 0 || size > 0x7)
         size = 1;
@@ -3948,7 +3948,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
     return ret;
 }
 
-target_ulong helper_4xx_tlbre_hi (target_ulong entry)
+target_ulong helper_4xx_tlbre_lo (target_ulong entry)
 {
     ppcemb_tlb_t *tlb;
     target_ulong ret;

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[Alexander Graf]

On 02.10.2010, at 07:38, John Clark wrote:

> Hello,
> 
> I found I had to make a few minor changes to the MMU code for the
> PowerPC 40x emulation to get NetBSD to run on a virtual PowerPC 405
> core with qemu-system-ppcemb. The 'tlbre' instruction was not working,
> and permission checking for a TLB entry was not as strict as it should
> be. Diffs are included below.
> 
> Thank you.
> 
> - John Clark
> 
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 3bc8a34..a8c1802 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -1172,9 +1172,9 @@ static int mmu40x_get_physical_address (CPUState *env, mmu_ctx_t *ctx,
>         case 0x1:
>         check_perms:
>             /* Check from TLB entry */
> -            /* XXX: there is a problem here or in the TLB fill code... */
> +            /* There is no longer a need to force PAGE_EXEC permission here */
> +            /* because of the tlb->attr fix in helper_4xx_tlbwe_lo() */

I guess that comment is superfluous, as readers several years from now don't care what was broken back in the day :).

>             ctx->prot = tlb->prot;
> -            ctx->prot |= PAGE_EXEC;
>             ret = check_prot(ctx->prot, rw, access_type);
>             if (ret == -2)
>                 env->spr[SPR_40x_ESR] = 0;
> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> index 3e6db85..54356e8 100644
> --- a/target-ppc/op_helper.c
> +++ b/target-ppc/op_helper.c
> @@ -3929,7 +3929,7 @@ static inline int booke_page_size_to_tlb(target_ulong page_size)
> }
> 
> /* Helpers for 4xx TLB management */
> -target_ulong helper_4xx_tlbre_lo (target_ulong entry)
> +target_ulong helper_4xx_tlbre_hi (target_ulong entry)
> {
>     ppcemb_tlb_t *tlb;
>     target_ulong ret;
> @@ -3939,7 +3939,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>     tlb = &env->tlb[entry].tlbe;
>     ret = tlb->EPN;
>     if (tlb->prot & PAGE_VALID)
> -        ret |= 0x400;
> +        ret |= 0x40;    /* V bit is 0x40, not 0x400 */

Ouch. Mind to make it a define?

>     size = booke_page_size_to_tlb(tlb->size);
>     if (size < 0 || size > 0x7)
>         size = 1;
> @@ -3948,7 +3948,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>     return ret;
> }
> 
> -target_ulong helper_4xx_tlbre_hi (target_ulong entry)
> +target_ulong helper_4xx_tlbre_lo (target_ulong entry)

Huh?


Alex

> {
>     ppcemb_tlb_t *tlb;
>     target_ulong ret;
> 

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[Alexander Graf]

On 02.10.2010, at 18:49, John Clark wrote:

>>>            /* Check from TLB entry */
>>> -            /* XXX: there is a problem here or in the TLB fill code... */
>>> +            /* There is no longer a need to force PAGE_EXEC permission here */
>>> +            /* because of the tlb->attr fix in helper_4xx_tlbwe_lo() */
>> 
>> I guess that comment is superfluous, as readers several years from now don't care what was broken back in the day :).
> 
> Yes, I suppose so :)
> 
>>> @@ -3939,7 +3939,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>>>    tlb = &env->tlb[entry].tlbe;
>>>    ret = tlb->EPN;
>>>    if (tlb->prot & PAGE_VALID)
>>> -        ret |= 0x400;
>>> +        ret |= 0x40;    /* V bit is 0x40, not 0x400 */
>> 
>> Ouch. Mind to make it a define?
> 
> Sure, I was surprised that there wasn't a define for that when I found it.

The ppc emulation code lacks a lot of defines. In fact, the same goes for x86 emulation too ;). But that doesn't mean we have to keep it that way!

> 
>>>    size = booke_page_size_to_tlb(tlb->size);
>>>    if (size < 0 || size > 0x7)
>>>        size = 1;
>>> @@ -3948,7 +3948,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>>>    return ret;
>>> }
>>> 
>>> -target_ulong helper_4xx_tlbre_hi (target_ulong entry)
>>> +target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>> 
>> Huh?
> 
> To summarize, 'tlbre' has two forms: one to retrieve the high bits of
> a TLB entry (TLBHI), and one to retrieve the low bits (TLBLO) of a TLB
> entry.  This code had the TLBLO form returning the bits corresponding
> to TLBHI and vice versa, hence the name change.  You can verify this
> if you like with this IBM PowerPC 405 core user manual on page 362:

Well the thing that strikes me as weird is mostly that you're changing a function name, but no callers to it. So is this function never used? Or was tlbre_lo defined before already and is now defined twice?

Alex

PS: Please use the "reply to all" function of your mailer. Others might be interested in the reply too :).

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[John Clark]

>>>>
>>>> -target_ulong helper_4xx_tlbre_hi (target_ulong entry)
>>>> +target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>>>
>>> Huh?
>>
>> To summarize, 'tlbre' has two forms: one to retrieve the high bits of
>> a TLB entry (TLBHI), and one to retrieve the low bits (TLBLO) of a TLB
>> entry.  This code had the TLBLO form returning the bits corresponding
>> to TLBHI and vice versa, hence the name change.  You can verify this
>> if you like with this IBM PowerPC 405 core user manual on page 362:
> 
> Well the thing that strikes me as weird is mostly that you're changing a function name, but no callers to it. So is this function never used? Or was tlbre_lo defined before already and is now defined twice?

You'll see that helper_4xx_tlbre_hi changes to helper_4xx_tlbre_lo and
that helper_4xx_tlbre_lo changes to helper_4xx_tlbre_hi, so
helper_4xx_tlbre_lo is not multiply defined.

> 
> PS: Please use the "reply to all" function of your mailer. Others might be interested in the reply too :).
> 

Yes, I realized my mistake a few minutes after sending the previous
reply and corrected it.

- John

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[Edgar E. Iglesias]

On Sat, Oct 02, 2010 at 06:55:36PM +0200, Alexander Graf wrote:
> 
> On 02.10.2010, at 18:49, John Clark wrote:
> 
> >>>            /* Check from TLB entry */
> >>> -            /* XXX: there is a problem here or in the TLB fill code... */
> >>> +            /* There is no longer a need to force PAGE_EXEC permission here */
> >>> +            /* because of the tlb->attr fix in helper_4xx_tlbwe_lo() */
> >> 
> >> I guess that comment is superfluous, as readers several years from now don't care what was broken back in the day :).
> > 
> > Yes, I suppose so :)
> > 
> >>> @@ -3939,7 +3939,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
> >>>    tlb = &env->tlb[entry].tlbe;
> >>>    ret = tlb->EPN;
> >>>    if (tlb->prot & PAGE_VALID)
> >>> -        ret |= 0x400;
> >>> +        ret |= 0x40;    /* V bit is 0x40, not 0x400 */
> >> 
> >> Ouch. Mind to make it a define?
> > 
> > Sure, I was surprised that there wasn't a define for that when I found it.
> 
> The ppc emulation code lacks a lot of defines. In fact, the same goes for x86 emulation too ;). But that doesn't mean we have to keep it that way!
> 
> > 
> >>>    size = booke_page_size_to_tlb(tlb->size);
> >>>    if (size < 0 || size > 0x7)
> >>>        size = 1;
> >>> @@ -3948,7 +3948,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
> >>>    return ret;
> >>> }
> >>> 
> >>> -target_ulong helper_4xx_tlbre_hi (target_ulong entry)
> >>> +target_ulong helper_4xx_tlbre_lo (target_ulong entry)
> >> 
> >> Huh?
> > 
> > To summarize, 'tlbre' has two forms: one to retrieve the high bits of
> > a TLB entry (TLBHI), and one to retrieve the low bits (TLBLO) of a TLB
> > entry.  This code had the TLBLO form returning the bits corresponding
> > to TLBHI and vice versa, hence the name change.  You can verify this
> > if you like with this IBM PowerPC 405 core user manual on page 362:
> 
> Well the thing that strikes me as weird is mostly that you're changing a function name, but no callers to it. So is this function never used? Or was tlbre_lo defined before already and is now defined twice?

Hi,

Alex:
I think you've missed the part of the patch that renames the _lo -> _hi.
As John says, qemu had the hi/lo parts reversed when reading 4xx TLB regs.

Except for the comments and the define, the patch looks good to me.
John, please also add a Signed-off-by line.

Cheers

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[Alexander Graf]

On 02.10.2010, at 19:06, Edgar E. Iglesias wrote:

> On Sat, Oct 02, 2010 at 06:55:36PM +0200, Alexander Graf wrote:
>> 
>> On 02.10.2010, at 18:49, John Clark wrote:
>> 
>>>>>           /* Check from TLB entry */
>>>>> -            /* XXX: there is a problem here or in the TLB fill code... */
>>>>> +            /* There is no longer a need to force PAGE_EXEC permission here */
>>>>> +            /* because of the tlb->attr fix in helper_4xx_tlbwe_lo() */
>>>> 
>>>> I guess that comment is superfluous, as readers several years from now don't care what was broken back in the day :).
>>> 
>>> Yes, I suppose so :)
>>> 
>>>>> @@ -3939,7 +3939,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>>>>>   tlb = &env->tlb[entry].tlbe;
>>>>>   ret = tlb->EPN;
>>>>>   if (tlb->prot & PAGE_VALID)
>>>>> -        ret |= 0x400;
>>>>> +        ret |= 0x40;    /* V bit is 0x40, not 0x400 */
>>>> 
>>>> Ouch. Mind to make it a define?
>>> 
>>> Sure, I was surprised that there wasn't a define for that when I found it.
>> 
>> The ppc emulation code lacks a lot of defines. In fact, the same goes for x86 emulation too ;). But that doesn't mean we have to keep it that way!
>> 
>>> 
>>>>>   size = booke_page_size_to_tlb(tlb->size);
>>>>>   if (size < 0 || size > 0x7)
>>>>>       size = 1;
>>>>> @@ -3948,7 +3948,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>>>>>   return ret;
>>>>> }
>>>>> 
>>>>> -target_ulong helper_4xx_tlbre_hi (target_ulong entry)
>>>>> +target_ulong helper_4xx_tlbre_lo (target_ulong entry)
>>>> 
>>>> Huh?
>>> 
>>> To summarize, 'tlbre' has two forms: one to retrieve the high bits of
>>> a TLB entry (TLBHI), and one to retrieve the low bits (TLBLO) of a TLB
>>> entry.  This code had the TLBLO form returning the bits corresponding
>>> to TLBHI and vice versa, hence the name change.  You can verify this
>>> if you like with this IBM PowerPC 405 core user manual on page 362:
>> 
>> Well the thing that strikes me as weird is mostly that you're changing a function name, but no callers to it. So is this function never used? Or was tlbre_lo defined before already and is now defined twice?
> 
> Hi,
> 
> Alex:
> I think you've missed the part of the patch that renames the _lo -> _hi.
> As John says, qemu had the hi/lo parts reversed when reading 4xx TLB regs.

Oh. THERE it is! Hah. Yeah, I really missed that line - exchanging both functions makes sense.


Thanks,

Alex

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[John Clark]

> Hi,
> 
> Alex:
> I think you've missed the part of the patch that renames the _lo -> _hi.
> As John says, qemu had the hi/lo parts reversed when reading 4xx TLB regs.
> 
> Except for the comments and the define, the patch looks good to me.
> John, please also add a Signed-off-by line.
> 
> Cheers

Ok, I have removed the superfluous comments, added defines for magic
numbers, added braces to conform to the QEMU coding style guidelines,
and regression tested it in my system.  Patch included below.

Thanks.

- John

Signed-off-by: John Clark <clarkjc@runbox.com>
--
 target-ppc/helper.c    |    2 -
 target-ppc/op_helper.c |   74 +++++++++++++++++++++++++++++++----------------
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 3bc8a34..edbdd80 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -1172,9 +1172,7 @@ static int mmu40x_get_physical_address (CPUState *env, mmu_ctx_t *ctx,
         case 0x1:
         check_perms:
             /* Check from TLB entry */
-            /* XXX: there is a problem here or in the TLB fill code... */
             ctx->prot = tlb->prot;
-            ctx->prot |= PAGE_EXEC;
             ret = check_prot(ctx->prot, rw, access_type);
             if (ret == -2)
                 env->spr[SPR_40x_ESR] = 0;
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 3e6db85..45f1655 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -3929,37 +3929,56 @@ static inline int booke_page_size_to_tlb(target_ulong page_size)
 }
 
 /* Helpers for 4xx TLB management */
-target_ulong helper_4xx_tlbre_lo (target_ulong entry)
+#define PPC4XX_TLB_ENTRY_MASK       0x0000003f  /* Mask for 64 TLB entries */
+
+#define PPC4XX_TLBHI_V              0x00000040
+#define PPC4XX_TLBHI_E              0x00000020
+#define PPC4XX_TLBHI_SIZE_MIN       0
+#define PPC4XX_TLBHI_SIZE_MAX       7
+#define PPC4XX_TLBHI_SIZE_DEFAULT   1
+#define PPC4XX_TLBHI_SIZE_SHIFT     7
+#define PPC4XX_TLBHI_SIZE_MASK      0x00000007
+
+#define PPC4XX_TLBLO_EX             0x00000200
+#define PPC4XX_TLBLO_WR             0x00000100
+#define PPC4XX_TLBLO_ATTR_MASK      0x000000FF
+#define PPC4XX_TLBLO_RPN_MASK       0xFFFFFC00
+
+target_ulong helper_4xx_tlbre_hi (target_ulong entry)
 {
     ppcemb_tlb_t *tlb;
     target_ulong ret;
     int size;
 
-    entry &= 0x3F;
+    entry &= PPC4XX_TLB_ENTRY_MASK;
     tlb = &env->tlb[entry].tlbe;
     ret = tlb->EPN;
-    if (tlb->prot & PAGE_VALID)
-        ret |= 0x400;
+    if (tlb->prot & PAGE_VALID) {
+        ret |= PPC4XX_TLBHI_V;
+    }
     size = booke_page_size_to_tlb(tlb->size);
-    if (size < 0 || size > 0x7)
-        size = 1;
-    ret |= size << 7;
+    if (size < PPC4XX_TLBHI_SIZE_MIN || size > PPC4XX_TLBHI_SIZE_MAX) {
+        size = PPC4XX_TLBHI_SIZE_DEFAULT;
+    }
+    ret |= size << PPC4XX_TLBHI_SIZE_SHIFT;
     env->spr[SPR_40x_PID] = tlb->PID;
     return ret;
 }
 
-target_ulong helper_4xx_tlbre_hi (target_ulong entry)
+target_ulong helper_4xx_tlbre_lo (target_ulong entry)
 {
     ppcemb_tlb_t *tlb;
     target_ulong ret;
 
-    entry &= 0x3F;
+    entry &= PPC4XX_TLB_ENTRY_MASK;
     tlb = &env->tlb[entry].tlbe;
     ret = tlb->RPN;
-    if (tlb->prot & PAGE_EXEC)
-        ret |= 0x200;
-    if (tlb->prot & PAGE_WRITE)
-        ret |= 0x100;
+    if (tlb->prot & PAGE_EXEC) {
+        ret |= PPC4XX_TLBLO_EX;
+    }
+    if (tlb->prot & PAGE_WRITE) {
+        ret |= PPC4XX_TLBLO_WR;
+    }
     return ret;
 }
 
@@ -3970,30 +3989,32 @@ void helper_4xx_tlbwe_hi (target_ulong entry, target_ulong val)
 
     LOG_SWTLB("%s entry %d val " TARGET_FMT_lx "\n", __func__, (int)entry,
               val);
-    entry &= 0x3F;
+    entry &= PPC4XX_TLB_ENTRY_MASK;
     tlb = &env->tlb[entry].tlbe;
     /* Invalidate previous TLB (if it's valid) */
     if (tlb->prot & PAGE_VALID) {
         end = tlb->EPN + tlb->size;
         LOG_SWTLB("%s: invalidate old TLB %d start " TARGET_FMT_lx " end "
                   TARGET_FMT_lx "\n", __func__, (int)entry, tlb->EPN, end);
-        for (page = tlb->EPN; page < end; page += TARGET_PAGE_SIZE)
+        for (page = tlb->EPN; page < end; page += TARGET_PAGE_SIZE) {
             tlb_flush_page(env, page);
+        }
     }
-    tlb->size = booke_tlb_to_page_size((val >> 7) & 0x7);
+    tlb->size = booke_tlb_to_page_size((val >> PPC4XX_TLBHI_SIZE_SHIFT)
+                                       & PPC4XX_TLBHI_SIZE_MASK);
     /* We cannot handle TLB size < TARGET_PAGE_SIZE.
      * If this ever occurs, one should use the ppcemb target instead
      * of the ppc or ppc64 one
      */
-    if ((val & 0x40) && tlb->size < TARGET_PAGE_SIZE) {
+    if ((val & PPC4XX_TLBHI_V) && tlb->size < TARGET_PAGE_SIZE) {
         cpu_abort(env, "TLB size " TARGET_FMT_lu " < %u "
                   "are not supported (%d)\n",
                   tlb->size, TARGET_PAGE_SIZE, (int)((val >> 7) & 0x7));
     }
     tlb->EPN = val & ~(tlb->size - 1);
-    if (val & 0x40) {
+    if (val & PPC4XX_TLBHI_V) {
         tlb->prot |= PAGE_VALID;
-        if (val & 0x20) {
+        if (val & PPC4XX_TLBHI_E) {
             /* XXX: TO BE FIXED */
             cpu_abort(env,
                       "Little-endian TLB entries are not supported by now\n");
@@ -4014,8 +4035,9 @@ void helper_4xx_tlbwe_hi (target_ulong entry, target_ulong val)
         end = tlb->EPN + tlb->size;
         LOG_SWTLB("%s: invalidate TLB %d start " TARGET_FMT_lx " end "
                   TARGET_FMT_lx "\n", __func__, (int)entry, tlb->EPN, end);
-        for (page = tlb->EPN; page < end; page += TARGET_PAGE_SIZE)
+        for (page = tlb->EPN; page < end; page += TARGET_PAGE_SIZE) {
             tlb_flush_page(env, page);
+        }
     }
 }
 
@@ -4025,15 +4047,17 @@ void helper_4xx_tlbwe_lo (target_ulong entry, target_ulong val)
 
     LOG_SWTLB("%s entry %i val " TARGET_FMT_lx "\n", __func__, (int)entry,
               val);
-    entry &= 0x3F;
+    entry &= PPC4XX_TLB_ENTRY_MASK;
     tlb = &env->tlb[entry].tlbe;
-    tlb->attr = val & 0xFF;
-    tlb->RPN = val & 0xFFFFFC00;
+    tlb->attr = val & PPC4XX_TLBLO_ATTR_MASK;
+    tlb->RPN = val & PPC4XX_TLBLO_RPN_MASK;
     tlb->prot = PAGE_READ;
-    if (val & 0x200)
+    if (val & PPC4XX_TLBLO_EX) {
         tlb->prot |= PAGE_EXEC;
-    if (val & 0x100)
+    }
+    if (val & PPC4XX_TLBLO_WR) {
         tlb->prot |= PAGE_WRITE;
+    }
     LOG_SWTLB("%s: set up TLB %d RPN " TARGET_FMT_plx " EPN " TARGET_FMT_lx
               " size " TARGET_FMT_lx " prot %c%c%c%c PID %d\n", __func__,
               (int)entry, tlb->RPN, tlb->EPN, tlb->size,
--

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[Alexander Graf]

On 02.10.2010, at 20:17, John Clark wrote:

>> Hi,
>> 
>> Alex:
>> I think you've missed the part of the patch that renames the _lo -> _hi.
>> As John says, qemu had the hi/lo parts reversed when reading 4xx TLB regs.
>> 
>> Except for the comments and the define, the patch looks good to me.
>> John, please also add a Signed-off-by line.
>> 
>> Cheers
> 
> Ok, I have removed the superfluous comments, added defines for magic
> numbers, added braces to conform to the QEMU coding style guidelines,
> and regression tested it in my system.  Patch included below.
> 
> Thanks.
> 
> - John
> 
> Signed-off-by: John Clark <clarkjc@runbox.com>

Signed-off-by: Alexander Graf <agraf@suse.de>

Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation

[Edgar E. Iglesias]

On Tue, Oct 05, 2010 at 06:42:34PM +0200, Alexander Graf wrote:
> 
> On 02.10.2010, at 20:17, John Clark wrote:
> 
> >> Hi,
> >> 
> >> Alex:
> >> I think you've missed the part of the patch that renames the _lo -> _hi.
> >> As John says, qemu had the hi/lo parts reversed when reading 4xx TLB regs.
> >> 
> >> Except for the comments and the define, the patch looks good to me.
> >> John, please also add a Signed-off-by line.
> >> 
> >> Cheers
> > 
> > Ok, I have removed the superfluous comments, added defines for magic
> > numbers, added braces to conform to the QEMU coding style guidelines,
> > and regression tested it in my system.  Patch included below.
> > 
> > Thanks.
> > 
> > - John
> > 
> > Signed-off-by: John Clark <clarkjc@runbox.com>
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Applied, thanks John and Alex.

Cheers