| Message ID | 1511435132-24020-1-git-send-email-clombard@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support for ASB_Notify on POWER9 | expand |
Hi Christophe, Few review comments: Christophe Lombard <clombard@linux.vnet.ibm.com> writes: > + > +int cxl_context_thread_tidr(struct cxl_context *ctx, int assign) > +{ > + int rc = 0; > + > + /* Clear any TIDR value assigned to the current thread */ > + if (!assign) { > + clear_thread_tidr(current); > + ctx->tid = 0; > + } else { > + /* Assign a unique TIDR (thread id) for the current thread */ > + rc = set_thread_tidr(current); > + if (!rc) > + ctx->tid = current->thread.tidr; set_thread_tidr can also return non-zero error values and will never return '0'. So this condition should be 'if(rc > 0)' instead of 'if (!rc)' > +#define CXL_IOCTL_GET_AFU_ID _IOR(CXL_MAGIC, 0x02, struct cxl_afu_id) > +#define CXL_IOCTL_THREAD_TIDR _IOR(CXL_MAGIC, 0x03, > int) Instead of adding a new syscall I think assiging a thread-id can be better done by adding a new flag to the cxl_ioctl_start_work.flag field and using one of the reserved fields to return the allocated tid back to the user.
Le 23/11/2017 à 15:16, Vaibhav Jain a écrit : > Hi Christophe, > > Few review comments: > > Christophe Lombard <clombard@linux.vnet.ibm.com> writes: >> + >> +int cxl_context_thread_tidr(struct cxl_context *ctx, int assign) >> +{ >> + int rc = 0; >> + >> + /* Clear any TIDR value assigned to the current thread */ >> + if (!assign) { >> + clear_thread_tidr(current); >> + ctx->tid = 0; >> + } else { >> + /* Assign a unique TIDR (thread id) for the current thread */ >> + rc = set_thread_tidr(current); >> + if (!rc) >> + ctx->tid = current->thread.tidr; > set_thread_tidr can also return non-zero error values and will never > return '0'. So this condition should be 'if(rc > 0)' instead of 'if (!rc)' good point. Thanks for this. > >> +#define CXL_IOCTL_GET_AFU_ID _IOR(CXL_MAGIC, 0x02, struct cxl_afu_id) >> +#define CXL_IOCTL_THREAD_TIDR _IOR(CXL_MAGIC, 0x03, >> int) > Instead of adding a new syscall I think assiging a thread-id can be > better done by adding a new flag to the cxl_ioctl_start_work.flag > field and using one of the reserved fields to return the allocated tid > back to the user. > We have worked previously on this solution but this solution is too restrictive for now - we offer the possibility to configure and to clear the tidr from user land - and for the future implementation.
On Thu, 2017-11-23 at 12:05 +0100, Christophe Lombard wrote: > 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. Is that keyd off some device-tree property or similar ? There is no guarantee that the TIDR and ASB_Notify still exist on future chips... Ben.
Le 23/11/2017 à 21:41, Benjamin Herrenschmidt a écrit : > On Thu, 2017-11-23 at 12:05 +0100, Christophe Lombard wrote: >> 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. > > Is that keyd off some device-tree property or similar ? There is no > guarantee that the TIDR and ASB_Notify still exist on future chips... > > Ben. > To my knowledge, there is no property (or similar), somewhere, that indicating that the TIDR is supported or not. For the time being, if I am not wrong, the only check we have, is this condition in the function set_thread_tidr(struct task_struct *t): if (!cpu_has_feature(CPU_FTR_ARCH_300)) return -EINVAL; Christophe
On Fri, 2017-11-24 at 11:14 +0100, christophe lombard wrote: > To my knowledge, there is no property (or similar), somewhere, that > indicating that the TIDR is supported or not. > For the time being, if I am not wrong, the only check we have, is > this condition in the function set_thread_tidr(struct task_struct *t): > > if (!cpu_has_feature(CPU_FTR_ARCH_300)) > return -EINVAL; > > > Christophe Then we need to fix that Ben.
Le 24/11/2017 à 14:02, Benjamin Herrenschmidt a écrit : > On Fri, 2017-11-24 at 11:14 +0100, christophe lombard wrote: >> To my knowledge, there is no property (or similar), somewhere, that >> indicating that the TIDR is supported or not. >> For the time being, if I am not wrong, the only check we have, is >> this condition in the function set_thread_tidr(struct task_struct *t): >> >> if (!cpu_has_feature(CPU_FTR_ARCH_300)) >> return -EINVAL; >> >> >> Christophe > > Then we need to fix that > > Ben. > You are right. We will insert a checking in the cxl driver to allow updating the TIDR if a P9 is present. This will be in the patch V2. Thanks Christophe
On Fri, 2017-11-24 at 17:37 +0100, christophe lombard wrote: > You are right. We will insert a checking in the cxl driver to allow > updating the TIDR if a P9 is present. This will be in the patch V2. > Thanks Best is to actually: 1) Add something to the device-tree in skiboot (and work with pHyp & KVM to do the same if not already in ibm,pa-features) 2) Key off htat. Nick, anything in your new feature stuff too ? Ben.
christophe lombard <clombard@linux.vnet.ibm.com> writes: > Le 24/11/2017 à 14:02, Benjamin Herrenschmidt a écrit : >> On Fri, 2017-11-24 at 11:14 +0100, christophe lombard wrote: >>> To my knowledge, there is no property (or similar), somewhere, that >>> indicating that the TIDR is supported or not. >>> For the time being, if I am not wrong, the only check we have, is >>> this condition in the function set_thread_tidr(struct task_struct *t): >>> >>> if (!cpu_has_feature(CPU_FTR_ARCH_300)) >>> return -EINVAL; >>> >>> >>> Christophe >> >> Then we need to fix that >> >> Ben. >> > > You are right. We will insert a checking in the cxl driver to allow > updating the TIDR if a P9 is present. This will be in the patch V2. > Thanks A cxl_is_power9() check should be fine. When the check fails you should return an error code that can be distinguished and interpreted correctly by userspace, ie. not EINVAL. That implies if the program calls with a different set of arguments the call might succeed, which is not true. Either ENODEV or ENXIO would be best I think. cheers
Le 27/11/2017 à 05:03, Michael Ellerman a écrit : > christophe lombard <clombard@linux.vnet.ibm.com> writes: > >> Le 24/11/2017 à 14:02, Benjamin Herrenschmidt a écrit : >>> On Fri, 2017-11-24 at 11:14 +0100, christophe lombard wrote: >>>> To my knowledge, there is no property (or similar), somewhere, that >>>> indicating that the TIDR is supported or not. >>>> For the time being, if I am not wrong, the only check we have, is >>>> this condition in the function set_thread_tidr(struct task_struct *t): >>>> >>>> if (!cpu_has_feature(CPU_FTR_ARCH_300)) >>>> return -EINVAL; >>>> >>>> >>>> Christophe >>> >>> Then we need to fix that >>> >>> Ben. >>> >> >> You are right. We will insert a checking in the cxl driver to allow >> updating the TIDR if a P9 is present. This will be in the patch V2. >> Thanks > > A cxl_is_power9() check should be fine. > > When the check fails you should return an error code that can be > distinguished and interpreted correctly by userspace, ie. not EINVAL. > That implies if the program calls with a different set of arguments the > call might succeed, which is not true. > > Either ENODEV or ENXIO would be best I think. > > cheers > This is what I had in mind about the function cxl_is_power9() and I am agree with the return codes. Thanks for your help. Christophe
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index bfdd783..370fec9 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -1557,6 +1557,7 @@ void clear_thread_tidr(struct task_struct *t) free_thread_tidr(t->thread.tidr); t->thread.tidr = 0; } +EXPORT_SYMBOL_GPL(clear_thread_tidr); void arch_release_task_struct(struct task_struct *t) { @@ -1583,6 +1584,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/api.c b/drivers/misc/cxl/api.c index 7c11bad..121bfbc 100644 --- a/drivers/misc/cxl/api.c +++ b/drivers/misc/cxl/api.c @@ -684,3 +684,12 @@ void _cxl_cx4_teardown_msi_irqs(struct pci_dev *pdev) } } /* Exported via cxl_base */ + +int cxl_thread_tidr(struct cxl_context *ctx, int assign) +{ + if (!ctx) + return -EINVAL; + + return cxl_context_thread_tidr(ctx, assign); +} +EXPORT_SYMBOL_GPL(cxl_thread_tidr); diff --git a/drivers/misc/cxl/context.c b/drivers/misc/cxl/context.c index 12a41b2..6fd61a8 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" @@ -42,6 +43,7 @@ int cxl_context_init(struct cxl_context *ctx, struct cxl_afu *afu, bool master) ctx->afu = afu; ctx->master = master; + ctx->tid = 0; ctx->pid = NULL; /* Set in start work ioctl */ mutex_init(&ctx->mapping_lock); ctx->mapping = NULL; @@ -362,3 +364,22 @@ 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 assign) +{ + int rc = 0; + + /* Clear any TIDR value assigned to the current thread */ + if (!assign) { + clear_thread_tidr(current); + ctx->tid = 0; + } else { + /* Assign a unique TIDR (thread id) for the current thread */ + rc = set_thread_tidr(current); + if (!rc) + ctx->tid = current->thread.tidr; + } + pr_devel("%s: current tidr: %d\n", __func__, ctx->tid); + + return rc; +} diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h index e46a406..5f8678f 100644 --- a/drivers/misc/cxl/cxl.h +++ b/drivers/misc/cxl/cxl.h @@ -561,6 +561,7 @@ struct cxl_context { unsigned int sst_size, sst_lru; wait_queue_head_t wq; + u32 tid; /* use mm context associated with this pid for ds faults */ struct pid *pid; spinlock_t lock; /* Protects pending_irq_mask, pending_fault and fault_addr */ @@ -1169,4 +1170,9 @@ 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); +/* Clear any TIDR value or assign a unique TIDR (thread id) for the + * current thread + */ +int cxl_context_thread_tidr(struct cxl_context *ctx, int assign); + #endif diff --git a/drivers/misc/cxl/cxllib.c b/drivers/misc/cxl/cxllib.c index dc9bc18..126d988 100644 --- a/drivers/misc/cxl/cxllib.c +++ b/drivers/misc/cxl/cxllib.c @@ -202,7 +202,7 @@ int cxllib_get_PE_attributes(struct task_struct *task, } else { attr->pid = 0; } - attr->tid = 0; + attr->tid = current->thread.tidr; 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..f0666ee 100644 --- a/drivers/misc/cxl/file.c +++ b/drivers/misc/cxl/file.c @@ -300,6 +300,17 @@ static long afu_ioctl_get_afu_id(struct cxl_context *ctx, return 0; } +static long afu_ioctl_thread_tidr(struct cxl_context *ctx, + int __user *uassign) +{ + int assign; + + if (copy_from_user(&assign, uassign, sizeof(assign))) + return -EFAULT; + + return cxl_context_thread_tidr(ctx, assign); +} + long afu_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct cxl_context *ctx = file->private_data; @@ -319,6 +330,8 @@ long afu_ioctl(struct file *file, unsigned int cmd, unsigned long arg) case CXL_IOCTL_GET_AFU_ID: return afu_ioctl_get_afu_id(ctx, (struct cxl_afu_id __user *) arg); + case CXL_IOCTL_THREAD_TIDR: + return afu_ioctl_thread_tidr(ctx, (int __user *)arg); } return -EINVAL; } diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c index 02b6b45..657c4a9 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(ctx->tid); 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..78b5eb5 100644 --- a/include/uapi/misc/cxl.h +++ b/include/uapi/misc/cxl.h @@ -81,7 +81,8 @@ struct cxl_adapter_image { /* AFU devices */ #define CXL_IOCTL_START_WORK _IOW(CXL_MAGIC, 0x00, struct cxl_ioctl_start_work) #define CXL_IOCTL_GET_PROCESS_ELEMENT _IOR(CXL_MAGIC, 0x01, __u32) -#define CXL_IOCTL_GET_AFU_ID _IOR(CXL_MAGIC, 0x02, struct cxl_afu_id) +#define CXL_IOCTL_GET_AFU_ID _IOR(CXL_MAGIC, 0x02, struct cxl_afu_id) +#define CXL_IOCTL_THREAD_TIDR _IOR(CXL_MAGIC, 0x03, int) /* adapter devices */ #define CXL_IOCTL_DOWNLOAD_IMAGE _IOW(CXL_MAGIC, 0x0A, struct cxl_adapter_image) #define CXL_IOCTL_VALIDATE_IMAGE _IOW(CXL_MAGIC, 0x0B, struct cxl_adapter_image)
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. Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> --- arch/powerpc/kernel/process.c | 2 ++ drivers/misc/cxl/api.c | 9 +++++++++ drivers/misc/cxl/context.c | 21 +++++++++++++++++++++ drivers/misc/cxl/cxl.h | 6 ++++++ drivers/misc/cxl/cxllib.c | 2 +- drivers/misc/cxl/file.c | 13 +++++++++++++ drivers/misc/cxl/native.c | 2 +- include/uapi/misc/cxl.h | 3 ++- 8 files changed, 55 insertions(+), 3 deletions(-)