Re: [Qemu-devel] [PATCH] target-ppc: Correct ppc3500_spin initial TLB size

["david@gibson.dropbear.id.au" <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]

On Fri, Jun 17, 2016 at 10:55:47PM +0000, Scott Wood wrote:
> On 06/17/2016 05:13 PM, Aaron Larson wrote:
> > When e500 PPC is booted multi-core, the non-boot cores are started via
> > the spin table.  ppce500_spin.c:spin_kick() calls
> > mmubooke_create_initial_mapping() to allocate a 64MB TLB entry, but
> > the created TLB entry is only 256KB.
> > 
> > The root cause is that the function computing the size of the TLB
> > entry, namely booke206_page_size_to_tlb assumes MAS1.TSIZE as defined
> > by latter PPC cores, specifically (n**4)KB. The result is then used by
> > mmubooke_create_initial_mapping using MAS1_TSIZE_SHIFT, but
> > MAS1_TSIZE_SHIFT is defined assuming TLB entries are (n**2)KB. I.e., a
> > difference of shift=7 or shift=8.
> > 
> > Simply changing MAS1_TSIZE_SHIFT from 7 to 8 is not appropriate since
> > the macro is used elsewhere.
> > 
> > Signed-off-by: Aaron Larson <alarson@ddci.com>
> > ---
> >  hw/ppc/ppce500_spin.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
> > index 76bd78b..7e38f0c 100644
> > --- a/hw/ppc/ppce500_spin.c
> > +++ b/hw/ppc/ppce500_spin.c
> > @@ -75,7 +75,11 @@ static void spin_reset(void *opaque)
> >  /* Create -kernel TLB entries for BookE, linearly spanning 256MB.  */
> >  static inline hwaddr booke206_page_size_to_tlb(uint64_t size)
> >  {
> > -    return ctz32(size >> 10) >> 1;
> > +    /* The EREF indicates that TLB pages are (4 to the power of 2)KB, which
> > +     * corresponds to MAS1_TSIZE_SHIFT=8, but to support legacy processors that
> > +     * assume TLB pages are (2 to the power of 2)KB MAS1_TSIZE_SHIFT is
> > +     * currently 7. */
> 
> This is backwards. It's the old processors that can only handle
> power-of-4 sizes.

To clarify, is this a problem in the code, or just in the comment?

> > +    return ctz32(size >> 10) >> (MAS1_TSIZE_SHIFT - 7);
> 
> The patch that changed MAS1_TSIZE_SHIFT from 8 to 7 was around the same
> time as the patch that added this code, which is probably why adjusting
> it got missed.  Commit 2bd9543cd3 did update the equivalent code in
> ppce500_mpc8544ds.c, which now resides in hw/ppc/e500.c and has been
> changed to not assume a power-of-2 size.  The ppce500_spin version
> should be eliminated.

Sounds sensible.

Aaron, for some reason I got multiple copies of your patch mail - a
couple of full ones and then a couple more extras which had 0 size.
Was that just something going wrong with your mailer, or did you
attempt to send a couple of different versions?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]