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