Re: [PATCH V2 4/6] powerpc/44x: don't use tlbivax on AMP systems

[Dave Kleikamp <shaggy@linux.vnet.ibm.com>]

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

-- 
Dave Kleikamp
IBM Linux Technology Center