[PATCH] cxl: Set the valid bit in PE for dedicated mode

[PATCH] cxl: Set the valid bit in PE for dedicated mode

[Vaibhav Jain]

Make sure to set the valid-bit in software-state field of the
populated PE. This was earlier missing for dedicated mode AFUs, hence
was causing a PSL freeze when the AFU was activated.

Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
 drivers/misc/cxl/native.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 4a82c313cf71..0e748c682ee9 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c
@@ -897,6 +897,10 @@ int cxl_attach_dedicated_process_psl9(struct cxl_context *ctx, u64 wed, u64 amr)
 	if (ctx->afu->adapter->native->sl_ops->update_dedicated_ivtes)
 		afu->adapter->native->sl_ops->update_dedicated_ivtes(ctx);
 
+	ctx->elem->software_state = cpu_to_be32(CXL_PE_SOFTWARE_STATE_V);
+	/* Make sure the changes to the PE are visible to the card */
+	smp_wmb();
+
 	result = cxl_ops->afu_reset(afu);
 	if (result)
 		return result;
-- 
2.13.5

Re: [PATCH] cxl: Set the valid bit in PE for dedicated mode

[Andrew Donnellan]

On 28/08/17 14:15, Vaibhav Jain wrote:
> Make sure to set the valid-bit in software-state field of the
> populated PE. This was earlier missing for dedicated mode AFUs, hence
> was causing a PSL freeze when the AFU was activated.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>

Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

> ---
>   drivers/misc/cxl/native.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
> index 4a82c313cf71..0e748c682ee9 100644
> --- a/drivers/misc/cxl/native.c
> +++ b/drivers/misc/cxl/native.c
> @@ -897,6 +897,10 @@ int cxl_attach_dedicated_process_psl9(struct cxl_context *ctx, u64 wed, u64 amr)
>   	if (ctx->afu->adapter->native->sl_ops->update_dedicated_ivtes)
>   		afu->adapter->native->sl_ops->update_dedicated_ivtes(ctx);
>   
> +	ctx->elem->software_state = cpu_to_be32(CXL_PE_SOFTWARE_STATE_V);
> +	/* Make sure the changes to the PE are visible to the card */
> +	smp_wmb();
> +
>   	result = cxl_ops->afu_reset(afu);
>   	if (result)
>   		return result;
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH] cxl: Set the valid bit in PE for dedicated mode

[christophe lombard]

Le 28/08/2017 à 06:15, Vaibhav Jain a écrit :
> Make sure to set the valid-bit in software-state field of the
> populated PE. This was earlier missing for dedicated mode AFUs, hence
> was causing a PSL freeze when the AFU was activated.

Acked-by:  Christophe Lombard <clombard@linux.vnet.ibm.com>

> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> ---
>   drivers/misc/cxl/native.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
> index 4a82c313cf71..0e748c682ee9 100644
> --- a/drivers/misc/cxl/native.c
> +++ b/drivers/misc/cxl/native.c
> @@ -897,6 +897,10 @@ int cxl_attach_dedicated_process_psl9(struct cxl_context *ctx, u64 wed, u64 amr)
>   	if (ctx->afu->adapter->native->sl_ops->update_dedicated_ivtes)
>   		afu->adapter->native->sl_ops->update_dedicated_ivtes(ctx);
>
> +	ctx->elem->software_state = cpu_to_be32(CXL_PE_SOFTWARE_STATE_V);
> +	/* Make sure the changes to the PE are visible to the card */
> +	smp_wmb();
> +
>   	result = cxl_ops->afu_reset(afu);
>   	if (result)
>   		return result;

Re: [PATCH] cxl: Set the valid bit in PE for dedicated mode

[Frederic Barrat]

Le 28/08/2017 à 06:15, Vaibhav Jain a écrit :
> Make sure to set the valid-bit in software-state field of the
> populated PE. This was earlier missing for dedicated mode AFUs, hence
> was causing a PSL freeze when the AFU was activated.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> ---

Acked-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com>

Thanks!



>   drivers/misc/cxl/native.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
> index 4a82c313cf71..0e748c682ee9 100644
> --- a/drivers/misc/cxl/native.c
> +++ b/drivers/misc/cxl/native.c
> @@ -897,6 +897,10 @@ int cxl_attach_dedicated_process_psl9(struct cxl_context *ctx, u64 wed, u64 amr)
>   	if (ctx->afu->adapter->native->sl_ops->update_dedicated_ivtes)
>   		afu->adapter->native->sl_ops->update_dedicated_ivtes(ctx);
> 
> +	ctx->elem->software_state = cpu_to_be32(CXL_PE_SOFTWARE_STATE_V);
> +	/* Make sure the changes to the PE are visible to the card */
> +	smp_wmb();
> +
>   	result = cxl_ops->afu_reset(afu);
>   	if (result)
>   		return result;
> 

Re: [PATCH] cxl: Set the valid bit in PE for dedicated mode

[Michael Ellerman]

Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:

> Make sure to set the valid-bit in software-state field of the
> populated PE. This was earlier missing for dedicated mode AFUs, hence
> was causing a PSL freeze when the AFU was activated.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> ---
>  drivers/misc/cxl/native.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
> index 4a82c313cf71..0e748c682ee9 100644
> --- a/drivers/misc/cxl/native.c
> +++ b/drivers/misc/cxl/native.c
> @@ -897,6 +897,10 @@ int cxl_attach_dedicated_process_psl9(struct cxl_context *ctx, u64 wed, u64 amr)
>  	if (ctx->afu->adapter->native->sl_ops->update_dedicated_ivtes)
>  		afu->adapter->native->sl_ops->update_dedicated_ivtes(ctx);
>  
> +	ctx->elem->software_state = cpu_to_be32(CXL_PE_SOFTWARE_STATE_V);
> +	/* Make sure the changes to the PE are visible to the card */

A barrier orders something vs something else. So what's the something
else in this case? Is it the afu_reset() below, what does that actually do?

> +	smp_wmb();
> +
>  	result = cxl_ops->afu_reset(afu);
>  	if (result)
>  		return result;

cheers

Re: [PATCH] cxl: Set the valid bit in PE for dedicated mode

[Vaibhav Jain]

Hi Mpe,

Thanks for reviewing the patch

Michael Ellerman <mpe@ellerman.id.au> writes:

>> +	ctx->elem->software_state = cpu_to_be32(CXL_PE_SOFTWARE_STATE_V);
>> +	/* Make sure the changes to the PE are visible to the card */
>
> A barrier orders something vs something else. So what's the something
> else in this case? Is it the afu_reset() below, what does that actually do?
>

The issue is with call to afu_enable() after the call to afu_reset that
would start the AFU. If this load gets reordered and PSL doesnt see the
valid bit set for this structure then it will result in PSL entering a
freeze-state.

Though on second thoughts afu_enable() is grabbing a spin-lock before
doing an mmio to start the AFU that would be forcing a barrier
anyways. But since that spans the function boundary hence to be safe
have added a write barrier after populating the process element.

Lastly function is not performance critical as it will be usually called
in the life time of a process only once. So the impact smp_wmb() is
having would be minimal.

-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH] cxl: Set the valid bit in PE for dedicated mode

[Michael Ellerman]

Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:

> Hi Mpe,
>
> Thanks for reviewing the patch
>
> Michael Ellerman <mpe@ellerman.id.au> writes:
>
>>> +	ctx->elem->software_state = cpu_to_be32(CXL_PE_SOFTWARE_STATE_V);
>>> +	/* Make sure the changes to the PE are visible to the card */
>>
>> A barrier orders something vs something else. So what's the something
>> else in this case? Is it the afu_reset() below, what does that actually do?
>>
>
> The issue is with call to afu_enable() after the call to afu_reset that
> would start the AFU. If this load gets reordered and PSL doesnt see the
> valid bit set for this structure then it will result in PSL entering a
> freeze-state.

OK, so it's ordering the store above to ctx->elem->software_state vs the
store to the AFU in afu_enable().

> Though on second thoughts afu_enable() is grabbing a spin-lock before
> doing an mmio to start the AFU that would be forcing a barrier
> anyways. But since that spans the function boundary hence to be safe
> have added a write barrier after populating the process element.

The spin lock doesn't help you, stores are allowed to leak into the
locked region.

But the MMIO is preceeded by a sync.

> Lastly function is not performance critical as it will be usually called
> in the life time of a process only once. So the impact smp_wmb() is
> having would be minimal.

Sure. Performance is not the issue, barriers are subtle so it's
important that they're well documented.


So I don't think you need the barrier, because the out_be64() will do it
for you. But if you really want to add one, I don't mind.

But, you should use wmb(), not smp_wmb(), because the ordering is still
required on non-SMP systems. And please update the comment to capture
all of the above discussion.

cheers