diff mbox

[Xen-devel] xen/arm: Fix smp_send_call_function_mask() for current CPU

Message ID 1408016846-12149-1-git-send-email-anup.patel@linaro.org
State New
Headers show

Commit Message

Anup Patel Aug. 14, 2014, 11:47 a.m. UTC 
The smp_send_call_function_mask() does not work on Foundation v8
model with one CPU. The reason being gicv2_send_SGI() is called
with irqmode==SGI_TARGET_LIST and *cpu_mask=0x1 on CPU0 which
does not work on Foundation v8 model.

Further, it is really strange that smp_send_call_function_mask()
depends on GIC SGIs for calling function on current CPU.

This patch fixes smp_send_call_function_mask() for current CPU
by directly calling smp_call_function_interrupt() on current CPU.
This is very similar to what Xen x86 does.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 xen/arch/arm/smp.c |    7 +++++++
 1 file changed, 7 insertions(+)

Comments

Anup Patel Aug. 14, 2014, 11:49 a.m. UTC | #1 
(Forgot to add patches[at] apm [dot] com)

--
Anup

On 14 August 2014 17:17, Anup Patel <anup.patel@linaro.org> wrote:
> The smp_send_call_function_mask() does not work on Foundation v8
> model with one CPU. The reason being gicv2_send_SGI() is called
> with irqmode==SGI_TARGET_LIST and *cpu_mask=0x1 on CPU0 which
> does not work on Foundation v8 model.
>
> Further, it is really strange that smp_send_call_function_mask()
> depends on GIC SGIs for calling function on current CPU.
>
> This patch fixes smp_send_call_function_mask() for current CPU
> by directly calling smp_call_function_interrupt() on current CPU.
> This is very similar to what Xen x86 does.
>
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>  xen/arch/arm/smp.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> index 30203b8..4fde3f9 100644
> --- a/xen/arch/arm/smp.c
> +++ b/xen/arch/arm/smp.c
> @@ -20,6 +20,13 @@ void smp_send_event_check_mask(const cpumask_t *mask)
>  void smp_send_call_function_mask(const cpumask_t *mask)
>  {
>      send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
> +
> +    if ( cpumask_test_cpu(smp_processor_id(), mask) )
> +    {
> +        local_irq_disable();
> +        smp_call_function_interrupt();
> +        local_irq_enable();
> +    }
>  }
>
>  /*
> --
> 1.7.9.5
>
Julien Grall Aug. 14, 2014, 12:44 p.m. UTC | #2 
Hi Anup,

On 08/14/2014 12:47 PM, Anup Patel wrote:
> The smp_send_call_function_mask() does not work on Foundation v8
> model with one CPU. The reason being gicv2_send_SGI() is called
> with irqmode==SGI_TARGET_LIST and *cpu_mask=0x1 on CPU0 which
> does not work on Foundation v8 model.

Please provide any steps, trace that make you think that irqmode ==
SGI_TARGET_LIST and *cpu_mask=0x1 is not working on Foundation V8 Model.

> Further, it is really strange that smp_send_call_function_mask()
> depends on GIC SGIs for calling function on current CPU.

Why it's strange??? The GIC specification doesn't seem to add any
restriction about sending an SGI to the current CPU.

It clearly looks like a bug in another part of Xen. And I doubt it's
because the Foundation Model is not able to support the use case above.

Without any further explanation than "It doesn't work" and "It's
strange", I don't think this patch should be accepted in Xen.
You need at least to point the paragraph in the spec...

> This patch fixes smp_send_call_function_mask() for current CPU
> by directly calling smp_call_function_interrupt() on current CPU.
> This is very similar to what Xen x86 does.

What was done in x86 may not make sense on ARM....

For me the current code is valid... So far, I didn't see any issue on
different boards. I've also used recently the Foundation v8 Model [1],
without any issue.

> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>  xen/arch/arm/smp.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> index 30203b8..4fde3f9 100644
> --- a/xen/arch/arm/smp.c
> +++ b/xen/arch/arm/smp.c
> @@ -20,6 +20,13 @@ void smp_send_event_check_mask(const cpumask_t *mask)
>  void smp_send_call_function_mask(const cpumask_t *mask)
>  {
>      send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
> +
> +    if ( cpumask_test_cpu(smp_processor_id(), mask) )
> +    {
> +        local_irq_disable();
> +        smp_call_function_interrupt();
> +        local_irq_enable();
> +    }
>  }

Send SGI mask doesn't remove the current CPU, therefore this will result
to send twice the SGIs to the current physical CPU.

While we have a check in smp_call_function_interrupt to avoid spurious
SGI interrupt. It's not acceptable to rely on it knowingly.

In general, if we can avoid test for a specific CPU in the code it's better.

Regards,

[1]
http://www.arm.com/products/tools/models/fast-models/foundation-model.php
Julien Grall Aug. 14, 2014, 9:52 p.m. UTC | #3 
On 14/08/14 13:44, Julien Grall wrote:
> On 08/14/2014 12:47 PM, Anup Patel wrote:
>> The smp_send_call_function_mask() does not work on Foundation v8
>> model with one CPU. The reason being gicv2_send_SGI() is called
>> with irqmode==SGI_TARGET_LIST and *cpu_mask=0x1 on CPU0 which
>> does not work on Foundation v8 model.
>
> Please provide any steps, trace that make you think that irqmode ==
> SGI_TARGET_LIST and *cpu_mask=0x1 is not working on Foundation V8 Model.
>
>> Further, it is really strange that smp_send_call_function_mask()
>> depends on GIC SGIs for calling function on current CPU.
>
> Why it's strange??? The GIC specification doesn't seem to add any
> restriction about sending an SGI to the current CPU.
>
> It clearly looks like a bug in another part of Xen. And I doubt it's
> because the Foundation Model is not able to support the use case above.
>
> Without any further explanation than "It doesn't work" and "It's
> strange", I don't think this patch should be accepted in Xen.
> You need at least to point the paragraph in the spec...
>
>> This patch fixes smp_send_call_function_mask() for current CPU
>> by directly calling smp_call_function_interrupt() on current CPU.
>> This is very similar to what Xen x86 does.
>
> What was done in x86 may not make sense on ARM....
>
> For me the current code is valid... So far, I didn't see any issue on
> different boards. I've also used recently the Foundation v8 Model [1],
> without any issue.

I just saw their is new version of the model. I've tested sucessfully 
xen on:

ARM V8 Foundation Model r0p0 (model build 0.8.5206)

I will give a try on newer version.

Regards,
Anup Patel Aug. 15, 2014, 3:58 a.m. UTC | #4 
Hi Julien,

On 14 August 2014 18:14, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Anup,
>
> On 08/14/2014 12:47 PM, Anup Patel wrote:
>> The smp_send_call_function_mask() does not work on Foundation v8
>> model with one CPU. The reason being gicv2_send_SGI() is called
>> with irqmode==SGI_TARGET_LIST and *cpu_mask=0x1 on CPU0 which
>> does not work on Foundation v8 model.
>
> Please provide any steps, trace that make you think that irqmode ==
> SGI_TARGET_LIST and *cpu_mask=0x1 is not working on Foundation V8 Model.
>
>> Further, it is really strange that smp_send_call_function_mask()
>> depends on GIC SGIs for calling function on current CPU.
>
> Why it's strange??? The GIC specification doesn't seem to add any
> restriction about sending an SGI to the current CPU.
>
> It clearly looks like a bug in another part of Xen. And I doubt it's
> because the Foundation Model is not able to support the use case above.
>
> Without any further explanation than "It doesn't work" and "It's
> strange", I don't think this patch should be accepted in Xen.
> You need at least to point the paragraph in the spec...

Its strange because for calling function on current CPU the
implementation will:
Trigger SGI for current CPU => Xen takes interrupt on Current CPU
=> IPI interrupt handler will call smp_call_function_interrupt()

The above can be simplified for current CPU by straight away
calling smp_call_function_interrupt() for current CPU.

If the current implementation does not sound strange and
sub-optimal to you then I have no further comments.

>
>> This patch fixes smp_send_call_function_mask() for current CPU
>> by directly calling smp_call_function_interrupt() on current CPU.
>> This is very similar to what Xen x86 does.
>
> What was done in x86 may not make sense on ARM....

Oh, really. Please see the explanation above.

In fact, Linux and other hypervisors directly call SMP function
for current CPU. I don't see how Xen ARM is different.

I would suggest you provide proper explanation before
making statements like: "What was done in x86 may not
make sense on ARM....."

>
> For me the current code is valid... So far, I didn't see any issue on
> different boards. I've also used recently the Foundation v8 Model [1],
> without any issue.

What is valid need not be optimal so I don't buy this argument.

I agree that Xen ARM64 not working for me on Foundation v8
model could potentially be bug in Foundation v8 model but
this is no reason to reject this patch because this patch is
just a small improvement which also happen to fix my problem.

If you still don't agree with above then feel happy to reject this patch.

Regards,
Anup

>
>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>> ---
>>  xen/arch/arm/smp.c |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
>> index 30203b8..4fde3f9 100644
>> --- a/xen/arch/arm/smp.c
>> +++ b/xen/arch/arm/smp.c
>> @@ -20,6 +20,13 @@ void smp_send_event_check_mask(const cpumask_t *mask)
>>  void smp_send_call_function_mask(const cpumask_t *mask)
>>  {
>>      send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
>> +
>> +    if ( cpumask_test_cpu(smp_processor_id(), mask) )
>> +    {
>> +        local_irq_disable();
>> +        smp_call_function_interrupt();
>> +        local_irq_enable();
>> +    }
>>  }
>
> Send SGI mask doesn't remove the current CPU, therefore this will result
> to send twice the SGIs to the current physical CPU.
>
> While we have a check in smp_call_function_interrupt to avoid spurious
> SGI interrupt. It's not acceptable to rely on it knowingly.
>
> In general, if we can avoid test for a specific CPU in the code it's better.
>
> Regards,
>
> [1]
> http://www.arm.com/products/tools/models/fast-models/foundation-model.php
>
> --
> Julien Grall
Stefano Stabellini Aug. 15, 2014, 9:28 a.m. UTC | #5 
Hi Anup,
thanks for the patch.


On Thu, 14 Aug 2014, Anup Patel wrote:
> The smp_send_call_function_mask() does not work on Foundation v8
> model with one CPU. The reason being gicv2_send_SGI() is called
> with irqmode==SGI_TARGET_LIST and *cpu_mask=0x1 on CPU0 which
> does not work on Foundation v8 model.

I read the GIC spec again, and this has to be a bug in the Foundation v8
model. Please make sure to raise this issue with ARM so that they can
fix it.


> Further, it is really strange that smp_send_call_function_mask()
> depends on GIC SGIs for calling function on current CPU.
> 
> This patch fixes smp_send_call_function_mask() for current CPU
> by directly calling smp_call_function_interrupt() on current CPU.
> This is very similar to what Xen x86 does.

It could be a reasonable improvement regardless of the Foundation v8
bug. I would recommend to:

- Avoid mentioning the bug in the Foundation model in the commit
message, because a bug in an emulator is not a good reason to make
changes in Xen. If that was the only reason, then we would need to
introduce the change as a per-platform quirk, like
PLATFORM_QUIRK_GIC_64K_STRIDE.

- Clear the smp_processor_id() bit from mask, otherwise we end up
calling the function twice on the processor we are running on at the
moment.



> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>  xen/arch/arm/smp.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> index 30203b8..4fde3f9 100644
> --- a/xen/arch/arm/smp.c
> +++ b/xen/arch/arm/smp.c
> @@ -20,6 +20,13 @@ void smp_send_event_check_mask(const cpumask_t *mask)
>  void smp_send_call_function_mask(const cpumask_t *mask)
>  {
>      send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
> +
> +    if ( cpumask_test_cpu(smp_processor_id(), mask) )
> +    {
> +        local_irq_disable();
> +        smp_call_function_interrupt();
> +        local_irq_enable();
> +    }
>  }
>  
>  /*
> -- 
> 1.7.9.5
>
Anup Patel Aug. 15, 2014, 10:12 a.m. UTC | #6 
Hi Stefano,

On 15 August 2014 14:58, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> Hi Anup,
> thanks for the patch.
>
>
> On Thu, 14 Aug 2014, Anup Patel wrote:
>> The smp_send_call_function_mask() does not work on Foundation v8
>> model with one CPU. The reason being gicv2_send_SGI() is called
>> with irqmode==SGI_TARGET_LIST and *cpu_mask=0x1 on CPU0 which
>> does not work on Foundation v8 model.
>
> I read the GIC spec again, and this has to be a bug in the Foundation v8
> model. Please make sure to raise this issue with ARM so that they can
> fix it.

I had the same feeling when I cross-checked GIC spec after
reading Julien's comment.

>
>
>> Further, it is really strange that smp_send_call_function_mask()
>> depends on GIC SGIs for calling function on current CPU.
>>
>> This patch fixes smp_send_call_function_mask() for current CPU
>> by directly calling smp_call_function_interrupt() on current CPU.
>> This is very similar to what Xen x86 does.
>
> It could be a reasonable improvement regardless of the Foundation v8
> bug. I would recommend to:
>
> - Avoid mentioning the bug in the Foundation model in the commit
> message, because a bug in an emulator is not a good reason to make
> changes in Xen. If that was the only reason, then we would need to
> introduce the change as a per-platform quirk, like
> PLATFORM_QUIRK_GIC_64K_STRIDE.

Sure, I will update the wordings in patch description as per
your suggestion.

>
> - Clear the smp_processor_id() bit from mask, otherwise we end up
> calling the function twice on the processor we are running on at the
> moment.

Sure, I will update this.

Thanks,
Anup

>
>
>
>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>> ---
>>  xen/arch/arm/smp.c |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
>> index 30203b8..4fde3f9 100644
>> --- a/xen/arch/arm/smp.c
>> +++ b/xen/arch/arm/smp.c
>> @@ -20,6 +20,13 @@ void smp_send_event_check_mask(const cpumask_t *mask)
>>  void smp_send_call_function_mask(const cpumask_t *mask)
>>  {
>>      send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
>> +
>> +    if ( cpumask_test_cpu(smp_processor_id(), mask) )
>> +    {
>> +        local_irq_disable();
>> +        smp_call_function_interrupt();
>> +        local_irq_enable();
>> +    }
>>  }
>>
>>  /*
>> --
>> 1.7.9.5
>>
diff mbox

Patch

diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
index 30203b8..4fde3f9 100644
--- a/xen/arch/arm/smp.c
+++ b/xen/arch/arm/smp.c
@@ -20,6 +20,13 @@  void smp_send_event_check_mask(const cpumask_t *mask)
 void smp_send_call_function_mask(const cpumask_t *mask)
 {
     send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
+
+    if ( cpumask_test_cpu(smp_processor_id(), mask) )
+    {
+        local_irq_disable();
+        smp_call_function_interrupt();
+        local_irq_enable();
+    }
 }
 
 /*