[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation
From: |
Edgar E. Iglesias |
Subject: |
Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation |
Date: |
Sat, 2 Oct 2010 19:06:38 +0200 |
User-agent: |
Mutt/1.5.20 (2009-06-14) |
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
- [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation, John Clark, 2010/10/02
- Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation, Alexander Graf, 2010/10/02
- Message not available
- Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation, Alexander Graf, 2010/10/02
- Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation, John Clark, 2010/10/02
- Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation,
Edgar E. Iglesias <=
- Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation, Alexander Graf, 2010/10/02
- Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation, John Clark, 2010/10/02
- Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation, Alexander Graf, 2010/10/05
- Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation, Edgar E. Iglesias, 2010/10/05