[V4] cxl: Add support for ASB_Notify on POWER9

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

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

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

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

Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
Reviewed-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>

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

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

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

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.