* [PATCH] add icswx support
@ 2010-04-23 22:04 Tseng-Hui (Frank) Lin
2010-04-24 0:55 ` Benjamin Herrenschmidt
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Tseng-Hui (Frank) Lin @ 2010-04-23 22:04 UTC (permalink / raw)
To: linuxppc-dev
Add Power7 icswx co-processor instruction support.
Signed-off-by: Sonny Rao <sonnyrao@linux.vnet.ibm.com>
Signed-off-by: Tseng-Hui (Frank) Lin <thlin@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;
} 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);
+extern void disuse_cop(unsigned long acop, struct mm_struct *mm);
+
/*
* 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
+
#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;
+ }
+}
+
+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);
+ }
+ mm->context.acop |= acop;
+
+ get_cpu_var(acop_reg) = mm->context.acop;
+ mtspr(SPRN_ACOP, mm->context.acop);
+ 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;
+ 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);
+ } else {
+ get_cpu_var(acop_reg) = mm->context.acop;
+ mtspr(SPRN_ACOP, mm->context.acop);
+ 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;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] add icswx support
2010-04-23 22:04 [PATCH] add icswx support Tseng-Hui (Frank) Lin
@ 2010-04-24 0:55 ` Benjamin Herrenschmidt
2010-04-27 21:56 ` Tseng-Hui (Frank) Lin
2010-04-29 17:11 ` Mike Kravetz
2010-05-03 1:43 ` Olof Johansson
2 siblings, 1 reply; 8+ messages in thread
From: Benjamin Herrenschmidt @ 2010-04-24 0:55 UTC (permalink / raw)
To: Tseng-Hui (Frank) Lin; +Cc: linuxppc-dev
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 <sonnyrao@linux.vnet.ibm.com>
> Signed-off-by: Tseng-Hui (Frank) Lin <thlin@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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] add icswx support
2010-04-24 0:55 ` Benjamin Herrenschmidt
@ 2010-04-27 21:56 ` Tseng-Hui (Frank) Lin
2010-04-27 22:01 ` Benjamin Herrenschmidt
2010-04-30 13:35 ` Michael Ellerman
0 siblings, 2 replies; 8+ messages in thread
From: Tseng-Hui (Frank) Lin @ 2010-04-27 21:56 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
On Sat, 2010-04-24 at 10:55 +1000, Benjamin Herrenschmidt wrote:
> 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.
>
icswx is a PowerPC co-processor instruction to send data to a
co-processor. On Book-S processors the LPAR_ID and process ID (PID) of
the owning process are registered in the window context of the
co-processor at initial time. When the icswx instruction is executed,
the L2 generates a cop-reg transaction on PowerBus. The transaction has
no address and the processor does not perform an MMU access to
authenticate the transaction. The coprocessor compares the LPAR_ID and
the PID included in the transaction and the LPAR_ID and PID held in the
window context to determine if the process is authorized to generate the
transaction.
> > Signed-off-by: Sonny Rao <sonnyrao@linux.vnet.ibm.com>
> > Signed-off-by: Tseng-Hui (Frank) Lin <thlin@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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] add icswx support
2010-04-27 21:56 ` Tseng-Hui (Frank) Lin
@ 2010-04-27 22:01 ` Benjamin Herrenschmidt
2010-04-30 13:35 ` Michael Ellerman
1 sibling, 0 replies; 8+ messages in thread
From: Benjamin Herrenschmidt @ 2010-04-27 22:01 UTC (permalink / raw)
To: Tseng-Hui (Frank) Lin; +Cc: linuxppc-dev
On Tue, 2010-04-27 at 16:56 -0500, Tseng-Hui (Frank) Lin wrote:
> icswx is a PowerPC co-processor instruction to send data to a
> co-processor. On Book-S processors the LPAR_ID and process ID (PID) of
> the owning process are registered in the window context of the
> co-processor at initial time. When the icswx instruction is executed,
> the L2 generates a cop-reg transaction on PowerBus. The transaction has
> no address and the processor does not perform an MMU access to
> authenticate the transaction. The coprocessor compares the LPAR_ID and
> the PID included in the transaction and the LPAR_ID and PID held in the
> window context to determine if the process is authorized to generate the
> transaction.
Ok, good, then stick that in the cset comment of your next patch
submission after you've addresses all my other comments :-)
Cheers,
Ben.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] add icswx support
2010-04-23 22:04 [PATCH] add icswx support Tseng-Hui (Frank) Lin
2010-04-24 0:55 ` Benjamin Herrenschmidt
@ 2010-04-29 17:11 ` Mike Kravetz
2010-04-29 22:50 ` Benjamin Herrenschmidt
2010-05-03 1:43 ` Olof Johansson
2 siblings, 1 reply; 8+ messages in thread
From: Mike Kravetz @ 2010-04-29 17:11 UTC (permalink / raw)
To: Tseng-Hui (Frank) Lin; +Cc: linuxppc-dev
On Fri, Apr 23, 2010 at 05:04:35PM -0500, Tseng-Hui (Frank) Lin wrote:
> Add Power7 icswx co-processor instruction support.
Silly question perhaps, but
Do we want this code to be enabled/exist for all processors? I don't
see any checks for Power7 processors. Or, will it be the responsibility
of the caller to ensure that they are running on P7?
--
Mike
> Signed-off-by: Sonny Rao <sonnyrao@linux.vnet.ibm.com>
> Signed-off-by: Tseng-Hui (Frank) Lin <thlin@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;
> } 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);
> +extern void disuse_cop(unsigned long acop, struct mm_struct *mm);
> +
> /*
> * 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
> +
> #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;
> + }
> +}
> +
> +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);
> + }
> + mm->context.acop |= acop;
> +
> + get_cpu_var(acop_reg) = mm->context.acop;
> + mtspr(SPRN_ACOP, mm->context.acop);
> + 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;
> + 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);
> + } else {
> + get_cpu_var(acop_reg) = mm->context.acop;
> + mtspr(SPRN_ACOP, mm->context.acop);
> + 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;
> }
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] add icswx support
2010-04-29 17:11 ` Mike Kravetz
@ 2010-04-29 22:50 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 8+ messages in thread
From: Benjamin Herrenschmidt @ 2010-04-29 22:50 UTC (permalink / raw)
To: Mike Kravetz; +Cc: linuxppc-dev, Tseng-Hui (Frank) Lin
On Thu, 2010-04-29 at 10:11 -0700, Mike Kravetz wrote:
> On Fri, Apr 23, 2010 at 05:04:35PM -0500, Tseng-Hui (Frank) Lin wrote:
> > Add Power7 icswx co-processor instruction support.
>
> Silly question perhaps, but
>
> Do we want this code to be enabled/exist for all processors? I don't
> see any checks for Power7 processors. Or, will it be the responsibility
> of the caller to ensure that they are running on P7?
Well, this is a good point, there should be at least a CPU feature here,
especially since some of that code needs to be called from the context
switch routine (though that bit seems to be missing at the moment).
Cheers,
Ben.
> --
> Mike
>
>
> > Signed-off-by: Sonny Rao <sonnyrao@linux.vnet.ibm.com>
> > Signed-off-by: Tseng-Hui (Frank) Lin <thlin@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;
> > } 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);
> > +extern void disuse_cop(unsigned long acop, struct mm_struct *mm);
> > +
> > /*
> > * 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
> > +
> > #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;
> > + }
> > +}
> > +
> > +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);
> > + }
> > + mm->context.acop |= acop;
> > +
> > + get_cpu_var(acop_reg) = mm->context.acop;
> > + mtspr(SPRN_ACOP, mm->context.acop);
> > + 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;
> > + 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);
> > + } else {
> > + get_cpu_var(acop_reg) = mm->context.acop;
> > + mtspr(SPRN_ACOP, mm->context.acop);
> > + 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;
> > }
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] add icswx support
2010-04-27 21:56 ` Tseng-Hui (Frank) Lin
2010-04-27 22:01 ` Benjamin Herrenschmidt
@ 2010-04-30 13:35 ` Michael Ellerman
1 sibling, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2010-04-30 13:35 UTC (permalink / raw)
To: Tseng-Hui (Frank) Lin; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]
On Tue, 2010-04-27 at 16:56 -0500, Tseng-Hui (Frank) Lin wrote:
> On Sat, 2010-04-24 at 10:55 +1000, Benjamin Herrenschmidt wrote:
> > 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.
> >
>
> icswx is a PowerPC co-processor instruction to send data to a
> co-processor. On Book-S processors the LPAR_ID and process ID (PID) of
> the owning process are registered in the window context of the
> co-processor at initial time. When the icswx instruction is executed,
> the L2 generates a cop-reg transaction on PowerBus. The transaction has
> no address and the processor does not perform an MMU access to
> authenticate the transaction. The coprocessor compares the LPAR_ID and
> the PID included in the transaction and the LPAR_ID and PID held in the
> window context to determine if the process is authorized to generate the
> transaction.
How does userspace discover that there are coprocessors to send requests
to? And how does the coprocessor send results back to the process?
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] add icswx support
2010-04-23 22:04 [PATCH] add icswx support Tseng-Hui (Frank) Lin
2010-04-24 0:55 ` Benjamin Herrenschmidt
2010-04-29 17:11 ` Mike Kravetz
@ 2010-05-03 1:43 ` Olof Johansson
2 siblings, 0 replies; 8+ messages in thread
From: Olof Johansson @ 2010-05-03 1:43 UTC (permalink / raw)
To: Tseng-Hui (Frank) Lin; +Cc: linuxppc-dev
On Fri, Apr 23, 2010 at 05:04:35PM -0500, Tseng-Hui (Frank) Lin wrote:
> Add Power7 icswx co-processor instruction support.
>
> Signed-off-by: Sonny Rao <sonnyrao@linux.vnet.ibm.com>
> Signed-off-by: Tseng-Hui (Frank) Lin <thlin@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;
Way too generic a name. Please call it acop_pid or something similar.
Same with the define.
This is also growing the mm_struct by 16 bytes on all platforms, no
matter if the process in question is using the coprocessor or not.
-Olof
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-05-03 1:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-23 22:04 [PATCH] add icswx support Tseng-Hui (Frank) Lin
2010-04-24 0:55 ` Benjamin Herrenschmidt
2010-04-27 21:56 ` Tseng-Hui (Frank) Lin
2010-04-27 22:01 ` Benjamin Herrenschmidt
2010-04-30 13:35 ` Michael Ellerman
2010-04-29 17:11 ` Mike Kravetz
2010-04-29 22:50 ` Benjamin Herrenschmidt
2010-05-03 1:43 ` Olof Johansson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.