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

[christophe lombard <clombard@linux.vnet.ibm.com>]

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
>