On Fri, 2010-04-23 at 17:04 -0500, Tseng-Hui (Frank) Lin wrote: > Add Power7 icswx co-processor instruction support.
Please provide a -much- more detailed explanation of what it is, what it does and why it requires hooking into the MMU context switch code. _I_ know these things but nobody else on the list does which limits the ability of people to review your patch. > Signed-off-by: Sonny Rao <sonny...@linux.vnet.ibm.com> > Signed-off-by: Tseng-Hui (Frank) Lin <th...@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/mmu-hash64.h | 3 + > arch/powerpc/include/asm/mmu_context.h | 4 ++ > arch/powerpc/include/asm/reg.h | 3 + > arch/powerpc/mm/mmu_context_hash64.c | 79 > ++++++++++++++++++++++++++++++++ > 4 files changed, 89 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/include/asm/mmu-hash64.h > b/arch/powerpc/include/asm/mmu-hash64.h > index 2102b21..ba5727d 100644 > --- a/arch/powerpc/include/asm/mmu-hash64.h > +++ b/arch/powerpc/include/asm/mmu-hash64.h > @@ -421,6 +421,9 @@ typedef struct { > #ifdef CONFIG_PPC_SUBPAGE_PROT > struct subpage_prot_table spt; > #endif /* CONFIG_PPC_SUBPAGE_PROT */ > + unsigned long acop; > +#define HASH64_MAX_PID (0xFFFF) > + unsigned int pid; Please add a comment as to what those two fields are about, something like acop; /* mask of enabled coprocessor types */ and pid /* pid value used with coprocessors */ or something like that. > } mm_context_t; > > > diff --git a/arch/powerpc/include/asm/mmu_context.h > b/arch/powerpc/include/asm/mmu_context.h > index 26383e0..d6c8841 100644 > --- a/arch/powerpc/include/asm/mmu_context.h > +++ b/arch/powerpc/include/asm/mmu_context.h > @@ -78,6 +78,10 @@ static inline void switch_mm(struct mm_struct *prev, > struct mm_struct *next, > > #define deactivate_mm(tsk,mm) do { } while (0) > > +extern void switch_cop(struct mm_struct *next); > +extern int use_cop(unsigned long acop, struct task_struct *task); ^ remove that space > +extern void disuse_cop(unsigned long acop, struct mm_struct *mm); I'd prefer "drop" or "forget" :-) > + > /* > * After we have set current->mm to a new value, this activates > * the context for the new mm so we see the new mappings. > diff --git a/arch/powerpc/include/asm/reg.h > b/arch/powerpc/include/asm/reg.h > index 5572e86..30503f8 100644 > --- a/arch/powerpc/include/asm/reg.h > +++ b/arch/powerpc/include/asm/reg.h > @@ -516,6 +516,9 @@ > #define SPRN_SIAR 780 > #define SPRN_SDAR 781 > > +#define SPRN_ACOP 31 > +#define SPRN_PID 48 > + Remove the definition of SPRN_PID from reg_booke.h to avoid a double definition then. > #define SPRN_PA6T_MMCR0 795 > #define PA6T_MMCR0_EN0 0x0000000000000001UL > #define PA6T_MMCR0_EN1 0x0000000000000002UL > diff --git a/arch/powerpc/mm/mmu_context_hash64.c > b/arch/powerpc/mm/mmu_context_hash64.c > index 2535828..d0a79f6 100644 > --- a/arch/powerpc/mm/mmu_context_hash64.c > +++ b/arch/powerpc/mm/mmu_context_hash64.c > @@ -18,6 +18,7 @@ > #include <linux/mm.h> > #include <linux/spinlock.h> > #include <linux/idr.h> > +#include <linux/percpu.h> > #include <linux/module.h> > #include <linux/gfp.h> > > @@ -25,6 +26,82 @@ > > static DEFINE_SPINLOCK(mmu_context_lock); > static DEFINE_IDA(mmu_context_ida); > +static DEFINE_IDA(cop_ida); > + > +/* Lazy switch the ACOP register */ > +static DEFINE_PER_CPU(unsigned long, acop_reg); > + > +void switch_cop(struct mm_struct *next) > +{ > + mtspr(SPRN_PID, next->context.pid); > + if (next->context.pid && > + __get_cpu_var(acop_reg) != next->context.acop) { > + mtspr(SPRN_ACOP, next->context.acop); > + __get_cpu_var(acop_reg) = next->context.acop; > + } > +} The above doesn't appear to be called anywhere ? > +int use_cop(unsigned long acop, struct task_struct *task) > +{ > + int pid; > + int err; > + struct mm_struct *mm = get_task_mm(task); > + > + if (!mm) > + return -EINVAL; > + > + if (!mm->context.pid) { > + if (!ida_pre_get(&cop_ida, GFP_KERNEL)) > + return -ENOMEM; > +again: > + spin_lock(&mmu_context_lock); > + err = ida_get_new_above(&cop_ida, 1, &pid); > + spin_unlock(&mmu_context_lock); > + > + if (err == -EAGAIN) > + goto again; > + else if (err) > + return err; > + > + if (pid > HASH64_MAX_PID) { > + spin_lock(&mmu_context_lock); > + ida_remove(&cop_ida, pid); > + spin_unlock(&mmu_context_lock); > + return -EBUSY; > + } > + mm->context.pid = pid; > + mtspr(SPRN_PID, mm->context.pid); Shouldn't the above be ? if (mm == current->active_mm) mtspr(....) > + } > + mm->context.acop |= acop; Locking ? > + get_cpu_var(acop_reg) = mm->context.acop; > + mtspr(SPRN_ACOP, mm->context.acop); Same comment about testing for current. > + put_cpu_var(acop_reg); > + > + return mm->context.pid; > +} > +EXPORT_SYMBOL(use_cop); > + > +void disuse_cop(unsigned long acop, struct mm_struct *mm) > +{ > + if (WARN_ON(!mm)) > + return; > + > + mm->context.acop &= ~acop; Shouldn't the above need some kind of locking if multiple threads hit it with different "acop" values ? > + if (!mm->context.acop) { > + spin_lock(&mmu_context_lock); > + ida_remove(&cop_ida, mm->context.pid); > + spin_unlock(&mmu_context_lock); > + mm->context.pid = 0; > + mtspr(SPRN_PID, 0); Same comment about current. > + } else { > + get_cpu_var(acop_reg) = mm->context.acop; > + mtspr(SPRN_ACOP, mm->context.acop); Same comment. > + put_cpu_var(acop_reg); > + } > + mmput(mm); > +} > +EXPORT_SYMBOL(disuse_cop); > > /* > * The proto-VSID space has 2^35 - 1 segments available for user > mappings. > @@ -94,6 +171,8 @@ EXPORT_SYMBOL_GPL(__destroy_context); > void destroy_context(struct mm_struct *mm) > { > __destroy_context(mm->context.id); > + if (mm->context.pid) > + ida_remove(&cop_ida, mm->context.pid); > subpage_prot_free(mm); > mm->context.id = NO_CONTEXT; > } Cheers, Ben. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev