| Message ID | 201606172208.u5HM8Evh019067@linux02.ddci.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + 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. -Scott
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?
On 06/19/2016 09:13 PM, david@gibson.dropbear.id.au wrote: > 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? Just the comment. -Scott
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. */ + return ctz32(size >> 10) >> (MAS1_TSIZE_SHIFT - 7); } static void mmubooke_create_initial_mapping(CPUPPCState *env,
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(-)