[PATCH V4] cxl: Add support for ASB_Notify on POWER9

The POWER9 core supports a new feature: ASB_Notify which requires the
support of the Special Purpose Register: TIDR.

The ASB_Notify command, generated by the AFU, will attempt to
wake-up the host thread identified by the particular LPID:PID:TID.

This patch assign a unique TIDR (thread id) for the current thread which
will be used in the process element entry.

A next patch will handle a new kind of "compatible" property in the
device-tree (PHB DT node) indicating which version of CAPI and which
features are supported, instead of handling PVR values.

Signed-off-by: Christophe Lombard <redacted>
Reviewed-by: Philippe Bergheaud <redacted>

---
Changelog[v4]
 - Rebased to latest upstream.
 - Updated the ioctl interface.
 - Removed the field tid in the context structure.

Changelog[v3]
 - Rebased to latest upstream.
 - Updated attr->tid field in cxllib_get_PE_attributes().

Changelog[v2]
 - Rebased to latest upstream.
 - Updated the ioctl interface.
 - Added a checking to allow updating the TIDR if a P9 chip is present.
---
 arch/powerpc/kernel/process.c |  1 +
 drivers/misc/cxl/context.c    | 15 +++++++++++++++
 drivers/misc/cxl/cxl.h        |  3 +++
 drivers/misc/cxl/cxllib.c     |  3 ++-
 drivers/misc/cxl/file.c       |  7 +++++++
 drivers/misc/cxl/native.c     |  2 +-
 include/uapi/misc/cxl.h       |  4 +++-
 7 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 5acb5a1..a6a70e2 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c

@@ -1589,6 +1589,7 @@ int set_thread_tidr(struct task_struct *t)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(set_thread_tidr);
 
 #endif /* CONFIG_PPC64 */
 

diff --git a/drivers/misc/cxl/context.c b/drivers/misc/cxl/context.c
index 12a41b2..e309d35 100644
--- a/drivers/misc/cxl/context.c
+++ b/drivers/misc/cxl/context.c

@@ -22,6 +22,7 @@
 #include <asm/cputable.h>
 #include <asm/current.h>
 #include <asm/copro.h>
+#include <asm/switch_to.h>
 
 #include "cxl.h"
 

@@ -362,3 +363,17 @@ void cxl_context_mm_count_put(struct cxl_context *ctx)
 	if (ctx->mm)
 		mmdrop(ctx->mm);
 }
+
+int cxl_context_thread_tidr(struct cxl_context *ctx)
+{
+	int rc = 0;
+
+	if (!cxl_is_power9())
+		return -ENODEV;
+
+	rc = set_thread_tidr(current);
+	pr_devel("%s: current tidr: %ld\n", __func__,
+		 current->thread.tidr);
+
+	return rc;
+}

diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index e46a406..1a5db0b 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h

@@ -1169,4 +1169,7 @@ void cxl_context_mm_count_get(struct cxl_context *ctx);
 /* Decrements the reference count to "struct mm_struct" */
 void cxl_context_mm_count_put(struct cxl_context *ctx);
 
+/* Handles an unique TIDR (thread id) for the current thread */
+int cxl_context_thread_tidr(struct cxl_context *ctx);
+
 #endif

diff --git a/drivers/misc/cxl/cxllib.c b/drivers/misc/cxl/cxllib.c
index dc9bc18..30ccba4 100644
--- a/drivers/misc/cxl/cxllib.c
+++ b/drivers/misc/cxl/cxllib.c

@@ -199,10 +199,11 @@ int cxllib_get_PE_attributes(struct task_struct *task,
 		 */
 		attr->pid = mm->context.id;
 		mmput(mm);
+		attr->tid = task->thread.tidr;
 	} else {
 		attr->pid = 0;
+		attr->tid = 0;
 	}
-	attr->tid = 0;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(cxllib_get_PE_attributes);

diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c
index 76c0b0c..a823c76 100644
--- a/drivers/misc/cxl/file.c
+++ b/drivers/misc/cxl/file.c

@@ -248,6 +248,13 @@ static long afu_ioctl_start_work(struct cxl_context *ctx,
 	 */
 	smp_mb();
 
+	/* Assign a unique TIDR (thread id) for the current thread */
+	if (work.flags & CXL_START_WORK_TID) {
+		rc = cxl_context_thread_tidr(ctx);
+		if (rc)
+			goto out;
+	}
+
 	trace_cxl_attach(ctx, work.work_element_descriptor, work.num_interrupts, amr);
 
 	if ((rc = cxl_ops->attach_process(ctx, false, work.work_element_descriptor,

diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 02b6b45..036fe5b 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c

@@ -673,7 +673,7 @@ static int process_element_entry_psl9(struct cxl_context *ctx, u64 wed, u64 amr)
 		pid = ctx->mm->context.id;
 	}
 
-	ctx->elem->common.tid = 0;
+	ctx->elem->common.tid = cpu_to_be32(current->thread.tidr);
 	ctx->elem->common.pid = cpu_to_be32(pid);
 
 	ctx->elem->sr = cpu_to_be64(calculate_sr(ctx));

diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h
index 49e8fd0..980ee8f 100644
--- a/include/uapi/misc/cxl.h
+++ b/include/uapi/misc/cxl.h

@@ -31,9 +31,11 @@ struct cxl_ioctl_start_work {
 #define CXL_START_WORK_AMR		0x0000000000000001ULL
 #define CXL_START_WORK_NUM_IRQS		0x0000000000000002ULL
 #define CXL_START_WORK_ERR_FF		0x0000000000000004ULL
+#define CXL_START_WORK_TID		0x0000000000000008ULL
 #define CXL_START_WORK_ALL		(CXL_START_WORK_AMR |\
 					 CXL_START_WORK_NUM_IRQS |\
-					 CXL_START_WORK_ERR_FF)
+					 CXL_START_WORK_ERR_FF |\
+					 CXL_START_WORK_TID)
 
 
 /* Possible modes that an afu can be in */

-- 
2.7.4

Hi Christophe,

Thanks for the changes to the patch. Few minor review comments:

Christophe Lombard [off-list ref] writes:

EINVAL might be a better return value instead of ENODEV in this case.

May need to copy the cxl_ioctl_start_work struct back to userspace with
the value of tidr allocated.

+	}
+
 	trace_cxl_attach(ctx, work.work_element_descriptor,
work.num_interrupts, amr);

should update the tracing here to also report the tidr

Should reserve a field in the cxl_ioctl_start_work struct to report the
tidr back to userspace.


-- 
Vaibhav Jain [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.

Le 20/12/2017 à 07:31, Vaibhav Jain a écrit :

Hi Christophe,

Thanks for the changes to the patch. Few minor review comments:

Thanks for the review.

Christophe Lombard [off-list ref] writes:

quoted

@@ -362,3 +363,17 @@ void cxl_context_mm_count_put(struct cxl_context *ctx)
  	if (ctx->mm)
  		mmdrop(ctx->mm);
  }
+
+int cxl_context_thread_tidr(struct cxl_context *ctx)
+{
+	int rc = 0;
+
+	if (!cxl_is_power9())
+		return -ENODEV;

EINVAL might be a better return value instead of ENODEV in this case.

This return code has been already discussed (with mpe) on the first
version of the patch. "Either ENODEV or ENXIO would be best that
can be distinguished and interpreted correctly by userspace"

quoted

--- a/drivers/misc/cxl/file.c
+++ b/drivers/misc/cxl/file.c

@@ -248,6 +248,13 @@ static long afu_ioctl_start_work(struct cxl_context *ctx,
  	 */
  	smp_mb();

+	/* Assign a unique TIDR (thread id) for the current thread */
+	if (work.flags & CXL_START_WORK_TID) {
+		rc = cxl_context_thread_tidr(ctx);
+		if (rc)
+			goto out;

May need to copy the cxl_ioctl_start_work struct back to userspace with
the value of tidr allocated.

In fact, it does not matter. I don't know what the userspace could do
with this value.

quoted

+	}
+
  	trace_cxl_attach(ctx, work.work_element_descriptor,
work.num_interrupts, amr);

should update the tracing here to also report the tidr

yep. I will provide a new patch to include this update.

quoted

diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h
index 49e8fd0..980ee8f 100644
--- a/include/uapi/misc/cxl.h
+++ b/include/uapi/misc/cxl.h

@@ -31,9 +31,11 @@ struct cxl_ioctl_start_work {

Should reserve a field in the cxl_ioctl_start_work struct to report the
tidr back to userspace.

We could do that, but as we discussed previously. We want to minimize
the impact on libcxl.

Hi Chritophe,

christophe lombard [off-list ref] writes:

Le 20/12/2017 =C3=A0 07:31, Vaibhav Jain a =C3=A9crit=C2=A0:

quoted

EINVAL might be a better return value instead of ENODEV in this case.

This return code has been already discussed (with mpe) on the first
version of the patch. "Either ENODEV or ENXIO would be best that
can be distinguished and interpreted correctly by userspace"

Agreed. Please ignore the review comment.

quoted

quoted

+	/* Assign a unique TIDR (thread id) for the current thread */
+	if (work.flags & CXL_START_WORK_TID) {
+		rc =3D cxl_context_thread_tidr(ctx);
+		if (rc)
+			goto out;

May need to copy the cxl_ioctl_start_work struct back to userspace with
the value of tidr allocated.

In fact, it does not matter. I don't know what the userspace could do
with this value.

Without libcxl knowing the tidr value, it cannot enforce the condition
that only threads that have called attach can issue 'wait' on the right
context.

Also AFU can selectively ask PSL to issue asb_notify to a specific
thread via the PSL interface. Without userspace knowing the tidr value
it might not be easy for it to give this value to AFU through a Problem
State Area register.

quoted

quoted

+	}
+
  	trace_cxl_attach(ctx, work.work_element_descriptor,
work.num_interrupts, amr);

should update the tracing here to also report the tidr
=20

yep. I will provide a new patch to include this update.

Thanks

--=20
Vaibhav Jain [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.

Le 20/12/2017 à 09:46, Vaibhav Jain a écrit :

Hi Chritophe,

christophe lombard [off-list ref] writes:

quoted

Le 20/12/2017 à 07:31, Vaibhav Jain a écrit :

quoted

EINVAL might be a better return value instead of ENODEV in this case.

This return code has been already discussed (with mpe) on the first
version of the patch. "Either ENODEV or ENXIO would be best that
can be distinguished and interpreted correctly by userspace"

Agreed. Please ignore the review comment.

quoted

quoted

quoted

+	/* Assign a unique TIDR (thread id) for the current thread */
+	if (work.flags & CXL_START_WORK_TID) {
+		rc = cxl_context_thread_tidr(ctx);
+		if (rc)
+			goto out;

May need to copy the cxl_ioctl_start_work struct back to userspace with
the value of tidr allocated.

In fact, it does not matter. I don't know what the userspace could do
with this value.

Without libcxl knowing the tidr value, it cannot enforce the condition
that only threads that have called attach can issue 'wait' on the right
context.

Also AFU can selectively ask PSL to issue asb_notify to a specific
thread via the PSL interface. Without userspace knowing the tidr value
it might not be easy for it to give this value to AFU through a Problem
State Area register.

Don't forget that The ASB_Notify will use LPID:PID:TID tuple found
in the Process Element Entry.
The AFU may optionally provide a TID on AxH_CEA[40:55] (AxH_CEA[39]
must be set to indicate an AFU provided TID)
If AxH_CEA[39] == 1’b0 then Process Element information
(LPID:PID:TID) is used to generate the PCIe address.
If AxH_CEA[39] == 1’b1then the LPID:PID are taken from the PEE
while the TID is taken from AxH_-CEA[40:55]

quoted

quoted

quoted

+	}
+
   	trace_cxl_attach(ctx, work.work_element_descriptor,
work.num_interrupts, amr);

should update the tracing here to also report the tidr

yep. I will provide a new patch to include this update.

Thanks

christophe lombard [off-list ref] writes:

Le 20/12/2017 =C3=A0 09:46, Vaibhav Jain a =C3=A9crit=C2=A0:

quoted

quoted

In fact, it does not matter. I don't know what the userspace could do
with this value.

Without libcxl knowing the tidr value, it cannot enforce the condition
that only threads that have called attach can issue 'wait' on the right
context.
=20
Also AFU can selectively ask PSL to issue asb_notify to a specific
thread via the PSL interface. Without userspace knowing the tidr value
it might not be easy for it to give this value to AFU through a Problem
State Area register.
=20

Don't forget that The ASB_Notify will use LPID:PID:TID tuple found
in the Process Element Entry.
The AFU may optionally provide a TID on AxH_CEA[40:55] (AxH_CEA[39]
must be set to indicate an AFU provided TID)
If AxH_CEA[39] =3D=3D 1=E2=80=99b0 then Process Element information
(LPID:PID:TID) is used to generate the PCIe address.
If AxH_CEA[39] =3D=3D 1=E2=80=99b1then the LPID:PID are taken from the PEE
while the TID is taken from AxH_-CEA[40:55]

Agree and that was the point I was trying to make when I said that AFU
can selectivly issue asb_notify to a thread. Without userspace threads
knowing their tidr libcxl would let any thread issue the 'wait'
instruction that may cause unpredictable results.

--=20
Vaibhav Jain [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.