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
