[PATCH] cxl: Ensure PSL interrupt is configured for contexts with no AFU IRQs

[PATCH] cxl: Ensure PSL interrupt is configured for contexts with no AFU IRQs

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

In the cxl kernel API, it is possible to create a context and start it
without allocating any interrupts. Since we assign or allocate the PSL
interrupt when allocating AFU interrupts this will lead to a situation
where we start the context with no means to take any faults.

The user API is not affected as it always goes through the cxl interrupt
allocation code paths and will have the PSL interrupt allocated or
assigned, even if no AFU interrupts were requested.

This checks that at least one interrupt is configured at the time of
attach, and if not it will assign the multiplexed PSL interrupt for
powernv, or allocate a single interrupt for PowerVM.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 drivers/misc/cxl/guest.c  | 12 ++++++++++++
 drivers/misc/cxl/native.c |  9 +++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
index c0cdf3c..04f6dff 100644
--- a/drivers/misc/cxl/guest.c
+++ b/drivers/misc/cxl/guest.c
@@ -552,6 +552,17 @@ static int attach_afu_directed(struct cxl_context *ctx, u64 wed, u64 amr)
 
 	elem->common.sstp0  = cpu_to_be64(ctx->sstp0);
 	elem->common.sstp1  = cpu_to_be64(ctx->sstp1);
+
+	/*
+	 * Ensure we have at least one interrupt allocated to take faults for
+	 * kernel contexts that may not have allocated any AFU IRQs at all:
+	 */
+	if (ctx->irqs.range[0] == 0) {
+		rc = afu_register_irqs(ctx, 0);
+		if (rc)
+			goto out_free;
+	}
+
 	for (r = 0; r < CXL_IRQ_RANGES; r++) {
 		for (i = 0; i < ctx->irqs.range[r]; i++) {
 			if (r == 0 && i == 0) {
@@ -597,6 +608,7 @@ static int attach_afu_directed(struct cxl_context *ctx, u64 wed, u64 amr)
 		enable_afu_irqs(ctx);
 	}
 
+out_free:
 	free_page((u64)elem);
 	return rc;
 }
diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index f933129..5b51183 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c
@@ -527,6 +527,15 @@ static int attach_afu_directed(struct cxl_context *ctx, bool real_mode, u64 wed,
 	ctx->elem->common.sstp0 = cpu_to_be64(ctx->sstp0);
 	ctx->elem->common.sstp1 = cpu_to_be64(ctx->sstp1);
 
+	/*
+	 * Ensure we have the multiplexed PSL interrupt set up to take faults
+	 * for kernel contexts that may not have allocated any AFU IRQs at all:
+	 */
+	if (ctx->irqs.range[0] == 0) {
+		ctx->irqs.offset[0] = ctx->afu->native->psl_hwirq;
+		ctx->irqs.range[0] = 1;
+	}
+
 	for (r = 0; r < CXL_IRQ_RANGES; r++) {
 		ctx->elem->ivte_offsets[r] = cpu_to_be16(ctx->irqs.offset[r]);
 		ctx->elem->ivte_ranges[r] = cpu_to_be16(ctx->irqs.range[r]);
-- 
2.1.4

Re: [PATCH] cxl: Ensure PSL interrupt is configured for contexts with no AFU IRQs

[Frederic Barrat]

Hi Ian,


> diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> index c0cdf3c..04f6dff 100644
> --- a/drivers/misc/cxl/guest.c
> +++ b/drivers/misc/cxl/guest.c
> @@ -552,6 +552,17 @@ static int attach_afu_directed(struct cxl_context *ctx, u64 wed, u64 amr)
>
>   	elem->common.sstp0  = cpu_to_be64(ctx->sstp0);
>   	elem->common.sstp1  = cpu_to_be64(ctx->sstp1);
> +
> +	/*
> +	 * Ensure we have at least one interrupt allocated to take faults for
> +	 * kernel contexts that may not have allocated any AFU IRQs at all:
> +	 */
> +	if (ctx->irqs.range[0] == 0) {
> +		rc = afu_register_irqs(ctx, 0);
> +		if (rc)
> +			goto out_free;
> +	}
> +

I believe there's a potential problem there for powerVM guest.
In afu_allocate_irqs(), the allocation of the bitmap for the AFU 
interrupts should return NULL (since count = 0). Therefore we'll skip 
the allocation for the irq names. Yet we need one for the PSL interrupt. 
I'm not too sure of what the effect of calling cxl_map_irq with a NULL 
name is (later, in afu_register_hwirqs), but it's likely not very good.
That is admittedly a latent pb introduced by the powerVM patch, only 
revealed here.
Of course, this is highly theoretical. pHyp only supports cxlflash, so 
count is not supposed to be null. Still...

Other than that, the patch looks good to me.

   Fred

Re: [PATCH] cxl: Ensure PSL interrupt is configured for contexts with no AFU IRQs

[Ian Munsie]

Excerpts from Frederic Barrat's message of 2016-05-05 00:26:09 +1000:
> I believe there's a potential problem there for powerVM guest.
> In afu_allocate_irqs(), the allocation of the bitmap for the AFU 
> interrupts should return NULL (since count = 0). Therefore we'll skip 
> the allocation for the irq names. Yet we need one for the PSL interrupt. 
> I'm not too sure of what the effect of calling cxl_map_irq with a NULL 
> name is (later, in afu_register_hwirqs), but it's likely not very good.
> That is admittedly a latent pb introduced by the powerVM patch, only 
> revealed here.

Actually I believe it should be fine - kcalloc should return
ZERO_SIZE_POINTER, not NULL and the names should still be allocated.
I'm pretty sure we already rely on this for AFUs that don't use any
interrupts per context, otherwise they would fail with -ENOMEM.

> Of course, this is highly theoretical. pHyp only supports cxlflash, so 
> count is not supposed to be null. Still...

Yeah, I was somewhat tempted not to bother coping with this situation in
PowerVM given we don't have any users that need it, but it seemed like a
better idea to make the API's behaviour consistent regardless of the
underlying platform.

> Other than that, the patch looks good to me.

Thanks for the review :)

Cheers,
-Ian

Re: [PATCH] cxl: Ensure PSL interrupt is configured for contexts with no AFU IRQs

[Frederic Barrat]

Le 05/05/2016 02:14, Ian Munsie a écrit :
> Excerpts from Frederic Barrat's message of 2016-05-05 00:26:09 +1000:
>> I believe there's a potential problem there for powerVM guest.
>> In afu_allocate_irqs(), the allocation of the bitmap for the AFU
>> interrupts should return NULL (since count = 0). Therefore we'll skip
>> the allocation for the irq names. Yet we need one for the PSL interrupt.
>> I'm not too sure of what the effect of calling cxl_map_irq with a NULL
>> name is (later, in afu_register_hwirqs), but it's likely not very good.
>> That is admittedly a latent pb introduced by the powerVM patch, only
>> revealed here.
>
> Actually I believe it should be fine - kcalloc should return
> ZERO_SIZE_POINTER, not NULL and the names should still be allocated.
> I'm pretty sure we already rely on this for AFUs that don't use any
> interrupts per context, otherwise they would fail with -ENOMEM.

You're right, all is fine.

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

   Fred

Re: cxl: Ensure PSL interrupt is configured for contexts with no AFU IRQs

[Michael Ellerman]

On Wed, 2016-04-05 at 04:52:58 UTC, Ian Munsie wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
> 
> In the cxl kernel API, it is possible to create a context and start it
> without allocating any interrupts. Since we assign or allocate the PSL
> interrupt when allocating AFU interrupts this will lead to a situation
> where we start the context with no means to take any faults.
> 
> The user API is not affected as it always goes through the cxl interrupt
> allocation code paths and will have the PSL interrupt allocated or
> assigned, even if no AFU interrupts were requested.
> 
> This checks that at least one interrupt is configured at the time of
> attach, and if not it will assign the multiplexed PSL interrupt for
> powernv, or allocate a single interrupt for PowerVM.
> 
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> Reviewed-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/b75d94509921cb6d9f475d7a85

cheers