Re: [PATCH v3 13/18] powerpc: Add support for setting SPRN_TIDR

[Michael Ellerman]

Sukadev Bhattiprolu  writes:

> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 37ed60b..d861fcd 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1120,6 +1120,13 @@ static inline void restore_sprs(struct thread_struct 
> *old_thread,
>   mtspr(SPRN_TAR, new_thread->tar);
>   }
>  #endif
> +#ifdef CONFIG_PPC64
> + if (old_thread->tidr != new_thread->tidr) {
> + /* TIDR should be non-zero only with ISA3.0. */
> + WARN_ON_ONCE(!cpu_has_feature(CPU_FTR_ARCH_300));

This is silly, just make the whole block depend on CPU_FTR_ARCH_300.

I've fixed it up.

> @@ -1466,6 +1581,10 @@ int arch_dup_task_struct(struct task_struct *dst, 
> struct task_struct *src)
>  
>   clear_task_ebb(dst);
>  
> +#ifdef CONFIG_PPC64
> + dst->thread.tidr = 0;
> +#endif
> +
>   return 0;
>  }

This is called from clone().


> @@ -1576,6 +1695,9 @@ int copy_thread(unsigned long clone_flags, unsigned 
> long usp,
>  #endif
>  
>   setup_ksp_vsid(p, sp);
> +#ifdef CONFIG_PPC64
> + p->thread.tidr = 0;
> +#endif
  
And so is this.

So I think you only need the latter.

That does mean you're clearing the TIDR on fork(), but not exec().

Typically you'd do the reverse, ie. clear it on exec() but leave it the
same on fork().

Because this is a "thread id" I think it does make sense to clear it on
fork(), otherwise you potentially have two processes with the same id
and only one of them will be woken up by the AS_notify.

So I'll drop the clear from arch_dup_task_struct().

cheers

Re: [PATCH v3 13/18] powerpc: Add support for setting SPRN_TIDR

[Michael Ellerman]

Sukadev Bhattiprolu  writes:

> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 37ed60b..d861fcd 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1120,6 +1120,13 @@ static inline void restore_sprs(struct thread_struct 
> *old_thread,
>   mtspr(SPRN_TAR, new_thread->tar);
>   }
>  #endif
> +#ifdef CONFIG_PPC64
> + if (old_thread->tidr != new_thread->tidr) {
> + /* TIDR should be non-zero only with ISA3.0. */
> + WARN_ON_ONCE(!cpu_has_feature(CPU_FTR_ARCH_300));

This is silly, just make the whole block depend on CPU_FTR_ARCH_300.

I've fixed it up.

> @@ -1466,6 +1581,10 @@ int arch_dup_task_struct(struct task_struct *dst, 
> struct task_struct *src)
>  
>   clear_task_ebb(dst);
>  
> +#ifdef CONFIG_PPC64
> + dst->thread.tidr = 0;
> +#endif
> +
>   return 0;
>  }

This is called from clone().


> @@ -1576,6 +1695,9 @@ int copy_thread(unsigned long clone_flags, unsigned 
> long usp,
>  #endif
>  
>   setup_ksp_vsid(p, sp);
> +#ifdef CONFIG_PPC64
> + p->thread.tidr = 0;
> +#endif
  
And so is this.

So I think you only need the latter.

That does mean you're clearing the TIDR on fork(), but not exec().

Typically you'd do the reverse, ie. clear it on exec() but leave it the
same on fork().

Because this is a "thread id" I think it does make sense to clear it on
fork(), otherwise you potentially have two processes with the same id
and only one of them will be woken up by the AS_notify.

So I'll drop the clear from arch_dup_task_struct().

cheers

[PATCH v3 13/18] powerpc: Add support for setting SPRN_TIDR

[Sukadev Bhattiprolu]

We need the SPRN_TIDR to be set for use with fast thread-wakeup (core-
to-core wakeup) and also with CAPI.

Each thread in a process needs to have a unique id within the process.
But as explained below, for now, we assign globally unique thread ids
to all threads in the system.

Signed-off-by: Sukadev Bhattiprolu 
Signed-off-by: Philippe Bergheaud 
Signed-off-by: Christophe Lombard 
---
Changelog[v3]
- Merge changes with and address comments to Christophe's patch.
  (i.e drop CONFIG_PPC_VAS; use CONFIG_PPC64; check CPU_ARCH_300
  before setting TIDR). Defer following to separate patches:
- emulation parts of Christophe's patch,
- setting TIDR for tasks other than 'current'
- setting feature bit in AT_HWCAP2

Changelog[v2]
- Michael Ellerman: Use an interface to assign TIDR so it is
assigned to only threads that need it; move assignment to
restore_sprs(). Drop lint from rebase;
---
 arch/powerpc/include/asm/processor.h |   1 +
 arch/powerpc/include/asm/switch_to.h |   3 +
 arch/powerpc/kernel/process.c| 122 +++
 3 files changed, 126 insertions(+)

diff --git a/arch/powerpc/include/asm/processor.h 
b/arch/powerpc/include/asm/processor.h
index fab7ff8..58cc212 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -329,6 +329,7 @@ struct thread_struct {
 */
int dscr_inherit;
unsigned long   ppr;/* used to save/restore SMT priority */
+   unsigned long   tidr;
 #endif
 #ifdef CONFIG_PPC_BOOK3S_64
unsigned long   tar;
diff --git a/arch/powerpc/include/asm/switch_to.h 
b/arch/powerpc/include/asm/switch_to.h
index bf820f5..ad2d762 100644
--- a/arch/powerpc/include/asm/switch_to.h
+++ b/arch/powerpc/include/asm/switch_to.h
@@ -92,4 +92,7 @@ static inline void clear_task_ebb(struct task_struct *t)
 #endif
 }
 
+extern int set_thread_tidr(struct task_struct *t);
+extern void clear_thread_tidr(struct task_struct *t);
+
 #endif /* _ASM_POWERPC_SWITCH_TO_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 37ed60b..d861fcd 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1120,6 +1120,13 @@ static inline void restore_sprs(struct thread_struct 
*old_thread,
mtspr(SPRN_TAR, new_thread->tar);
}
 #endif
+#ifdef CONFIG_PPC64
+   if (old_thread->tidr != new_thread->tidr) {
+   /* TIDR should be non-zero only with ISA3.0. */
+   WARN_ON_ONCE(!cpu_has_feature(CPU_FTR_ARCH_300));
+   mtspr(SPRN_TIDR, new_thread->tidr);
+   }
+#endif
 }
 
 #ifdef CONFIG_PPC_BOOK3S_64
@@ -1438,9 +1445,117 @@ void flush_thread(void)
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 }
 
+#ifdef CONFIG_PPC64
+static DEFINE_SPINLOCK(vas_thread_id_lock);
+static DEFINE_IDA(vas_thread_ida);
+
+/*
+ * We need to assign a unique thread id to each thread in a process.
+ *
+ * This thread id, referred to as TIDR, and separate from the Linux's tgid,
+ * is intended to be used to direct an ASB_Notify from the hardware to the
+ * thread, when a suitable event occurs in the system.
+ *
+ * One such event is a "paste" instruction in the context of Fast Thread
+ * Wakeup (aka Core-to-core wake up in the Virtual Accelerator Switchboard
+ * (VAS) in POWER9.
+ *
+ * To get a unique TIDR per process we could simply reuse task_pid_nr() but
+ * the problem is that task_pid_nr() is not yet available copy_thread() is
+ * called. Fixing that would require changing more intrusive arch-neutral
+ * code in code path in copy_process()?.
+ *
+ * Further, to assign unique TIDRs within each process, we need an atomic
+ * field (or an IDR) in task_struct, which again intrudes into the arch-
+ * neutral code. So try to assign globally unique TIDRs for now.
+ *
+ * NOTE: TIDR 0 indicates that the thread does not need a TIDR value.
+ *  For now, only threads that expect to be notified by the VAS
+ *  hardware need a TIDR value and we assign values > 0 for those.
+ */
+#define MAX_THREAD_CONTEXT ((1 << 16) - 1)
+static int assign_thread_tidr(void)
+{
+   int index;
+   int err;
+
+again:
+   if (!ida_pre_get(_thread_ida, GFP_KERNEL))
+   return -ENOMEM;
+
+   spin_lock(_thread_id_lock);
+   err = ida_get_new_above(_thread_ida, 1, );
+   spin_unlock(_thread_id_lock);
+
+   if (err == -EAGAIN)
+   goto again;
+   else if (err)
+   return err;
+
+   if (index > MAX_THREAD_CONTEXT) {
+   spin_lock(_thread_id_lock);
+   ida_remove(_thread_ida, index);
+   spin_unlock(_thread_id_lock);
+   return -ENOMEM;
+   }
+
+   return index;
+}
+
+static void free_thread_tidr(int id)
+{
+   

[PATCH v3 13/18] powerpc: Add support for setting SPRN_TIDR

[Sukadev Bhattiprolu]

We need the SPRN_TIDR to be set for use with fast thread-wakeup (core-
to-core wakeup) and also with CAPI.

Each thread in a process needs to have a unique id within the process.
But as explained below, for now, we assign globally unique thread ids
to all threads in the system.

Signed-off-by: Sukadev Bhattiprolu 
Signed-off-by: Philippe Bergheaud 
Signed-off-by: Christophe Lombard 
---
Changelog[v3]
- Merge changes with and address comments to Christophe's patch.
  (i.e drop CONFIG_PPC_VAS; use CONFIG_PPC64; check CPU_ARCH_300
  before setting TIDR). Defer following to separate patches:
- emulation parts of Christophe's patch,
- setting TIDR for tasks other than 'current'
- setting feature bit in AT_HWCAP2

Changelog[v2]
- Michael Ellerman: Use an interface to assign TIDR so it is
assigned to only threads that need it; move assignment to
restore_sprs(). Drop lint from rebase;
---
 arch/powerpc/include/asm/processor.h |   1 +
 arch/powerpc/include/asm/switch_to.h |   3 +
 arch/powerpc/kernel/process.c| 122 +++
 3 files changed, 126 insertions(+)

diff --git a/arch/powerpc/include/asm/processor.h 
b/arch/powerpc/include/asm/processor.h
index fab7ff8..58cc212 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -329,6 +329,7 @@ struct thread_struct {
 */
int dscr_inherit;
unsigned long   ppr;/* used to save/restore SMT priority */
+   unsigned long   tidr;
 #endif
 #ifdef CONFIG_PPC_BOOK3S_64
unsigned long   tar;
diff --git a/arch/powerpc/include/asm/switch_to.h 
b/arch/powerpc/include/asm/switch_to.h
index bf820f5..ad2d762 100644
--- a/arch/powerpc/include/asm/switch_to.h
+++ b/arch/powerpc/include/asm/switch_to.h
@@ -92,4 +92,7 @@ static inline void clear_task_ebb(struct task_struct *t)
 #endif
 }
 
+extern int set_thread_tidr(struct task_struct *t);
+extern void clear_thread_tidr(struct task_struct *t);
+
 #endif /* _ASM_POWERPC_SWITCH_TO_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 37ed60b..d861fcd 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1120,6 +1120,13 @@ static inline void restore_sprs(struct thread_struct 
*old_thread,
mtspr(SPRN_TAR, new_thread->tar);
}
 #endif
+#ifdef CONFIG_PPC64
+   if (old_thread->tidr != new_thread->tidr) {
+   /* TIDR should be non-zero only with ISA3.0. */
+   WARN_ON_ONCE(!cpu_has_feature(CPU_FTR_ARCH_300));
+   mtspr(SPRN_TIDR, new_thread->tidr);
+   }
+#endif
 }
 
 #ifdef CONFIG_PPC_BOOK3S_64
@@ -1438,9 +1445,117 @@ void flush_thread(void)
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 }
 
+#ifdef CONFIG_PPC64
+static DEFINE_SPINLOCK(vas_thread_id_lock);
+static DEFINE_IDA(vas_thread_ida);
+
+/*
+ * We need to assign a unique thread id to each thread in a process.
+ *
+ * This thread id, referred to as TIDR, and separate from the Linux's tgid,
+ * is intended to be used to direct an ASB_Notify from the hardware to the
+ * thread, when a suitable event occurs in the system.
+ *
+ * One such event is a "paste" instruction in the context of Fast Thread
+ * Wakeup (aka Core-to-core wake up in the Virtual Accelerator Switchboard
+ * (VAS) in POWER9.
+ *
+ * To get a unique TIDR per process we could simply reuse task_pid_nr() but
+ * the problem is that task_pid_nr() is not yet available copy_thread() is
+ * called. Fixing that would require changing more intrusive arch-neutral
+ * code in code path in copy_process()?.
+ *
+ * Further, to assign unique TIDRs within each process, we need an atomic
+ * field (or an IDR) in task_struct, which again intrudes into the arch-
+ * neutral code. So try to assign globally unique TIDRs for now.
+ *
+ * NOTE: TIDR 0 indicates that the thread does not need a TIDR value.
+ *  For now, only threads that expect to be notified by the VAS
+ *  hardware need a TIDR value and we assign values > 0 for those.
+ */
+#define MAX_THREAD_CONTEXT ((1 << 16) - 1)
+static int assign_thread_tidr(void)
+{
+   int index;
+   int err;
+
+again:
+   if (!ida_pre_get(_thread_ida, GFP_KERNEL))
+   return -ENOMEM;
+
+   spin_lock(_thread_id_lock);
+   err = ida_get_new_above(_thread_ida, 1, );
+   spin_unlock(_thread_id_lock);
+
+   if (err == -EAGAIN)
+   goto again;
+   else if (err)
+   return err;
+
+   if (index > MAX_THREAD_CONTEXT) {
+   spin_lock(_thread_id_lock);
+   ida_remove(_thread_ida, index);
+   spin_unlock(_thread_id_lock);
+   return -ENOMEM;
+   }
+
+   return index;
+}
+
+static void free_thread_tidr(int id)
+{
+   spin_lock(_thread_id_lock);
+   ida_remove(_thread_ida, id);
+   spin_unlock(_thread_id_lock);
+}