Message ID | 4CA6C549.9090801@runbox.com |
---|---|
State | New |
Headers | show |
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
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;