Message ID | 1513704438-28958-1-git-send-email-clombard@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | [V4] cxl: Add support for ASB_Notify on POWER9 | expand |
Hi Christophe, Thanks for the changes to the patch. Few minor review comments: Christophe Lombard <clombard@linux.vnet.ibm.com> writes: > @@ -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. > --- 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. > + } > + > trace_cxl_attach(ctx, work.work_element_descriptor, > work.num_interrupts, amr); should update the tracing here to also report the tidr > 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.
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 <clombard@linux.vnet.ibm.com> writes: > >> @@ -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" > >> --- 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. >> + } >> + >> 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. >> 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 <clombard@linux.vnet.ibm.com> writes: > Le 20/12/2017 à 07:31, Vaibhav Jain a écrit : >> 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. >>> + /* 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. >>> + } >>> + >>> 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
Le 20/12/2017 à 09:46, Vaibhav Jain a écrit : > Hi Chritophe, > > christophe lombard <clombard@linux.vnet.ibm.com> writes: > >> Le 20/12/2017 à 07:31, Vaibhav Jain a écrit : >>> 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. > >>>> + /* 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] >>>> + } >>>> + >>>> 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 <clombard@linux.vnet.ibm.com> writes: > Le 20/12/2017 à 09:46, Vaibhav Jain a écrit : >>> 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] 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.
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 */