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

[christophe lombard]

Le 27/11/2017 à 05:03, Michael Ellerman a écrit :

christophe lombard  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

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

[Michael Ellerman]

christophe lombard  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

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

[Benjamin Herrenschmidt]

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.

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

[christophe lombard]

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

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

[Benjamin Herrenschmidt]

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.

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

[christophe lombard]

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

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

[Benjamin Herrenschmidt]

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.

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

[christophe lombard]

Le 23/11/2017 à 15:16, Vaibhav Jain a écrit :

Hi Christophe,

Few review comments:

Christophe Lombard  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.

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

[Vaibhav Jain]

Hi Christophe,

Few review comments:

Christophe Lombard  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.

-- 
Vaibhav Jain 
Linux Technology Center, IBM India Pvt. Ltd.