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

[christophe lombard]

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 <clomb...@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.