| Message ID | 1296586126-32765-5-git-send-email-shaggy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Josh Boyer |
| Headers | show |
On Feb 1, 2011, at 12:48 PM, Dave Kleikamp wrote:
> Since other OS's may be running on the other cores don't use tlbivax
Are you guys building SMP kernel for use with AMP? Just wondering why you'd be using tlbivax at all.
- k
On Wed, 2011-02-02 at 01:48 -0600, Kumar Gala wrote: > On Feb 1, 2011, at 12:48 PM, Dave Kleikamp wrote: > > > Since other OS's may be running on the other cores don't use tlbivax > > Are you guys building SMP kernel for use with AMP? Just wondering why you'd be using tlbivax at all. Yes, for instance, a 4-core chip could run two 2-way instances. Shaggy
On Tue, Feb 01, 2011 at 12:48:44PM -0600, Dave Kleikamp wrote: > Since other OS's may be running on the other cores don't use tlbivax [snip] > +#ifdef CONFIG_44x > +void __init early_init_mmu_44x(void) > +{ > + unsigned long root = of_get_flat_dt_root(); > + if (of_flat_dt_is_compatible(root, "ibm,47x-AMP")) > + amp = 1; > +} > +#endif /* CONFIG_44x */ A test against a hardcoded compatible string seems a nasty way to do this. Maybe we should define a new boolean property for the root node.
On Thu, 2011-02-03 at 10:08 +1100, David Gibson wrote: > On Tue, Feb 01, 2011 at 12:48:44PM -0600, Dave Kleikamp wrote: > > Since other OS's may be running on the other cores don't use tlbivax > > [snip] > > +#ifdef CONFIG_44x > > +void __init early_init_mmu_44x(void) > > +{ > > + unsigned long root = of_get_flat_dt_root(); > > + if (of_flat_dt_is_compatible(root, "ibm,47x-AMP")) > > + amp = 1; > > +} > > +#endif /* CONFIG_44x */ > > A test against a hardcoded compatible string seems a nasty way to do > this. Maybe we should define a new boolean property for the root > node. I'm not crazy about this string, but I needed something in the device tree to key off of. Freescale has something similar (i.e. MPC8572DS-CAMP), so I chose to follow their example. I'd be happy to replace it with a boolean property. Any objection to just using "amp"? Thanks, Shaggy
On Wed, Feb 02, 2011 at 05:53:59PM -0600, Dave Kleikamp wrote: > On Thu, 2011-02-03 at 10:08 +1100, David Gibson wrote: > > On Tue, Feb 01, 2011 at 12:48:44PM -0600, Dave Kleikamp wrote: > > > Since other OS's may be running on the other cores don't use tlbivax > > > > [snip] > > > +#ifdef CONFIG_44x > > > +void __init early_init_mmu_44x(void) > > > +{ > > > + unsigned long root = of_get_flat_dt_root(); > > > + if (of_flat_dt_is_compatible(root, "ibm,47x-AMP")) > > > + amp = 1; > > > +} > > > +#endif /* CONFIG_44x */ > > > > A test against a hardcoded compatible string seems a nasty way to do > > this. Maybe we should define a new boolean property for the root > > node. > > I'm not crazy about this string, but I needed something in the device > tree to key off of. Freescale has something similar (i.e. > MPC8572DS-CAMP), so I chose to follow their example. I'd be happy to > replace it with a boolean property. Any objection to just using > "amp"? Bit too short, I think. I'd suggest either spelling out 'asymmetric-multiprocessor' or 'cooperative-partition' (a more accurate term, IMO).
On Thu, 2011-02-03 at 16:03 +1100, David Gibson wrote: > On Wed, Feb 02, 2011 at 05:53:59PM -0600, Dave Kleikamp wrote: > > On Thu, 2011-02-03 at 10:08 +1100, David Gibson wrote: > > > On Tue, Feb 01, 2011 at 12:48:44PM -0600, Dave Kleikamp wrote: > > > > Since other OS's may be running on the other cores don't use tlbivax > > > > > > [snip] > > > > +#ifdef CONFIG_44x > > > > +void __init early_init_mmu_44x(void) > > > > +{ > > > > + unsigned long root = of_get_flat_dt_root(); > > > > + if (of_flat_dt_is_compatible(root, "ibm,47x-AMP")) > > > > + amp = 1; > > > > +} > > > > +#endif /* CONFIG_44x */ > > > > > > A test against a hardcoded compatible string seems a nasty way to do > > > this. Maybe we should define a new boolean property for the root > > > node. > > > > I'm not crazy about this string, but I needed something in the device > > tree to key off of. Freescale has something similar (i.e. > > MPC8572DS-CAMP), so I chose to follow their example. I'd be happy to > > replace it with a boolean property. Any objection to just using > > "amp"? > > Bit too short, I think. I'd suggest either spelling out > 'asymmetric-multiprocessor' or 'cooperative-partition' (a more > accurate term, IMO). I could be wrong, but I thought the A stands for Asynchronous, not Asymmetric. I thought Asymmetric means that different types of tasks run on the secondary processors, as on the Cell. Anyway, going with 'cooperative-partition' would avoid that confusion. Shaggy
On Thu, Feb 3, 2011 at 5:15 PM, Dave Kleikamp <shaggy@linux.vnet.ibm.com> wrote: >> Bit too short, I think. I'd suggest either spelling out >> 'asymmetric-multiprocessor' or 'cooperative-partition' (a more >> accurate term, IMO). > > I could be wrong, but I thought the A stands for Asynchronous, not > Asymmetric. I thought Asymmetric means that different types of tasks > run on the secondary processors, as on the Cell. Anyway, going with > 'cooperative-partition' would avoid that confusion. Well, if we pretend that everyone already knows what the "A" stands for, that's not going to avoid confusion. Some people still are going to be wrong. It would be great if we could settle the matter once and for all. I've always thought the A stood for asymmetric, since you're running multiple cores, but each OS is not aware of the others. It doesn't necessarily have to be two copies of Linux. In fact, it usually isn't. As for "MPC8572DS-CAMP", I've always hated that. A specific property that defines an AMP environment is a much better idea.
On Tue, Feb 01, 2011 at 12:48:44PM -0600, Dave Kleikamp wrote: >diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c >index 2a030d8..b33c5e6 100644 >--- a/arch/powerpc/mm/tlb_nohash.c >+++ b/arch/powerpc/mm/tlb_nohash.c >@@ -35,6 +35,7 @@ > #include <linux/preempt.h> > #include <linux/spinlock.h> > #include <linux/memblock.h> >+#include <linux/of_fdt.h> > > #include <asm/tlbflush.h> > #include <asm/tlb.h> >@@ -153,6 +154,17 @@ EXPORT_SYMBOL(local_flush_tlb_page); > */ > #ifdef CONFIG_SMP > >+static int amp; >+ >+#ifdef CONFIG_44x >+void __init early_init_mmu_44x(void) >+{ >+ unsigned long root = of_get_flat_dt_root(); >+ if (of_flat_dt_is_compatible(root, "ibm,47x-AMP")) >+ amp = 1; >+} >+#endif /* CONFIG_44x */ Something aside from the property thing sits weirdly with me on this as well. We have this guarded by CONFIG_44x but also CONFIG_SMP, and we're doing 476 specific checks (for now). There is at least one 44x board that has dual-CPUs (AMCC Arches, iirc) that can theoretically be run in AMP mode. However, it won't be using an SMP kernel because it's a single core per CPU. Admittedly I don't think it supports the tlbivax instruction either so the patch as it stands doesn't impact that theoretical scenario much. I do wonder if we really need to guard the call to this behind CONFIG_SMP though. Maybe a slight performance increase I suppose, but if we wind up using the AMP check elsewhere then it might be needed anyway. Something to think about. Oh, and I agree 'cooperative-partition' or something would be a better check. josh
On Fri, 2011-02-04 at 08:56 -0500, Josh Boyer wrote: > On Tue, Feb 01, 2011 at 12:48:44PM -0600, Dave Kleikamp wrote: > >diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c > >index 2a030d8..b33c5e6 100644 > >--- a/arch/powerpc/mm/tlb_nohash.c > >+++ b/arch/powerpc/mm/tlb_nohash.c > >@@ -35,6 +35,7 @@ > > #include <linux/preempt.h> > > #include <linux/spinlock.h> > > #include <linux/memblock.h> > >+#include <linux/of_fdt.h> > > > > #include <asm/tlbflush.h> > > #include <asm/tlb.h> > >@@ -153,6 +154,17 @@ EXPORT_SYMBOL(local_flush_tlb_page); > > */ > > #ifdef CONFIG_SMP > > > >+static int amp; > >+ > >+#ifdef CONFIG_44x > >+void __init early_init_mmu_44x(void) > >+{ > >+ unsigned long root = of_get_flat_dt_root(); > >+ if (of_flat_dt_is_compatible(root, "ibm,47x-AMP")) > >+ amp = 1; > >+} > >+#endif /* CONFIG_44x */ > > Something aside from the property thing sits weirdly with me on this as > well. > > We have this guarded by CONFIG_44x but also CONFIG_SMP, and we're doing > 476 specific checks (for now). There is at least one 44x board that has > dual-CPUs (AMCC Arches, iirc) that can theoretically be run in AMP mode. > However, it won't be using an SMP kernel because it's a single core per CPU. > Admittedly I don't think it supports the tlbivax instruction either so > the patch as it stands doesn't impact that theoretical scenario much. I should have used CONFIG_PPC_47x here. > I do wonder if we really need to guard the call to this behind > CONFIG_SMP though. Maybe a slight performance increase I suppose, but > if we wind up using the AMP check elsewhere then it might be needed > anyway. Something to think about. I agree that it's awkward. The code affected by this is all behind CONFIG_SMP. There's no reason to use tlbivax, or the alternate ipi, in a uni kernel. An alternative would be to define early_init_mmu_44x (or 47x) outside of CONFIG_SMP, but the contents of the function would still be inside CONFIG_SMP, and it would be an empty function otherwise. > Oh, and I agree 'cooperative-partition' or something would be a better > check. I'm good with that then. > > josh
On Thu, Feb 03, 2011 at 05:15:57PM -0600, Dave Kleikamp wrote: > On Thu, 2011-02-03 at 16:03 +1100, David Gibson wrote: > > On Wed, Feb 02, 2011 at 05:53:59PM -0600, Dave Kleikamp wrote: > > > On Thu, 2011-02-03 at 10:08 +1100, David Gibson wrote: > > > > On Tue, Feb 01, 2011 at 12:48:44PM -0600, Dave Kleikamp wrote: > > > > > Since other OS's may be running on the other cores don't use tlbivax > > > > > > > > [snip] > > > > > +#ifdef CONFIG_44x > > > > > +void __init early_init_mmu_44x(void) > > > > > +{ > > > > > + unsigned long root = of_get_flat_dt_root(); > > > > > + if (of_flat_dt_is_compatible(root, "ibm,47x-AMP")) > > > > > + amp = 1; > > > > > +} > > > > > +#endif /* CONFIG_44x */ > > > > > > > > A test against a hardcoded compatible string seems a nasty way to do > > > > this. Maybe we should define a new boolean property for the root > > > > node. > > > > > > I'm not crazy about this string, but I needed something in the device > > > tree to key off of. Freescale has something similar (i.e. > > > MPC8572DS-CAMP), so I chose to follow their example. I'd be happy to > > > replace it with a boolean property. Any objection to just using > > > "amp"? > > > > Bit too short, I think. I'd suggest either spelling out > > 'asymmetric-multiprocessor' or 'cooperative-partition' (a more > > accurate term, IMO). > > I could be wrong, but I thought the A stands for Asynchronous, not > Asymmetric. I thought Asymmetric means that different types of tasks > run on the secondary processors, as on the Cell. Yeah, I thought so too, but Freescale at least seem to use it this way. 'Asynchronous' would make even less sense. > Anyway, going with > 'cooperative-partition' would avoid that confusion. > > Shaggy
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h index bb40a06..f3a7c65 100644 --- a/arch/powerpc/include/asm/mmu.h +++ b/arch/powerpc/include/asm/mmu.h @@ -80,7 +80,7 @@ static inline int mmu_has_feature(unsigned long feature) extern unsigned int __start___mmu_ftr_fixup, __stop___mmu_ftr_fixup; -/* MMU initialization (64-bit only fo now) */ +/* MMU initialization */ extern void early_init_mmu(void); extern void early_init_mmu_secondary(void); diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c index d1ca976..e50ead7 100644 --- a/arch/powerpc/kernel/setup_32.c +++ b/arch/powerpc/kernel/setup_32.c @@ -126,6 +126,8 @@ notrace void __init machine_init(unsigned long dt_ptr) /* Enable early debugging if any specified (see udbg.h) */ udbg_early_init(); + early_init_mmu(); + probe_machine(); setup_kdump_trampoline(); diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c index 2a030d8..b33c5e6 100644 --- a/arch/powerpc/mm/tlb_nohash.c +++ b/arch/powerpc/mm/tlb_nohash.c @@ -35,6 +35,7 @@ #include <linux/preempt.h> #include <linux/spinlock.h> #include <linux/memblock.h> +#include <linux/of_fdt.h> #include <asm/tlbflush.h> #include <asm/tlb.h> @@ -153,6 +154,17 @@ EXPORT_SYMBOL(local_flush_tlb_page); */ #ifdef CONFIG_SMP +static int amp; + +#ifdef CONFIG_44x +void __init early_init_mmu_44x(void) +{ + unsigned long root = of_get_flat_dt_root(); + if (of_flat_dt_is_compatible(root, "ibm,47x-AMP")) + amp = 1; +} +#endif /* CONFIG_44x */ + static DEFINE_RAW_SPINLOCK(tlbivax_lock); static int mm_is_core_local(struct mm_struct *mm) @@ -232,7 +244,7 @@ void __flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr, cpu_mask = mm_cpumask(mm); if (!mm_is_core_local(mm)) { /* If broadcast tlbivax is supported, use it */ - if (mmu_has_feature(MMU_FTR_USE_TLBIVAX_BCAST)) { + if (!amp && mmu_has_feature(MMU_FTR_USE_TLBIVAX_BCAST)) { int lock = mmu_has_feature(MMU_FTR_LOCK_BCAST_INVAL); if (lock) raw_spin_lock(&tlbivax_lock); @@ -587,4 +599,11 @@ void setup_initial_memory_limit(phys_addr_t first_memblock_base, /* Finally limit subsequent allocations */ memblock_set_current_limit(first_memblock_base + ppc64_rma_size); } +#else /* ! CONFIG_PPC64 */ +void __init early_init_mmu(void) +{ +#if defined(CONFIG_SMP) && defined(CONFIG_44x) + early_init_mmu_44x(); +#endif +} #endif /* CONFIG_PPC64 */
Since other OS's may be running on the other cores don't use tlbivax Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Josh Boyer <jwboyer@linux.vnet.ibm.com> Cc: linuxppc-dev@lists.ozlabs.org --- arch/powerpc/include/asm/mmu.h | 2 +- arch/powerpc/kernel/setup_32.c | 2 ++ arch/powerpc/mm/tlb_nohash.c | 21 ++++++++++++++++++++- 3 files changed, 23 insertions(+), 2 deletions(-)