Minor MMU fixes for PowerPC 40x emulation

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

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

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 :).

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

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

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