LinuxLists.cc - [PATCH v5 0/7] ocxl: Implement Power9 as

[permalink] [raw]

Subject: [PATCH v5 4/7] ocxl: Rename pnv_ocxl_spa_remove_pe to clarify it's action

From: Alastair D'Silva <[email protected]>

The function removes the process element from NPU cache.

Signed-off-by: Alastair D'Silva <[email protected]>
---
arch/powerpc/include/asm/pnv-ocxl.h | 2 +-
arch/powerpc/platforms/powernv/ocxl.c | 4 ++--
drivers/misc/ocxl/link.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/pnv-ocxl.h b/arch/powerpc/include/asm/pnv-ocxl.h
index f6945d3bc971..208b5503f4ed 100644
--- a/arch/powerpc/include/asm/pnv-ocxl.h
+++ b/arch/powerpc/include/asm/pnv-ocxl.h
@@ -28,7 +28,7 @@ extern int pnv_ocxl_map_xsl_regs(struct pci_dev *dev, void __iomem **dsisr,
extern int pnv_ocxl_spa_setup(struct pci_dev *dev, void *spa_mem, int PE_mask,
void **platform_data);
extern void pnv_ocxl_spa_release(void *platform_data);
-extern int pnv_ocxl_spa_remove_pe(void *platform_data, int pe_handle);
+extern int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle);

extern int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr);
extern void pnv_ocxl_free_xive_irq(u32 irq);
diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
index fa9b53af3c7b..8c65aacda9c8 100644
--- a/arch/powerpc/platforms/powernv/ocxl.c
+++ b/arch/powerpc/platforms/powernv/ocxl.c
@@ -475,7 +475,7 @@ void pnv_ocxl_spa_release(void *platform_data)
}
EXPORT_SYMBOL_GPL(pnv_ocxl_spa_release);

-int pnv_ocxl_spa_remove_pe(void *platform_data, int pe_handle)
+int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)
{
struct spa_data *data = (struct spa_data *) platform_data;
int rc;
@@ -483,7 +483,7 @@ int pnv_ocxl_spa_remove_pe(void *platform_data, int pe_handle)
rc = opal_npu_spa_clear_cache(data->phb_opal_id, data->bdfn, pe_handle);
return rc;
}
-EXPORT_SYMBOL_GPL(pnv_ocxl_spa_remove_pe);
+EXPORT_SYMBOL_GPL(pnv_ocxl_spa_remove_pe_from_cache);

int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr)
{
diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
index f30790582dc0..656e8610eec2 100644
--- a/drivers/misc/ocxl/link.c
+++ b/drivers/misc/ocxl/link.c
@@ -599,7 +599,7 @@ int ocxl_link_remove_pe(void *link_handle, int pasid)
* On powerpc, the entry needs to be cleared from the context
* cache of the NPU.
*/
- rc = pnv_ocxl_spa_remove_pe(link->platform_data, pe_handle);
+ rc = pnv_ocxl_spa_remove_pe_from_cache(link->platform_data, pe_handle);
WARN_ON(rc);

pe_data = radix_tree_delete(&spa->pe_tree, pe_handle);
--
2.17.0

2018-05-11 06:15:58

[permalink] [raw]

Subject: [PATCH v5 3/7] powerpc: use task_pid_nr() for TID allocation

From: Alastair D'Silva <[email protected]>

The current implementation of TID allocation, using a global IDR, may
result in an errant process starving the system of available TIDs.
Instead, use task_pid_nr(), as mentioned by the original author. The
scenario described which prevented it's use is not applicable, as
set_thread_tidr can only be called after the task struct has been
populated.

In the unlikely event that 2 threads share the TID and are waiting,
all potential outcomes have been determined safe.

Signed-off-by: Alastair D'Silva <[email protected]>
---
arch/powerpc/include/asm/switch_to.h | 1 -
arch/powerpc/kernel/process.c | 122 ++++++---------------------
2 files changed, 28 insertions(+), 95 deletions(-)

diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
index be8c9fa23983..5b03d8a82409 100644
--- a/arch/powerpc/include/asm/switch_to.h
+++ b/arch/powerpc/include/asm/switch_to.h
@@ -94,6 +94,5 @@ static inline void clear_task_ebb(struct task_struct *t)
extern int set_thread_uses_vas(void);

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 3b00da47699b..c5b8e53acbae 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1496,103 +1496,41 @@ int set_thread_uses_vas(void)
}

#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.
+/**
+ * Assign a TIDR (thread ID) for task @t and set it in the thread
+ * structure. For now, we only support setting TIDR for 'current' task.
*
- * 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.
+ * Since the TID value is a truncated form of it PID, it is possible
+ * (but unlikely) for 2 threads to have the same TID. In the unlikely event
+ * that 2 threads share the same TID and are waiting, one of the following
+ * cases will happen:
*
- * 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.
+ * 1. The correct thread is running, the wrong thread is not
+ * In this situation, the correct thread is woken and proceeds to pass it's
+ * condition check.
*
- * 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()?.
+ * 2. Neither threads are running
+ * In this situation, neither thread will be woken. When scheduled, the waiting
+ * threads will execute either a wait, which will return immediately, followed
+ * by a condition check, which will pass for the correct thread and fail
+ * for the wrong thread, or they will execute the condition check immediately.
*
- * 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.
+ * 3. The wrong thread is running, the correct thread is not
+ * The wrong thread will be woken, but will fail it's condition check and
+ * re-execute wait. The correct thread, when scheduled, will execute either
+ * it's condition check (which will pass), or wait, which returns immediately
+ * when called the first time after the thread is scheduled, followed by it's
+ * condition check (which will pass).
*
- * 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;
- unsigned long flags;
-
-again:
- if (!ida_pre_get(&vas_thread_ida, GFP_KERNEL))
- return -ENOMEM;
-
- spin_lock_irqsave(&vas_thread_id_lock, flags);
- err = ida_get_new_above(&vas_thread_ida, 1, &index);
- spin_unlock_irqrestore(&vas_thread_id_lock, flags);
-
- if (err == -EAGAIN)
- goto again;
- else if (err)
- return err;
-
- if (index > MAX_THREAD_CONTEXT) {
- spin_lock_irqsave(&vas_thread_id_lock, flags);
- ida_remove(&vas_thread_ida, index);
- spin_unlock_irqrestore(&vas_thread_id_lock, flags);
- return -ENOMEM;
- }
-
- return index;
-}
-
-static void free_thread_tidr(int id)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&vas_thread_id_lock, flags);
- ida_remove(&vas_thread_ida, id);
- spin_unlock_irqrestore(&vas_thread_id_lock, flags);
-}
-
-/*
- * Clear any TIDR value assigned to this thread.
- */
-void clear_thread_tidr(struct task_struct *t)
-{
- if (!t->thread.tidr)
- return;
-
- if (!cpu_has_feature(CPU_FTR_P9_TIDR)) {
- WARN_ON_ONCE(1);
- return;
- }
-
- mtspr(SPRN_TIDR, 0);
- free_thread_tidr(t->thread.tidr);
- t->thread.tidr = 0;
-}
-
-void arch_release_task_struct(struct task_struct *t)
-{
- clear_thread_tidr(t);
-}
-
-/*
- * Assign a unique TIDR (thread id) for task @t and set it in the thread
- * structure. For now, we only support setting TIDR for 'current' task.
+ * 4. Both threads are running
+ * Both threads will be woken. The wrong thread will fail it's condition check
+ * and execute another wait, while the correct thread will pass it's condition
+ * check.
+ *
+ * @t: the task to set the thread ID for
*/
int set_thread_tidr(struct task_struct *t)
{
- int rc;
-
if (!cpu_has_feature(CPU_FTR_P9_TIDR))
return -EINVAL;

@@ -1602,11 +1540,7 @@ int set_thread_tidr(struct task_struct *t)
if (t->thread.tidr)
return 0;

- rc = assign_thread_tidr();
- if (rc < 0)
- return rc;
-
- t->thread.tidr = rc;
+ t->thread.tidr = (u16)task_pid_nr(t);
mtspr(SPRN_TIDR, t->thread.tidr);

return 0;
--
2.17.0

2018-05-11 06:16:43

[permalink] [raw]

Subject: [PATCH v5 6/7] ocxl: Add an IOCTL so userspace knows what OCXL features are available

From: Alastair D'Silva <[email protected]>

In order for a userspace AFU driver to call the POWER9 specific
OCXL_IOCTL_ENABLE_P9_WAIT, it needs to verify that it can actually
make that call.

Signed-off-by: Alastair D'Silva <[email protected]>
---
drivers/misc/ocxl/file.c | 25 +++++++++++++++++++++++++
include/uapi/misc/ocxl.h | 6 ++++++
2 files changed, 31 insertions(+)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index eb409a469f21..33ae46ce0a8a 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c
@@ -168,12 +168,32 @@ static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx,
}
#endif

+
+static long afu_ioctl_get_features(struct ocxl_context *ctx,
+ struct ocxl_ioctl_features __user *uarg)
+{
+ struct ocxl_ioctl_features arg;
+
+ memset(&arg, 0, sizeof(arg));
+
+#ifdef CONFIG_PPC64
+ if (cpu_has_feature(CPU_FTR_P9_TIDR))
+ arg.flags[0] |= OCXL_IOCTL_FEATURES_FLAGS0_P9_WAIT;
+#endif
+
+ if (copy_to_user(uarg, &arg, sizeof(arg)))
+ return -EFAULT;
+
+ return 0;
+}
+
#define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH" : \
x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" : \
x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" : \
x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" : \
x == OCXL_IOCTL_GET_METADATA ? "GET_METADATA" : \
x == OCXL_IOCTL_ENABLE_P9_WAIT ? "ENABLE_P9_WAIT" : \
+ x == OCXL_IOCTL_GET_FEATURES ? "GET_FEATURES" : \
"UNKNOWN")

static long afu_ioctl(struct file *file, unsigned int cmd,
@@ -239,6 +259,11 @@ static long afu_ioctl(struct file *file, unsigned int cmd,
break;
#endif

+ case OCXL_IOCTL_GET_FEATURES:
+ rc = afu_ioctl_get_features(ctx,
+ (struct ocxl_ioctl_features __user *) args);
+ break;
+
default:
rc = -EINVAL;
}
diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
index 561e6f0dfcb7..97937cfa3baa 100644
--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h
@@ -55,6 +55,11 @@ struct ocxl_ioctl_p9_wait {
__u64 reserved3[3];
};

+#define OCXL_IOCTL_FEATURES_FLAGS0_P9_WAIT 0x01
+struct ocxl_ioctl_features {
+ __u64 flags[4];
+};
+
struct ocxl_ioctl_irq_fd {
__u64 irq_offset;
__s32 eventfd;
@@ -70,5 +75,6 @@ struct ocxl_ioctl_irq_fd {
#define OCXL_IOCTL_IRQ_SET_FD _IOW(OCXL_MAGIC, 0x13, struct ocxl_ioctl_irq_fd)
#define OCXL_IOCTL_GET_METADATA _IOR(OCXL_MAGIC, 0x14, struct ocxl_ioctl_metadata)
#define OCXL_IOCTL_ENABLE_P9_WAIT _IOR(OCXL_MAGIC, 0x15, struct ocxl_ioctl_p9_wait)
+#define OCXL_IOCTL_GET_FEATURES _IOR(OCXL_MAGIC, 0x16, struct ocxl_ioctl_features)

#endif /* _UAPI_MISC_OCXL_H */
--
2.17.0

2018-05-11 06:17:00

[permalink] [raw]

Subject: [PATCH v5 5/7] ocxl: Expose the thread_id needed for wait on POWER9

From: Alastair D'Silva <[email protected]>

In order to successfully issue as_notify, an AFU needs to know the TID
to notify, which in turn means that this information should be
available in userspace so it can be communicated to the AFU.

Signed-off-by: Alastair D'Silva <[email protected]>
---
drivers/misc/ocxl/context.c | 5 ++-
drivers/misc/ocxl/file.c | 53 +++++++++++++++++++++++++++++++
drivers/misc/ocxl/link.c | 36 +++++++++++++++++++++
drivers/misc/ocxl/ocxl_internal.h | 1 +
include/misc/ocxl.h | 9 ++++++
include/uapi/misc/ocxl.h | 8 +++++
6 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
index 909e8807824a..95f74623113e 100644
--- a/drivers/misc/ocxl/context.c
+++ b/drivers/misc/ocxl/context.c
@@ -34,6 +34,8 @@ int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
mutex_init(&ctx->xsl_error_lock);
mutex_init(&ctx->irq_lock);
idr_init(&ctx->irq_idr);
+ ctx->tidr = 0;
+
/*
* Keep a reference on the AFU to make sure it's valid for the
* duration of the life of the context
@@ -65,6 +67,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
{
int rc;

+ // Locks both status & tidr
mutex_lock(&ctx->status_mutex);
if (ctx->status != OPENED) {
rc = -EIO;
@@ -72,7 +75,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
}

rc = ocxl_link_add_pe(ctx->afu->fn->link, ctx->pasid,
- current->mm->context.id, 0, amr, current->mm,
+ current->mm->context.id, ctx->tidr, amr, current->mm,
xsl_fault_error, ctx);
if (rc)
goto out;
diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index 038509e5d031..eb409a469f21 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c
@@ -5,6 +5,8 @@
#include <linux/sched/signal.h>
#include <linux/uaccess.h>
#include <uapi/misc/ocxl.h>
+#include <asm/reg.h>
+#include <asm/switch_to.h>
#include "ocxl_internal.h"

@@ -123,11 +125,55 @@ static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
return 0;
}

+#ifdef CONFIG_PPC64
+static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx,
+ struct ocxl_ioctl_p9_wait __user *uarg)
+{
+ struct ocxl_ioctl_p9_wait arg;
+
+ memset(&arg, 0, sizeof(arg));
+
+ if (cpu_has_feature(CPU_FTR_P9_TIDR)) {
+ enum ocxl_context_status status;
+
+ // Locks both status & tidr
+ mutex_lock(&ctx->status_mutex);
+ if (!ctx->tidr) {
+ if (set_thread_tidr(current))
+ return -ENOENT;
+
+ ctx->tidr = current->thread.tidr;
+ }
+
+ status = ctx->status;
+ mutex_unlock(&ctx->status_mutex);
+
+ if (status == ATTACHED) {
+ int rc;
+ struct link *link = ctx->afu->fn->link;
+
+ rc = ocxl_link_update_pe(link, ctx->pasid, ctx->tidr);
+ if (rc)
+ return rc;
+ }
+
+ arg.thread_id = ctx->tidr;
+ } else
+ return -ENOENT;
+
+ if (copy_to_user(uarg, &arg, sizeof(arg)))
+ return -EFAULT;
+
+ return 0;
+}
+#endif
+
#define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH" : \
x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" : \
x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" : \
x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" : \
x == OCXL_IOCTL_GET_METADATA ? "GET_METADATA" : \
+ x == OCXL_IOCTL_ENABLE_P9_WAIT ? "ENABLE_P9_WAIT" : \
"UNKNOWN")

static long afu_ioctl(struct file *file, unsigned int cmd,
@@ -186,6 +232,13 @@ static long afu_ioctl(struct file *file, unsigned int cmd,
(struct ocxl_ioctl_metadata __user *) args);
break;

+#ifdef CONFIG_PPC64
+ case OCXL_IOCTL_ENABLE_P9_WAIT:
+ rc = afu_ioctl_enable_p9_wait(ctx,
+ (struct ocxl_ioctl_p9_wait __user *) args);
+ break;
+#endif
+
default:
rc = -EINVAL;
}
diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
index 656e8610eec2..88876ae8f330 100644
--- a/drivers/misc/ocxl/link.c
+++ b/drivers/misc/ocxl/link.c
@@ -544,6 +544,42 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
}
EXPORT_SYMBOL_GPL(ocxl_link_add_pe);

+int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid)
+{
+ struct link *link = (struct link *) link_handle;
+ struct spa *spa = link->spa;
+ struct ocxl_process_element *pe;
+ int pe_handle, rc;
+
+ if (pasid > SPA_PASID_MAX)
+ return -EINVAL;
+
+ pe_handle = pasid & SPA_PE_MASK;
+ pe = spa->spa_mem + pe_handle;
+
+ mutex_lock(&spa->spa_lock);
+
+ pe->tid = tid;
+
+ /*
+ * The barrier makes sure the PE is updated
+ * before we clear the NPU context cache below, so that the
+ * old PE cannot be reloaded erroneously.
+ */
+ mb();
+
+ /*
+ * hook to platform code
+ * On powerpc, the entry needs to be cleared from the context
+ * cache of the NPU.
+ */
+ rc = pnv_ocxl_spa_remove_pe_from_cache(link->platform_data, pe_handle);
+ WARN_ON(rc);
+
+ mutex_unlock(&spa->spa_lock);
+ return rc;
+}
+
int ocxl_link_remove_pe(void *link_handle, int pasid)
{
struct link *link = (struct link *) link_handle;
diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
index 5d421824afd9..a32f2151029f 100644
--- a/drivers/misc/ocxl/ocxl_internal.h
+++ b/drivers/misc/ocxl/ocxl_internal.h
@@ -77,6 +77,7 @@ struct ocxl_context {
struct ocxl_xsl_error xsl_error;
struct mutex irq_lock;
struct idr irq_idr;
+ u16 tidr; // Thread ID used for P9 wait implementation
};

struct ocxl_process_element {
diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
index 51ccf76db293..9ff6ddc28e22 100644
--- a/include/misc/ocxl.h
+++ b/include/misc/ocxl.h
@@ -188,6 +188,15 @@ extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
void *xsl_err_data);

+/**
+ * Update values within a Process Element
+ *
+ * link_handle: the link handle associated with the process element
+ * pasid: the PASID for the AFU context
+ * tid: the new thread id for the process element
+ */
+extern int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
+
/*
* Remove a Process Element from the Shared Process Area for a link
*/
diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
index 0af83d80fb3e..561e6f0dfcb7 100644
--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h
@@ -48,6 +48,13 @@ struct ocxl_ioctl_metadata {
__u64 reserved[13]; // Total of 16*u64
};

+struct ocxl_ioctl_p9_wait {
+ __u16 thread_id; // The thread ID required to wake this thread
+ __u16 reserved1;
+ __u32 reserved2;
+ __u64 reserved3[3];
+};
+
struct ocxl_ioctl_irq_fd {
__u64 irq_offset;
__s32 eventfd;
@@ -62,5 +69,6 @@ struct ocxl_ioctl_irq_fd {
#define OCXL_IOCTL_IRQ_FREE _IOW(OCXL_MAGIC, 0x12, __u64)
#define OCXL_IOCTL_IRQ_SET_FD _IOW(OCXL_MAGIC, 0x13, struct ocxl_ioctl_irq_fd)
#define OCXL_IOCTL_GET_METADATA _IOR(OCXL_MAGIC, 0x14, struct ocxl_ioctl_metadata)
+#define OCXL_IOCTL_ENABLE_P9_WAIT _IOR(OCXL_MAGIC, 0x15, struct ocxl_ioctl_p9_wait)

#endif /* _UAPI_MISC_OCXL_H */
--
2.17.0

2018-05-11 06:17:17

[permalink] [raw]

Subject: [PATCH v5 1/7] powerpc: Add TIDR CPU feature for POWER9

From: Alastair D'Silva <[email protected]>

This patch adds a CPU feature bit to show whether the CPU has
the TIDR register available, enabling as_notify/wait in userspace.

Signed-off-by: Alastair D'Silva <[email protected]>
---
arch/powerpc/include/asm/cputable.h | 3 ++-
arch/powerpc/kernel/dt_cpu_ftrs.c | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index 66fcab13c8b4..9c0a3083571b 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -215,6 +215,7 @@ static inline void cpu_feature_keys_init(void) { }
#define CPU_FTR_P9_TM_HV_ASSIST LONG_ASM_CONST(0x0000100000000000)
#define CPU_FTR_P9_TM_XER_SO_BUG LONG_ASM_CONST(0x0000200000000000)
#define CPU_FTR_P9_TLBIE_BUG LONG_ASM_CONST(0x0000400000000000)
+#define CPU_FTR_P9_TIDR LONG_ASM_CONST(0x0000800000000000)

#ifndef __ASSEMBLY__

@@ -462,7 +463,7 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_PKEY | \
- CPU_FTR_P9_TLBIE_BUG)
+ CPU_FTR_P9_TLBIE_BUG | CPU_FTR_P9_TIDR)
#define CPU_FTRS_POWER9_DD1 ((CPU_FTRS_POWER9 | CPU_FTR_POWER9_DD1) & \
(~CPU_FTR_SAO))
#define CPU_FTRS_POWER9_DD2_0 CPU_FTRS_POWER9
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 8ab51f6ca03a..415555e5b69f 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -716,6 +716,7 @@ static __init void cpufeatures_cpu_quirks(void)
if ((version & 0xffff0000) == 0x004e0000) {
cur_cpu_spec->cpu_features &= ~(CPU_FTR_DAWR);
cur_cpu_spec->cpu_features |= CPU_FTR_P9_TLBIE_BUG;
+ cur_cpu_spec->cpu_features |= CPU_FTR_P9_TIDR;
}

/*
--
2.17.0

2018-05-11 06:17:21

[permalink] [raw]

Subject: [PATCH v5 7/7] ocxl: Document new OCXL IOCTLs

From: Alastair D'Silva <[email protected]>

Signed-off-by: Alastair D'Silva <[email protected]>
---
Documentation/accelerators/ocxl.rst | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/Documentation/accelerators/ocxl.rst b/Documentation/accelerators/ocxl.rst
index ddcc58d01cfb..14cefc020e2d 100644
--- a/Documentation/accelerators/ocxl.rst
+++ b/Documentation/accelerators/ocxl.rst
@@ -157,6 +157,17 @@ OCXL_IOCTL_GET_METADATA:
Obtains configuration information from the card, such at the size of
MMIO areas, the AFU version, and the PASID for the current context.

+OCXL_IOCTL_ENABLE_P9_WAIT:
+
+ Allows the AFU to wake a userspace thread executing 'wait'. Returns
+ information to userspace to allow it to configure the AFU. Note that
+ this is only available on POWER9.
+
+OCXL_IOCTL_GET_FEATURES:
+
+ Reports on which CPU features that affect OpenCAPI are usable from
+ userspace.
+

mmap
----
--
2.17.0

2018-05-11 06:17:40

[permalink] [raw]

Subject: [PATCH v5 2/7] powerpc: Use TIDR CPU feature to control TIDR allocation

From: Alastair D'Silva <[email protected]>

Switch the use of TIDR on it's CPU feature, rather than assuming it
is available based on architecture.

Signed-off-by: Alastair D'Silva <[email protected]>
---
arch/powerpc/kernel/process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 1237f13fed51..3b00da47699b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1154,7 +1154,7 @@ static inline void restore_sprs(struct thread_struct *old_thread,
mtspr(SPRN_TAR, new_thread->tar);
}

- if (cpu_has_feature(CPU_FTR_ARCH_300) &&
+ if (cpu_has_feature(CPU_FTR_P9_TIDR) &&
old_thread->tidr != new_thread->tidr)
mtspr(SPRN_TIDR, new_thread->tidr);
#endif
@@ -1570,7 +1570,7 @@ void clear_thread_tidr(struct task_struct *t)
if (!t->thread.tidr)
return;

- if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
+ if (!cpu_has_feature(CPU_FTR_P9_TIDR)) {
WARN_ON_ONCE(1);
return;
}
@@ -1593,7 +1593,7 @@ int set_thread_tidr(struct task_struct *t)
{
int rc;

- if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ if (!cpu_has_feature(CPU_FTR_P9_TIDR))
return -EINVAL;

if (t != current)
--
2.17.0

2018-05-11 08:40:49

[permalink] [raw]

Subject: Re: [PATCH v5 1/7] powerpc: Add TIDR CPU feature for POWER9

Le 11/05/2018 à 08:12, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> This patch adds a CPU feature bit to show whether the CPU has
> the TIDR register available, enabling as_notify/wait in userspace.
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---

Reviewed-by: Frederic Barrat <[email protected]>

> arch/powerpc/include/asm/cputable.h | 3 ++-
> arch/powerpc/kernel/dt_cpu_ftrs.c | 1 +
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index 66fcab13c8b4..9c0a3083571b 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -215,6 +215,7 @@ static inline void cpu_feature_keys_init(void) { }
> #define CPU_FTR_P9_TM_HV_ASSIST LONG_ASM_CONST(0x0000100000000000)
> #define CPU_FTR_P9_TM_XER_SO_BUG LONG_ASM_CONST(0x0000200000000000)
> #define CPU_FTR_P9_TLBIE_BUG LONG_ASM_CONST(0x0000400000000000)
> +#define CPU_FTR_P9_TIDR LONG_ASM_CONST(0x0000800000000000)
>
> #ifndef __ASSEMBLY__
>
> @@ -462,7 +463,7 @@ static inline void cpu_feature_keys_init(void) { }
> CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
> CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
> CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_PKEY | \
> - CPU_FTR_P9_TLBIE_BUG)
> + CPU_FTR_P9_TLBIE_BUG | CPU_FTR_P9_TIDR)
> #define CPU_FTRS_POWER9_DD1 ((CPU_FTRS_POWER9 | CPU_FTR_POWER9_DD1) & \
> (~CPU_FTR_SAO))
> #define CPU_FTRS_POWER9_DD2_0 CPU_FTRS_POWER9
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 8ab51f6ca03a..415555e5b69f 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -716,6 +716,7 @@ static __init void cpufeatures_cpu_quirks(void)
> if ((version & 0xffff0000) == 0x004e0000) {
> cur_cpu_spec->cpu_features &= ~(CPU_FTR_DAWR);
> cur_cpu_spec->cpu_features |= CPU_FTR_P9_TLBIE_BUG;
> + cur_cpu_spec->cpu_features |= CPU_FTR_P9_TIDR;
> }
>
> /*
>

2018-05-11 08:47:20

[permalink] [raw]

Subject: Re: [PATCH v5 2/7] powerpc: Use TIDR CPU feature to control TIDR allocation

Le 11/05/2018 à 08:12, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> Switch the use of TIDR on it's CPU feature, rather than assuming it
> is available based on architecture.
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---

Reviewed-by: Frederic Barrat <[email protected]>

> arch/powerpc/kernel/process.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 1237f13fed51..3b00da47699b 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1154,7 +1154,7 @@ static inline void restore_sprs(struct thread_struct *old_thread,
> mtspr(SPRN_TAR, new_thread->tar);
> }
>
> - if (cpu_has_feature(CPU_FTR_ARCH_300) &&
> + if (cpu_has_feature(CPU_FTR_P9_TIDR) &&
> old_thread->tidr != new_thread->tidr)
> mtspr(SPRN_TIDR, new_thread->tidr);
> #endif
> @@ -1570,7 +1570,7 @@ void clear_thread_tidr(struct task_struct *t)
> if (!t->thread.tidr)
> return;
>
> - if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
> + if (!cpu_has_feature(CPU_FTR_P9_TIDR)) {
> WARN_ON_ONCE(1);
> return;
> }
> @@ -1593,7 +1593,7 @@ int set_thread_tidr(struct task_struct *t)
> {
> int rc;
>
> - if (!cpu_has_feature(CPU_FTR_ARCH_300))
> + if (!cpu_has_feature(CPU_FTR_P9_TIDR))
> return -EINVAL;
>
> if (t != current)
>

2018-05-11 08:49:41

[permalink] [raw]

Subject: Re: [PATCH v5 3/7] powerpc: use task_pid_nr() for TID allocation

Le 11/05/2018 à 08:12, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> The current implementation of TID allocation, using a global IDR, may
> result in an errant process starving the system of available TIDs.
> Instead, use task_pid_nr(), as mentioned by the original author. The
> scenario described which prevented it's use is not applicable, as
> set_thread_tidr can only be called after the task struct has been
> populated.
>
> In the unlikely event that 2 threads share the TID and are waiting,
> all potential outcomes have been determined safe.
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---

Thanks for adding the comment. It assumes the reader is aware that the
TIDR value is only used for the notification using the 'wait'
instruction, but that's likely to be the case.

Reviewed-by: Frederic Barrat <[email protected]>

> arch/powerpc/include/asm/switch_to.h | 1 -
> arch/powerpc/kernel/process.c | 122 ++++++---------------------
> 2 files changed, 28 insertions(+), 95 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
> index be8c9fa23983..5b03d8a82409 100644
> --- a/arch/powerpc/include/asm/switch_to.h
> +++ b/arch/powerpc/include/asm/switch_to.h
> @@ -94,6 +94,5 @@ static inline void clear_task_ebb(struct task_struct *t)
> extern int set_thread_uses_vas(void);
>
> 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 3b00da47699b..c5b8e53acbae 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1496,103 +1496,41 @@ int set_thread_uses_vas(void)
> }
>
> #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.
> +/**
> + * Assign a TIDR (thread ID) for task @t and set it in the thread
> + * structure. For now, we only support setting TIDR for 'current' task.
> *
> - * 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.
> + * Since the TID value is a truncated form of it PID, it is possible
> + * (but unlikely) for 2 threads to have the same TID. In the unlikely event
> + * that 2 threads share the same TID and are waiting, one of the following
> + * cases will happen:
> *
> - * 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.
> + * 1. The correct thread is running, the wrong thread is not
> + * In this situation, the correct thread is woken and proceeds to pass it's
> + * condition check.
> *
> - * 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()?.
> + * 2. Neither threads are running
> + * In this situation, neither thread will be woken. When scheduled, the waiting
> + * threads will execute either a wait, which will return immediately, followed
> + * by a condition check, which will pass for the correct thread and fail
> + * for the wrong thread, or they will execute the condition check immediately.
> *
> - * 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.
> + * 3. The wrong thread is running, the correct thread is not
> + * The wrong thread will be woken, but will fail it's condition check and
> + * re-execute wait. The correct thread, when scheduled, will execute either
> + * it's condition check (which will pass), or wait, which returns immediately
> + * when called the first time after the thread is scheduled, followed by it's
> + * condition check (which will pass).
> *
> - * 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;
> - unsigned long flags;
> -
> -again:
> - if (!ida_pre_get(&vas_thread_ida, GFP_KERNEL))
> - return -ENOMEM;
> -
> - spin_lock_irqsave(&vas_thread_id_lock, flags);
> - err = ida_get_new_above(&vas_thread_ida, 1, &index);
> - spin_unlock_irqrestore(&vas_thread_id_lock, flags);
> -
> - if (err == -EAGAIN)
> - goto again;
> - else if (err)
> - return err;
> -
> - if (index > MAX_THREAD_CONTEXT) {
> - spin_lock_irqsave(&vas_thread_id_lock, flags);
> - ida_remove(&vas_thread_ida, index);
> - spin_unlock_irqrestore(&vas_thread_id_lock, flags);
> - return -ENOMEM;
> - }
> -
> - return index;
> -}
> -
> -static void free_thread_tidr(int id)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&vas_thread_id_lock, flags);
> - ida_remove(&vas_thread_ida, id);
> - spin_unlock_irqrestore(&vas_thread_id_lock, flags);
> -}
> -
> -/*
> - * Clear any TIDR value assigned to this thread.
> - */
> -void clear_thread_tidr(struct task_struct *t)
> -{
> - if (!t->thread.tidr)
> - return;
> -
> - if (!cpu_has_feature(CPU_FTR_P9_TIDR)) {
> - WARN_ON_ONCE(1);
> - return;
> - }
> -
> - mtspr(SPRN_TIDR, 0);
> - free_thread_tidr(t->thread.tidr);
> - t->thread.tidr = 0;
> -}
> -
> -void arch_release_task_struct(struct task_struct *t)
> -{
> - clear_thread_tidr(t);
> -}
> -
> -/*
> - * Assign a unique TIDR (thread id) for task @t and set it in the thread
> - * structure. For now, we only support setting TIDR for 'current' task.
> + * 4. Both threads are running
> + * Both threads will be woken. The wrong thread will fail it's condition check
> + * and execute another wait, while the correct thread will pass it's condition
> + * check.
> + *
> + * @t: the task to set the thread ID for
> */
> int set_thread_tidr(struct task_struct *t)
> {
> - int rc;
> -
> if (!cpu_has_feature(CPU_FTR_P9_TIDR))
> return -EINVAL;
>
> @@ -1602,11 +1540,7 @@ int set_thread_tidr(struct task_struct *t)
> if (t->thread.tidr)
> return 0;
>
> - rc = assign_thread_tidr();
> - if (rc < 0)
> - return rc;
> -
> - t->thread.tidr = rc;
> + t->thread.tidr = (u16)task_pid_nr(t);
> mtspr(SPRN_TIDR, t->thread.tidr);
>
> return 0;
>

2018-05-11 08:51:13

[permalink] [raw]

Subject: Re: [PATCH v5 4/7] ocxl: Rename pnv_ocxl_spa_remove_pe to clarify it's action

Le 11/05/2018 à 08:13, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> The function removes the process element from NPU cache.
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---

Acked-by: Frederic Barrat <[email protected]>

> arch/powerpc/include/asm/pnv-ocxl.h | 2 +-
> arch/powerpc/platforms/powernv/ocxl.c | 4 ++--
> drivers/misc/ocxl/link.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pnv-ocxl.h b/arch/powerpc/include/asm/pnv-ocxl.h
> index f6945d3bc971..208b5503f4ed 100644
> --- a/arch/powerpc/include/asm/pnv-ocxl.h
> +++ b/arch/powerpc/include/asm/pnv-ocxl.h
> @@ -28,7 +28,7 @@ extern int pnv_ocxl_map_xsl_regs(struct pci_dev *dev, void __iomem **dsisr,
> extern int pnv_ocxl_spa_setup(struct pci_dev *dev, void *spa_mem, int PE_mask,
> void **platform_data);
> extern void pnv_ocxl_spa_release(void *platform_data);
> -extern int pnv_ocxl_spa_remove_pe(void *platform_data, int pe_handle);
> +extern int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle);
>
> extern int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr);
> extern void pnv_ocxl_free_xive_irq(u32 irq);
> diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
> index fa9b53af3c7b..8c65aacda9c8 100644
> --- a/arch/powerpc/platforms/powernv/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/ocxl.c
> @@ -475,7 +475,7 @@ void pnv_ocxl_spa_release(void *platform_data)
> }
> EXPORT_SYMBOL_GPL(pnv_ocxl_spa_release);
>
> -int pnv_ocxl_spa_remove_pe(void *platform_data, int pe_handle)
> +int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)
> {
> struct spa_data *data = (struct spa_data *) platform_data;
> int rc;
> @@ -483,7 +483,7 @@ int pnv_ocxl_spa_remove_pe(void *platform_data, int pe_handle)
> rc = opal_npu_spa_clear_cache(data->phb_opal_id, data->bdfn, pe_handle);
> return rc;
> }
> -EXPORT_SYMBOL_GPL(pnv_ocxl_spa_remove_pe);
> +EXPORT_SYMBOL_GPL(pnv_ocxl_spa_remove_pe_from_cache);
>
> int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr)
> {
> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> index f30790582dc0..656e8610eec2 100644
> --- a/drivers/misc/ocxl/link.c
> +++ b/drivers/misc/ocxl/link.c
> @@ -599,7 +599,7 @@ int ocxl_link_remove_pe(void *link_handle, int pasid)
> * On powerpc, the entry needs to be cleared from the context
> * cache of the NPU.
> */
> - rc = pnv_ocxl_spa_remove_pe(link->platform_data, pe_handle);
> + rc = pnv_ocxl_spa_remove_pe_from_cache(link->platform_data, pe_handle);
> WARN_ON(rc);
>
> pe_data = radix_tree_delete(&spa->pe_tree, pe_handle);
>

2018-05-11 09:26:52

[permalink] [raw]

Subject: Re: [PATCH v5 5/7] ocxl: Expose the thread_id needed for wait on POWER9

Le 11/05/2018 à 08:13, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> In order to successfully issue as_notify, an AFU needs to know the TID
> to notify, which in turn means that this information should be
> available in userspace so it can be communicated to the AFU.
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---

Ok, so we keep the limitation of having only one thread per context able
to call 'wait', even though we don't have to worry about depleting the
pool of TIDs any more. I think that's acceptable, though we don't really
have a reason to justify it any more. Any reason you want to keep it
that way?

Fred

> drivers/misc/ocxl/context.c | 5 ++-
> drivers/misc/ocxl/file.c | 53 +++++++++++++++++++++++++++++++
> drivers/misc/ocxl/link.c | 36 +++++++++++++++++++++
> drivers/misc/ocxl/ocxl_internal.h | 1 +
> include/misc/ocxl.h | 9 ++++++
> include/uapi/misc/ocxl.h | 8 +++++
> 6 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
> index 909e8807824a..95f74623113e 100644
> --- a/drivers/misc/ocxl/context.c
> +++ b/drivers/misc/ocxl/context.c
> @@ -34,6 +34,8 @@ int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> mutex_init(&ctx->xsl_error_lock);
> mutex_init(&ctx->irq_lock);
> idr_init(&ctx->irq_idr);
> + ctx->tidr = 0;
> +
> /*
> * Keep a reference on the AFU to make sure it's valid for the
> * duration of the life of the context
> @@ -65,6 +67,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
> {
> int rc;
>
> + // Locks both status & tidr
> mutex_lock(&ctx->status_mutex);
> if (ctx->status != OPENED) {
> rc = -EIO;
> @@ -72,7 +75,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
> }
>
> rc = ocxl_link_add_pe(ctx->afu->fn->link, ctx->pasid,
> - current->mm->context.id, 0, amr, current->mm,
> + current->mm->context.id, ctx->tidr, amr, current->mm,
> xsl_fault_error, ctx);
> if (rc)
> goto out;
> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
> index 038509e5d031..eb409a469f21 100644
> --- a/drivers/misc/ocxl/file.c
> +++ b/drivers/misc/ocxl/file.c
> @@ -5,6 +5,8 @@
> #include <linux/sched/signal.h>
> #include <linux/uaccess.h>
> #include <uapi/misc/ocxl.h>
> +#include <asm/reg.h>
> +#include <asm/switch_to.h>
> #include "ocxl_internal.h"
>
>
> @@ -123,11 +125,55 @@ static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
> return 0;
> }
>
> +#ifdef CONFIG_PPC64
> +static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx,
> + struct ocxl_ioctl_p9_wait __user *uarg)
> +{
> + struct ocxl_ioctl_p9_wait arg;
> +
> + memset(&arg, 0, sizeof(arg));
> +
> + if (cpu_has_feature(CPU_FTR_P9_TIDR)) {
> + enum ocxl_context_status status;
> +
> + // Locks both status & tidr
> + mutex_lock(&ctx->status_mutex);
> + if (!ctx->tidr) {
> + if (set_thread_tidr(current))
> + return -ENOENT;
> +
> + ctx->tidr = current->thread.tidr;
> + }
> +
> + status = ctx->status;
> + mutex_unlock(&ctx->status_mutex);
> +
> + if (status == ATTACHED) {
> + int rc;
> + struct link *link = ctx->afu->fn->link;
> +
> + rc = ocxl_link_update_pe(link, ctx->pasid, ctx->tidr);
> + if (rc)
> + return rc;
> + }
> +
> + arg.thread_id = ctx->tidr;
> + } else
> + return -ENOENT;
> +
> + if (copy_to_user(uarg, &arg, sizeof(arg)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +#endif
> +
> #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH" : \
> x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" : \
> x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" : \
> x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" : \
> x == OCXL_IOCTL_GET_METADATA ? "GET_METADATA" : \
> + x == OCXL_IOCTL_ENABLE_P9_WAIT ? "ENABLE_P9_WAIT" : \
> "UNKNOWN")
>
> static long afu_ioctl(struct file *file, unsigned int cmd,
> @@ -186,6 +232,13 @@ static long afu_ioctl(struct file *file, unsigned int cmd,
> (struct ocxl_ioctl_metadata __user *) args);
> break;
>
> +#ifdef CONFIG_PPC64
> + case OCXL_IOCTL_ENABLE_P9_WAIT:
> + rc = afu_ioctl_enable_p9_wait(ctx,
> + (struct ocxl_ioctl_p9_wait __user *) args);
> + break;
> +#endif
> +
> default:
> rc = -EINVAL;
> }
> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> index 656e8610eec2..88876ae8f330 100644
> --- a/drivers/misc/ocxl/link.c
> +++ b/drivers/misc/ocxl/link.c
> @@ -544,6 +544,42 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> }
> EXPORT_SYMBOL_GPL(ocxl_link_add_pe);
>
> +int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid)
> +{
> + struct link *link = (struct link *) link_handle;
> + struct spa *spa = link->spa;
> + struct ocxl_process_element *pe;
> + int pe_handle, rc;
> +
> + if (pasid > SPA_PASID_MAX)
> + return -EINVAL;
> +
> + pe_handle = pasid & SPA_PE_MASK;
> + pe = spa->spa_mem + pe_handle;
> +
> + mutex_lock(&spa->spa_lock);
> +
> + pe->tid = tid;
> +
> + /*
> + * The barrier makes sure the PE is updated
> + * before we clear the NPU context cache below, so that the
> + * old PE cannot be reloaded erroneously.
> + */
> + mb();
> +
> + /*
> + * hook to platform code
> + * On powerpc, the entry needs to be cleared from the context
> + * cache of the NPU.
> + */
> + rc = pnv_ocxl_spa_remove_pe_from_cache(link->platform_data, pe_handle);
> + WARN_ON(rc);
> +
> + mutex_unlock(&spa->spa_lock);
> + return rc;
> +}
> +
> int ocxl_link_remove_pe(void *link_handle, int pasid)
> {
> struct link *link = (struct link *) link_handle;
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 5d421824afd9..a32f2151029f 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -77,6 +77,7 @@ struct ocxl_context {
> struct ocxl_xsl_error xsl_error;
> struct mutex irq_lock;
> struct idr irq_idr;
> + u16 tidr; // Thread ID used for P9 wait implementation
> };
>
> struct ocxl_process_element {
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 51ccf76db293..9ff6ddc28e22 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -188,6 +188,15 @@ extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
> void *xsl_err_data);
>
> +/**
> + * Update values within a Process Element
> + *
> + * link_handle: the link handle associated with the process element
> + * pasid: the PASID for the AFU context
> + * tid: the new thread id for the process element
> + */
> +extern int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> +
> /*
> * Remove a Process Element from the Shared Process Area for a link
> */
> diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
> index 0af83d80fb3e..561e6f0dfcb7 100644
> --- a/include/uapi/misc/ocxl.h
> +++ b/include/uapi/misc/ocxl.h
> @@ -48,6 +48,13 @@ struct ocxl_ioctl_metadata {
> __u64 reserved[13]; // Total of 16*u64
> };
>
> +struct ocxl_ioctl_p9_wait {
> + __u16 thread_id; // The thread ID required to wake this thread
> + __u16 reserved1;
> + __u32 reserved2;
> + __u64 reserved3[3];
> +};
> +
> struct ocxl_ioctl_irq_fd {
> __u64 irq_offset;
> __s32 eventfd;
> @@ -62,5 +69,6 @@ struct ocxl_ioctl_irq_fd {
> #define OCXL_IOCTL_IRQ_FREE _IOW(OCXL_MAGIC, 0x12, __u64)
> #define OCXL_IOCTL_IRQ_SET_FD _IOW(OCXL_MAGIC, 0x13, struct ocxl_ioctl_irq_fd)
> #define OCXL_IOCTL_GET_METADATA _IOR(OCXL_MAGIC, 0x14, struct ocxl_ioctl_metadata)
> +#define OCXL_IOCTL_ENABLE_P9_WAIT _IOR(OCXL_MAGIC, 0x15, struct ocxl_ioctl_p9_wait)
>
> #endif /* _UAPI_MISC_OCXL_H */
>

2018-05-11 09:29:29

[permalink] [raw]

Subject: Re: [PATCH v5 7/7] ocxl: Document new OCXL IOCTLs

Le 11/05/2018 à 08:13, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---

Acked-by: Frederic Barrat <[email protected]>

> Documentation/accelerators/ocxl.rst | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/accelerators/ocxl.rst b/Documentation/accelerators/ocxl.rst
> index ddcc58d01cfb..14cefc020e2d 100644
> --- a/Documentation/accelerators/ocxl.rst
> +++ b/Documentation/accelerators/ocxl.rst
> @@ -157,6 +157,17 @@ OCXL_IOCTL_GET_METADATA:
> Obtains configuration information from the card, such at the size of
> MMIO areas, the AFU version, and the PASID for the current context.
>
> +OCXL_IOCTL_ENABLE_P9_WAIT:
> +
> + Allows the AFU to wake a userspace thread executing 'wait'. Returns
> + information to userspace to allow it to configure the AFU. Note that
> + this is only available on POWER9.
> +
> +OCXL_IOCTL_GET_FEATURES:
> +
> + Reports on which CPU features that affect OpenCAPI are usable from
> + userspace.
> +
>
> mmap
> ----
>

2018-05-11 09:30:01

[permalink] [raw]

Subject: Re: [PATCH v5 6/7] ocxl: Add an IOCTL so userspace knows what OCXL features are available

Le 11/05/2018 à 08:13, Alastair D'Silva a écrit :
> From: Alastair D'Silva <[email protected]>
>
> In order for a userspace AFU driver to call the POWER9 specific
> OCXL_IOCTL_ENABLE_P9_WAIT, it needs to verify that it can actually
> make that call.
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---

Acked-by: Frederic Barrat <[email protected]>

> drivers/misc/ocxl/file.c | 25 +++++++++++++++++++++++++
> include/uapi/misc/ocxl.h | 6 ++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
> index eb409a469f21..33ae46ce0a8a 100644
> --- a/drivers/misc/ocxl/file.c
> +++ b/drivers/misc/ocxl/file.c
> @@ -168,12 +168,32 @@ static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx,
> }
> #endif
>
> +
> +static long afu_ioctl_get_features(struct ocxl_context *ctx,
> + struct ocxl_ioctl_features __user *uarg)
> +{
> + struct ocxl_ioctl_features arg;
> +
> + memset(&arg, 0, sizeof(arg));
> +
> +#ifdef CONFIG_PPC64
> + if (cpu_has_feature(CPU_FTR_P9_TIDR))
> + arg.flags[0] |= OCXL_IOCTL_FEATURES_FLAGS0_P9_WAIT;
> +#endif
> +
> + if (copy_to_user(uarg, &arg, sizeof(arg)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH" : \
> x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" : \
> x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" : \
> x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" : \
> x == OCXL_IOCTL_GET_METADATA ? "GET_METADATA" : \
> x == OCXL_IOCTL_ENABLE_P9_WAIT ? "ENABLE_P9_WAIT" : \
> + x == OCXL_IOCTL_GET_FEATURES ? "GET_FEATURES" : \
> "UNKNOWN")
>
> static long afu_ioctl(struct file *file, unsigned int cmd,
> @@ -239,6 +259,11 @@ static long afu_ioctl(struct file *file, unsigned int cmd,
> break;
> #endif
>
> + case OCXL_IOCTL_GET_FEATURES:
> + rc = afu_ioctl_get_features(ctx,
> + (struct ocxl_ioctl_features __user *) args);
> + break;
> +
> default:
> rc = -EINVAL;
> }
> diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
> index 561e6f0dfcb7..97937cfa3baa 100644
> --- a/include/uapi/misc/ocxl.h
> +++ b/include/uapi/misc/ocxl.h
> @@ -55,6 +55,11 @@ struct ocxl_ioctl_p9_wait {
> __u64 reserved3[3];
> };
>
> +#define OCXL_IOCTL_FEATURES_FLAGS0_P9_WAIT 0x01
> +struct ocxl_ioctl_features {
> + __u64 flags[4];
> +};
> +
> struct ocxl_ioctl_irq_fd {
> __u64 irq_offset;
> __s32 eventfd;
> @@ -70,5 +75,6 @@ struct ocxl_ioctl_irq_fd {
> #define OCXL_IOCTL_IRQ_SET_FD _IOW(OCXL_MAGIC, 0x13, struct ocxl_ioctl_irq_fd)
> #define OCXL_IOCTL_GET_METADATA _IOR(OCXL_MAGIC, 0x14, struct ocxl_ioctl_metadata)
> #define OCXL_IOCTL_ENABLE_P9_WAIT _IOR(OCXL_MAGIC, 0x15, struct ocxl_ioctl_p9_wait)
> +#define OCXL_IOCTL_GET_FEATURES _IOR(OCXL_MAGIC, 0x16, struct ocxl_ioctl_features)
>
> #endif /* _UAPI_MISC_OCXL_H */
>

2018-05-11 10:13:31

[permalink] [raw]

Subject: RE: [PATCH v5 5/7] ocxl: Expose the thread_id needed for wait on POWER9

> -----Original Message-----
> From: Frederic Barrat <[email protected]>
> Sent: Friday, 11 May 2018 7:25 PM
> To: Alastair D'Silva <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Alastair D'Silva <[email protected]>
> Subject: Re: [PATCH v5 5/7] ocxl: Expose the thread_id needed for wait on
> POWER9
>
>
>
> Le 11/05/2018 à 08:13, Alastair D'Silva a écrit :
> > From: Alastair D'Silva <[email protected]>
> >
> > In order to successfully issue as_notify, an AFU needs to know the TID
> > to notify, which in turn means that this information should be
> > available in userspace so it can be communicated to the AFU.
> >
> > Signed-off-by: Alastair D'Silva <[email protected]>
> > ---
>
> Ok, so we keep the limitation of having only one thread per context able to
> call 'wait', even though we don't have to worry about depleting the pool of
> TIDs any more. I think that's acceptable, though we don't really have a reason
> to justify it any more. Any reason you want to keep it that way?
>

No strong reason, just trying to minimise the amount of changes. We can always expand the scope later, if we have a use-case for it.

> Fred
>
>
> > drivers/misc/ocxl/context.c | 5 ++-
> > drivers/misc/ocxl/file.c | 53 +++++++++++++++++++++++++++++++
> > drivers/misc/ocxl/link.c | 36 +++++++++++++++++++++
> > drivers/misc/ocxl/ocxl_internal.h | 1 +
> > include/misc/ocxl.h | 9 ++++++
> > include/uapi/misc/ocxl.h | 8 +++++
> > 6 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
> > index 909e8807824a..95f74623113e 100644
> > --- a/drivers/misc/ocxl/context.c
> > +++ b/drivers/misc/ocxl/context.c
> > @@ -34,6 +34,8 @@ int ocxl_context_init(struct ocxl_context *ctx, struct
> ocxl_afu *afu,
> > mutex_init(&ctx->xsl_error_lock);
> > mutex_init(&ctx->irq_lock);
> > idr_init(&ctx->irq_idr);
> > + ctx->tidr = 0;
> > +
> > /*
> > * Keep a reference on the AFU to make sure it's valid for the
> > * duration of the life of the context @@ -65,6 +67,7 @@ int
> > ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
> > {
> > int rc;
> >
> > + // Locks both status & tidr
> > mutex_lock(&ctx->status_mutex);
> > if (ctx->status != OPENED) {
> > rc = -EIO;
> > @@ -72,7 +75,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64
> amr)
> > }
> >
> > rc = ocxl_link_add_pe(ctx->afu->fn->link, ctx->pasid,
> > - current->mm->context.id, 0, amr, current->mm,
> > + current->mm->context.id, ctx->tidr, amr, current-
> >mm,
> > xsl_fault_error, ctx);
> > if (rc)
> > goto out;
> > diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index
> > 038509e5d031..eb409a469f21 100644
> > --- a/drivers/misc/ocxl/file.c
> > +++ b/drivers/misc/ocxl/file.c
> > @@ -5,6 +5,8 @@
> > #include <linux/sched/signal.h>
> > #include <linux/uaccess.h>
> > #include <uapi/misc/ocxl.h>
> > +#include <asm/reg.h>
> > +#include <asm/switch_to.h>
> > #include "ocxl_internal.h"
> >
> >
> > @@ -123,11 +125,55 @@ static long afu_ioctl_get_metadata(struct
> ocxl_context *ctx,
> > return 0;
> > }
> >
> > +#ifdef CONFIG_PPC64
> > +static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx,
> > + struct ocxl_ioctl_p9_wait __user *uarg) {
> > + struct ocxl_ioctl_p9_wait arg;
> > +
> > + memset(&arg, 0, sizeof(arg));
> > +
> > + if (cpu_has_feature(CPU_FTR_P9_TIDR)) {
> > + enum ocxl_context_status status;
> > +
> > + // Locks both status & tidr
> > + mutex_lock(&ctx->status_mutex);
> > + if (!ctx->tidr) {
> > + if (set_thread_tidr(current))
> > + return -ENOENT;
> > +
> > + ctx->tidr = current->thread.tidr;
> > + }
> > +
> > + status = ctx->status;
> > + mutex_unlock(&ctx->status_mutex);
> > +
> > + if (status == ATTACHED) {
> > + int rc;
> > + struct link *link = ctx->afu->fn->link;
> > +
> > + rc = ocxl_link_update_pe(link, ctx->pasid, ctx->tidr);
> > + if (rc)
> > + return rc;
> > + }
> > +
> > + arg.thread_id = ctx->tidr;
> > + } else
> > + return -ENOENT;
> > +
> > + if (copy_to_user(uarg, &arg, sizeof(arg)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH" :
> \
> > x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" : \
> > x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" :
> \
> > x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" :
> \
> > x == OCXL_IOCTL_GET_METADATA ?
> "GET_METADATA" : \
> > + x == OCXL_IOCTL_ENABLE_P9_WAIT ?
> "ENABLE_P9_WAIT" : \
> > "UNKNOWN")
> >
> > static long afu_ioctl(struct file *file, unsigned int cmd, @@ -186,6
> > +232,13 @@ static long afu_ioctl(struct file *file, unsigned int cmd,
> > (struct ocxl_ioctl_metadata __user *) args);
> > break;
> >
> > +#ifdef CONFIG_PPC64
> > + case OCXL_IOCTL_ENABLE_P9_WAIT:
> > + rc = afu_ioctl_enable_p9_wait(ctx,
> > + (struct ocxl_ioctl_p9_wait __user *) args);
> > + break;
> > +#endif
> > +
> > default:
> > rc = -EINVAL;
> > }
> > diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c index
> > 656e8610eec2..88876ae8f330 100644
> > --- a/drivers/misc/ocxl/link.c
> > +++ b/drivers/misc/ocxl/link.c
> > @@ -544,6 +544,42 @@ int ocxl_link_add_pe(void *link_handle, int pasid,
> u32 pidr, u32 tidr,
> > }
> > EXPORT_SYMBOL_GPL(ocxl_link_add_pe);
> >
> > +int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid) {
> > + struct link *link = (struct link *) link_handle;
> > + struct spa *spa = link->spa;
> > + struct ocxl_process_element *pe;
> > + int pe_handle, rc;
> > +
> > + if (pasid > SPA_PASID_MAX)
> > + return -EINVAL;
> > +
> > + pe_handle = pasid & SPA_PE_MASK;
> > + pe = spa->spa_mem + pe_handle;
> > +
> > + mutex_lock(&spa->spa_lock);
> > +
> > + pe->tid = tid;
> > +
> > + /*
> > + * The barrier makes sure the PE is updated
> > + * before we clear the NPU context cache below, so that the
> > + * old PE cannot be reloaded erroneously.
> > + */
> > + mb();
> > +
> > + /*
> > + * hook to platform code
> > + * On powerpc, the entry needs to be cleared from the context
> > + * cache of the NPU.
> > + */
> > + rc = pnv_ocxl_spa_remove_pe_from_cache(link->platform_data,
> pe_handle);
> > + WARN_ON(rc);
> > +
> > + mutex_unlock(&spa->spa_lock);
> > + return rc;
> > +}
> > +
> > int ocxl_link_remove_pe(void *link_handle, int pasid)
> > {
> > struct link *link = (struct link *) link_handle; diff --git
> > a/drivers/misc/ocxl/ocxl_internal.h
> > b/drivers/misc/ocxl/ocxl_internal.h
> > index 5d421824afd9..a32f2151029f 100644
> > --- a/drivers/misc/ocxl/ocxl_internal.h
> > +++ b/drivers/misc/ocxl/ocxl_internal.h
> > @@ -77,6 +77,7 @@ struct ocxl_context {
> > struct ocxl_xsl_error xsl_error;
> > struct mutex irq_lock;
> > struct idr irq_idr;
> > + u16 tidr; // Thread ID used for P9 wait implementation
> > };
> >
> > struct ocxl_process_element {
> > diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h index
> > 51ccf76db293..9ff6ddc28e22 100644
> > --- a/include/misc/ocxl.h
> > +++ b/include/misc/ocxl.h
> > @@ -188,6 +188,15 @@ extern int ocxl_link_add_pe(void *link_handle, int
> pasid, u32 pidr, u32 tidr,
> > void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
> > void *xsl_err_data);
> >
> > +/**
> > + * Update values within a Process Element
> > + *
> > + * link_handle: the link handle associated with the process element
> > + * pasid: the PASID for the AFU context
> > + * tid: the new thread id for the process element */ extern int
> > +ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> > +
> > /*
> > * Remove a Process Element from the Shared Process Area for a link
> > */
> > diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h index
> > 0af83d80fb3e..561e6f0dfcb7 100644
> > --- a/include/uapi/misc/ocxl.h
> > +++ b/include/uapi/misc/ocxl.h
> > @@ -48,6 +48,13 @@ struct ocxl_ioctl_metadata {
> > __u64 reserved[13]; // Total of 16*u64
> > };
> >
> > +struct ocxl_ioctl_p9_wait {
> > + __u16 thread_id; // The thread ID required to wake this thread
> > + __u16 reserved1;
> > + __u32 reserved2;
> > + __u64 reserved3[3];
> > +};
> > +
> > struct ocxl_ioctl_irq_fd {
> > __u64 irq_offset;
> > __s32 eventfd;
> > @@ -62,5 +69,6 @@ struct ocxl_ioctl_irq_fd {
> > #define OCXL_IOCTL_IRQ_FREE _IOW(OCXL_MAGIC, 0x12, __u64)
> > #define OCXL_IOCTL_IRQ_SET_FD _IOW(OCXL_MAGIC, 0x13, struct
> ocxl_ioctl_irq_fd)
> > #define OCXL_IOCTL_GET_METADATA _IOR(OCXL_MAGIC, 0x14, struct
> > ocxl_ioctl_metadata)
> > +#define OCXL_IOCTL_ENABLE_P9_WAIT _IOR(OCXL_MAGIC, 0x15,
> struct ocxl_ioctl_p9_wait)
> >
> > #endif /* _UAPI_MISC_OCXL_H */
> >
>
>
> ---
> This email has been checked for viruses by AVG.
> http://www.avg.com

2018-05-11 11:04:24

[permalink] [raw]

Subject: Re: [PATCH v5 5/7] ocxl: Expose the thread_id needed for wait on POWER9

Le 11/05/2018 à 12:06, Alastair D'Silva a écrit :
>
>> -----Original Message-----
>> From: Frederic Barrat <[email protected]>
>> Sent: Friday, 11 May 2018 7:25 PM
>> To: Alastair D'Silva <[email protected]>; [email protected]
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; Alastair D'Silva <[email protected]>
>> Subject: Re: [PATCH v5 5/7] ocxl: Expose the thread_id needed for wait on
>> POWER9
>>
>>
>>
>> Le 11/05/2018 à 08:13, Alastair D'Silva a écrit :
>>> From: Alastair D'Silva <[email protected]>
>>>
>>> In order to successfully issue as_notify, an AFU needs to know the TID
>>> to notify, which in turn means that this information should be
>>> available in userspace so it can be communicated to the AFU.
>>>
>>> Signed-off-by: Alastair D'Silva <[email protected]>
>>> ---
>>
>> Ok, so we keep the limitation of having only one thread per context able to
>> call 'wait', even though we don't have to worry about depleting the pool of
>> TIDs any more. I think that's acceptable, though we don't really have a reason
>> to justify it any more. Any reason you want to keep it that way?
>>
>
> No strong reason, just trying to minimise the amount of changes. We can always expand the scope later, if we have a use-case for it.

ok. Agreed, it's not worth holding up this series, we can send a follow
up patch.

Fred

>> Fred
>>
>>
>>> drivers/misc/ocxl/context.c | 5 ++-
>>> drivers/misc/ocxl/file.c | 53 +++++++++++++++++++++++++++++++
>>> drivers/misc/ocxl/link.c | 36 +++++++++++++++++++++
>>> drivers/misc/ocxl/ocxl_internal.h | 1 +
>>> include/misc/ocxl.h | 9 ++++++
>>> include/uapi/misc/ocxl.h | 8 +++++
>>> 6 files changed, 111 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
>>> index 909e8807824a..95f74623113e 100644
>>> --- a/drivers/misc/ocxl/context.c
>>> +++ b/drivers/misc/ocxl/context.c
>>> @@ -34,6 +34,8 @@ int ocxl_context_init(struct ocxl_context *ctx, struct
>> ocxl_afu *afu,
>>> mutex_init(&ctx->xsl_error_lock);
>>> mutex_init(&ctx->irq_lock);
>>> idr_init(&ctx->irq_idr);
>>> + ctx->tidr = 0;
>>> +
>>> /*
>>> * Keep a reference on the AFU to make sure it's valid for the
>>> * duration of the life of the context @@ -65,6 +67,7 @@ int
>>> ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
>>> {
>>> int rc;
>>>
>>> + // Locks both status & tidr
>>> mutex_lock(&ctx->status_mutex);
>>> if (ctx->status != OPENED) {
>>> rc = -EIO;
>>> @@ -72,7 +75,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64
>> amr)
>>> }
>>>
>>> rc = ocxl_link_add_pe(ctx->afu->fn->link, ctx->pasid,
>>> - current->mm->context.id, 0, amr, current->mm,
>>> + current->mm->context.id, ctx->tidr, amr, current-
>>> mm,
>>> xsl_fault_error, ctx);
>>> if (rc)
>>> goto out;
>>> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c index
>>> 038509e5d031..eb409a469f21 100644
>>> --- a/drivers/misc/ocxl/file.c
>>> +++ b/drivers/misc/ocxl/file.c
>>> @@ -5,6 +5,8 @@
>>> #include <linux/sched/signal.h>
>>> #include <linux/uaccess.h>
>>> #include <uapi/misc/ocxl.h>
>>> +#include <asm/reg.h>
>>> +#include <asm/switch_to.h>
>>> #include "ocxl_internal.h"
>>>
>>>
>>> @@ -123,11 +125,55 @@ static long afu_ioctl_get_metadata(struct
>> ocxl_context *ctx,
>>> return 0;
>>> }
>>>
>>> +#ifdef CONFIG_PPC64
>>> +static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx,
>>> + struct ocxl_ioctl_p9_wait __user *uarg) {
>>> + struct ocxl_ioctl_p9_wait arg;
>>> +
>>> + memset(&arg, 0, sizeof(arg));
>>> +
>>> + if (cpu_has_feature(CPU_FTR_P9_TIDR)) {
>>> + enum ocxl_context_status status;
>>> +
>>> + // Locks both status & tidr
>>> + mutex_lock(&ctx->status_mutex);
>>> + if (!ctx->tidr) {
>>> + if (set_thread_tidr(current))
>>> + return -ENOENT;
>>> +
>>> + ctx->tidr = current->thread.tidr;
>>> + }
>>> +
>>> + status = ctx->status;
>>> + mutex_unlock(&ctx->status_mutex);
>>> +
>>> + if (status == ATTACHED) {
>>> + int rc;
>>> + struct link *link = ctx->afu->fn->link;
>>> +
>>> + rc = ocxl_link_update_pe(link, ctx->pasid, ctx->tidr);
>>> + if (rc)
>>> + return rc;
>>> + }
>>> +
>>> + arg.thread_id = ctx->tidr;
>>> + } else
>>> + return -ENOENT;
>>> +
>>> + if (copy_to_user(uarg, &arg, sizeof(arg)))
>>> + return -EFAULT;
>>> +
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH" :
>> \
>>> x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" : \
>>> x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" :
>> \
>>> x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" :
>> \
>>> x == OCXL_IOCTL_GET_METADATA ?
>> "GET_METADATA" : \
>>> + x == OCXL_IOCTL_ENABLE_P9_WAIT ?
>> "ENABLE_P9_WAIT" : \
>>> "UNKNOWN")
>>>
>>> static long afu_ioctl(struct file *file, unsigned int cmd, @@ -186,6
>>> +232,13 @@ static long afu_ioctl(struct file *file, unsigned int cmd,
>>> (struct ocxl_ioctl_metadata __user *) args);
>>> break;
>>>
>>> +#ifdef CONFIG_PPC64
>>> + case OCXL_IOCTL_ENABLE_P9_WAIT:
>>> + rc = afu_ioctl_enable_p9_wait(ctx,
>>> + (struct ocxl_ioctl_p9_wait __user *) args);
>>> + break;
>>> +#endif
>>> +
>>> default:
>>> rc = -EINVAL;
>>> }
>>> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c index
>>> 656e8610eec2..88876ae8f330 100644
>>> --- a/drivers/misc/ocxl/link.c
>>> +++ b/drivers/misc/ocxl/link.c
>>> @@ -544,6 +544,42 @@ int ocxl_link_add_pe(void *link_handle, int pasid,
>> u32 pidr, u32 tidr,
>>> }
>>> EXPORT_SYMBOL_GPL(ocxl_link_add_pe);
>>>
>>> +int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid) {
>>> + struct link *link = (struct link *) link_handle;
>>> + struct spa *spa = link->spa;
>>> + struct ocxl_process_element *pe;
>>> + int pe_handle, rc;
>>> +
>>> + if (pasid > SPA_PASID_MAX)
>>> + return -EINVAL;
>>> +
>>> + pe_handle = pasid & SPA_PE_MASK;
>>> + pe = spa->spa_mem + pe_handle;
>>> +
>>> + mutex_lock(&spa->spa_lock);
>>> +
>>> + pe->tid = tid;
>>> +
>>> + /*
>>> + * The barrier makes sure the PE is updated
>>> + * before we clear the NPU context cache below, so that the
>>> + * old PE cannot be reloaded erroneously.
>>> + */
>>> + mb();
>>> +
>>> + /*
>>> + * hook to platform code
>>> + * On powerpc, the entry needs to be cleared from the context
>>> + * cache of the NPU.
>>> + */
>>> + rc = pnv_ocxl_spa_remove_pe_from_cache(link->platform_data,
>> pe_handle);
>>> + WARN_ON(rc);
>>> +
>>> + mutex_unlock(&spa->spa_lock);
>>> + return rc;
>>> +}
>>> +
>>> int ocxl_link_remove_pe(void *link_handle, int pasid)
>>> {
>>> struct link *link = (struct link *) link_handle; diff --git
>>> a/drivers/misc/ocxl/ocxl_internal.h
>>> b/drivers/misc/ocxl/ocxl_internal.h
>>> index 5d421824afd9..a32f2151029f 100644
>>> --- a/drivers/misc/ocxl/ocxl_internal.h
>>> +++ b/drivers/misc/ocxl/ocxl_internal.h
>>> @@ -77,6 +77,7 @@ struct ocxl_context {
>>> struct ocxl_xsl_error xsl_error;
>>> struct mutex irq_lock;
>>> struct idr irq_idr;
>>> + u16 tidr; // Thread ID used for P9 wait implementation
>>> };
>>>
>>> struct ocxl_process_element {
>>> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h index
>>> 51ccf76db293..9ff6ddc28e22 100644
>>> --- a/include/misc/ocxl.h
>>> +++ b/include/misc/ocxl.h
>>> @@ -188,6 +188,15 @@ extern int ocxl_link_add_pe(void *link_handle, int
>> pasid, u32 pidr, u32 tidr,
>>> void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
>>> void *xsl_err_data);
>>>
>>> +/**
>>> + * Update values within a Process Element
>>> + *
>>> + * link_handle: the link handle associated with the process element
>>> + * pasid: the PASID for the AFU context
>>> + * tid: the new thread id for the process element */ extern int
>>> +ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
>>> +
>>> /*
>>> * Remove a Process Element from the Shared Process Area for a link
>>> */
>>> diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h index
>>> 0af83d80fb3e..561e6f0dfcb7 100644
>>> --- a/include/uapi/misc/ocxl.h
>>> +++ b/include/uapi/misc/ocxl.h
>>> @@ -48,6 +48,13 @@ struct ocxl_ioctl_metadata {
>>> __u64 reserved[13]; // Total of 16*u64
>>> };
>>>
>>> +struct ocxl_ioctl_p9_wait {
>>> + __u16 thread_id; // The thread ID required to wake this thread
>>> + __u16 reserved1;
>>> + __u32 reserved2;
>>> + __u64 reserved3[3];
>>> +};
>>> +
>>> struct ocxl_ioctl_irq_fd {
>>> __u64 irq_offset;
>>> __s32 eventfd;
>>> @@ -62,5 +69,6 @@ struct ocxl_ioctl_irq_fd {
>>> #define OCXL_IOCTL_IRQ_FREE _IOW(OCXL_MAGIC, 0x12, __u64)
>>> #define OCXL_IOCTL_IRQ_SET_FD _IOW(OCXL_MAGIC, 0x13, struct
>> ocxl_ioctl_irq_fd)
>>> #define OCXL_IOCTL_GET_METADATA _IOR(OCXL_MAGIC, 0x14, struct
>>> ocxl_ioctl_metadata)
>>> +#define OCXL_IOCTL_ENABLE_P9_WAIT _IOR(OCXL_MAGIC, 0x15,
>> struct ocxl_ioctl_p9_wait)
>>>
>>> #endif /* _UAPI_MISC_OCXL_H */
>>>
>>
>>
>> ---
>> This email has been checked for viruses by AVG.
>> http://www.avg.com
>
>

2018-05-31 04:21:41

[permalink] [raw]

Subject: Re: [PATCH v5 1/7] powerpc: Add TIDR CPU feature for POWER9

On 11/05/18 16:12, Alastair D'Silva wrote:
> From: Alastair D'Silva <[email protected]>
>
> This patch adds a CPU feature bit to show whether the CPU has
> the TIDR register available, enabling as_notify/wait in userspace.
>
> Signed-off-by: Alastair D'Silva <[email protected]>

Reviewed-by: Andrew Donnellan <[email protected]>

--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited

2018-05-31 04:22:56

[permalink] [raw]

Subject: Re: [PATCH v5 2/7] powerpc: Use TIDR CPU feature to control TIDR allocation

On 11/05/18 16:12, Alastair D'Silva wrote:
> From: Alastair D'Silva <[email protected]>
>
> Switch the use of TIDR on it's CPU feature, rather than assuming it
> is available based on architecture.
>
> Signed-off-by: Alastair D'Silva <[email protected]>

Reviewed-by: Andrew Donnellan <[email protected]>

--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited

2018-05-31 04:32:01

[permalink] [raw]

Subject: Re: [PATCH v5 3/7] powerpc: use task_pid_nr() for TID allocation

On 11/05/18 16:12, Alastair D'Silva wrote:
> From: Alastair D'Silva <[email protected]>
>
> The current implementation of TID allocation, using a global IDR, may
> result in an errant process starving the system of available TIDs.
> Instead, use task_pid_nr(), as mentioned by the original author. The
> scenario described which prevented it's use is not applicable, as
> set_thread_tidr can only be called after the task struct has been
> populated.
>
> In the unlikely event that 2 threads share the TID and are waiting,
> all potential outcomes have been determined safe.
>
> Signed-off-by: Alastair D'Silva <[email protected]>

Thanks for the clarifying comment. The diff is painful to read but I
think it makes sense :)

Reviewed-by: Andrew Donnellan <[email protected]>

--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited

2018-05-31 04:34:39

[permalink] [raw]

Subject: Re: [PATCH v5 4/7] ocxl: Rename pnv_ocxl_spa_remove_pe to clarify it's action

On 11/05/18 16:13, Alastair D'Silva wrote:
> From: Alastair D'Silva <[email protected]>
>
> The function removes the process element from NPU cache.
>
> Signed-off-by: Alastair D'Silva <[email protected]>

Acked-by: Andrew Donnellan <[email protected]>

--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited

2018-05-31 04:43:53

[permalink] [raw]

Subject: Re: [PATCH v5 5/7] ocxl: Expose the thread_id needed for wait on POWER9

On 11/05/18 16:13, Alastair D'Silva wrote:
> From: Alastair D'Silva <[email protected]>
>
> In order to successfully issue as_notify, an AFU needs to know the TID
> to notify, which in turn means that this information should be
> available in userspace so it can be communicated to the AFU.
>
> Signed-off-by: Alastair D'Silva <[email protected]>

Acked-by: Andrew Donnellan <[email protected]>

Comments below.

> +#ifdef CONFIG_PPC64
> +static long afu_ioctl_enable_p9_wait(struct ocxl_context *ctx,
> + struct ocxl_ioctl_p9_wait __user *uarg)
> +{
> + struct ocxl_ioctl_p9_wait arg;
> +
> + memset(&arg, 0, sizeof(arg));
> +
> + if (cpu_has_feature(CPU_FTR_P9_TIDR)) {
> + enum ocxl_context_status status;
> +
> + // Locks both status & tidr
> + mutex_lock(&ctx->status_mutex);
> + if (!ctx->tidr) {
> + if (set_thread_tidr(current))
> + return -ENOENT;
> +
> + ctx->tidr = current->thread.tidr;
> + }
> +
> + status = ctx->status;
> + mutex_unlock(&ctx->status_mutex);
> +
> + if (status == ATTACHED) {
> + int rc;
> + struct link *link = ctx->afu->fn->link;
> +
> + rc = ocxl_link_update_pe(link, ctx->pasid, ctx->tidr);
> + if (rc)
> + return rc;
> + }
> +
> + arg.thread_id = ctx->tidr;
> + } else
> + return -ENOENT;

I didn't pick this up before - please please please use braces on both
sides of the if here.

> +
> + if (copy_to_user(uarg, &arg, sizeof(arg)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +#endif

> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 51ccf76db293..9ff6ddc28e22 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -188,6 +188,15 @@ extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
> void *xsl_err_data);
>
> +/**
> + * Update values within a Process Element
> + *
> + * link_handle: the link handle associated with the process element
> + * pasid: the PASID for the AFU context
> + * tid: the new thread id for the process element
> + */
> +extern int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);

My earlier comment about __u16 vs u16 applies for this declaration (and
the body declaration) as well

--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited

2018-05-31 04:47:17

[permalink] [raw]

Subject: Re: [PATCH v5 6/7] ocxl: Add an IOCTL so userspace knows what OCXL features are available

On 11/05/18 16:13, Alastair D'Silva wrote:
> From: Alastair D'Silva <[email protected]>
>
> In order for a userspace AFU driver to call the POWER9 specific
> OCXL_IOCTL_ENABLE_P9_WAIT, it needs to verify that it can actually
> make that call.
>
> Signed-off-by: Alastair D'Silva <[email protected]>

Acked-by: Andrew Donnellan <[email protected]>

--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited

2018-05-31 04:49:31