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