[PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
cpus to stand-by when booting. Thus, using SEV is enough for the boot
cpu to kick other secondaries. Further more, the current implementation
of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
then lead a data fault on GICv3 send_SGI implementation.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/arch/arm/arm64/smpboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 341cc77..62e6abb 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -38,7 +38,7 @@ static int __init smp_spin_table_cpu_up(int cpu)
 
     sev();
 
-    return cpu_up_send_sgi(cpu);
+    return 0;
 }
 
 static void __init smp_spin_table_init(int cpu, struct dt_device_node *dn)
-- 
2.1.4

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Julien Grall]

Hi Chen,

Subject: I think you can drop the "_" in spin_table.

On 07/04/15 08:33, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
> 
> On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
> cpus to stand-by when booting. Thus, using SEV is enough for the boot
> cpu to kick other secondaries. Further more, the current implementation
> of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
> then lead a data fault on GICv3 send_SGI implementation.

I'm not familiar with spin table on ARM64, so I will let Ian answer
about it.

Aside that, the GICv3 implementation looks buggy to me.
The GIC code provides two helpers which lead to pass NULL to the
callback send_SGI:
	- send_SGI_self: AFAICT nobody is using it
	- send_SGI_allbutself: Only used by the smp boot code


I think the former can be dropped or modify to send_SGI_one.

For the later, I can't find why we need to send an SGI on ARM too. Ian,
Stefano, any idea?

Regards.

-- 
Julien Grall

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Ian Campbell]

On Wed, 2015-04-08 at 13:23 +0100, Julien Grall wrote:
> Hi Chen,
> 
> Subject: I think you can drop the "_" in spin_table.
> 
> On 07/04/15 08:33, Chen Baozi wrote:
> > From: Chen Baozi <baozich@gmail.com>
> > 
> > On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
> > cpus to stand-by when booting. Thus, using SEV is enough for the boot
> > cpu to kick other secondaries. Further more, the current implementation
> > of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
> > then lead a data fault on GICv3 send_SGI implementation.
> 
> I'm not familiar with spin table on ARM64, so I will let Ian answer
> about it.

For arm32 it's sadly all a bit adhoc and not terribly well documented.
(If I'm wrong I'd love a pointer to the doc). But for arm64 it does seem
to be documented (linux/Documentation/arm64/booting.txt)

So it seems possible that the SGI dates to older arm based systems.

> Aside that, the GICv3 implementation looks buggy to me.
> The GIC code provides two helpers which lead to pass NULL to the
> callback send_SGI:
> 	- send_SGI_self: AFAICT nobody is using it

I think we ended up with this by analogy to x86's send_IPI_self,
probably expecting we would need it there too.

> 	- send_SGI_allbutself: Only used by the smp boot code
> 
> 
> I think the former can be dropped or modify to send_SGI_one.

Yes, either would be ok.

> For the later, I can't find why we need to send an SGI on ARM too. Ian,
> Stefano, any idea?

IIRC there were some platform firmware (u-boot? boot-wrapper? vexpress
first stage firmware?) which needed it (because they used wfi not wfe?).

Or perhaps we cargo-culted from somewhere, or maybe it's just always
been wrong.

ISTR that there is some subtle difference with the Event register
between armv7 and armv8, which might explain this. I don't quite recall
what though.

So, I dunno, it seems like there is a good chance we could remove this,
but that might break some random platform which we have forgotten about.

The GIC NULL issue thing should probably fixed either way, but we could
also try dropping the send SGI from both arm32 and 64 and see what
happens...

Ian.

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Julien Grall]

Hi Ian,

On 15/04/15 15:57, Ian Campbell wrote:
> On Wed, 2015-04-08 at 13:23 +0100, Julien Grall wrote:
>> Hi Chen,
>>
>> Subject: I think you can drop the "_" in spin_table.
>>
>> On 07/04/15 08:33, Chen Baozi wrote:
>>> From: Chen Baozi <baozich@gmail.com>
>>>
>>> On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
>>> cpus to stand-by when booting. Thus, using SEV is enough for the boot
>>> cpu to kick other secondaries. Further more, the current implementation
>>> of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
>>> then lead a data fault on GICv3 send_SGI implementation.
>>
>> I'm not familiar with spin table on ARM64, so I will let Ian answer
>> about it.
> 
> For arm32 it's sadly all a bit adhoc and not terribly well documented.
> (If I'm wrong I'd love a pointer to the doc). But for arm64 it does seem
> to be documented (linux/Documentation/arm64/booting.txt)

Thanks for the pointer.

[..]

> So, I dunno, it seems like there is a good chance we could remove this,
> but that might break some random platform which we have forgotten about.

Hmmm... I looked the wrong code in Linux :/.
The SGI is required for most of the platforms.

> The GIC NULL issue thing should probably fixed either way, but we could
> also try dropping the send SGI from both arm32 and 64 and see what
> happens...

I will see to rework send_SGI_self and send_SGI_allbutself.

Although I may not be able to send the patch before a couple of weeks.

Nonetheless, based on the doc this patch looks valid to me and can go
without the reworking because GICv3 is only supported on ARM64 which
won't require to send an SGI:

Reviewed-by: Julien Grall <julien.grall@citrix.com>

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Ian Campbell]

On Wed, 2015-04-15 at 16:56 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 15/04/15 15:57, Ian Campbell wrote:
> > On Wed, 2015-04-08 at 13:23 +0100, Julien Grall wrote:
> >> Hi Chen,
> >>
> >> Subject: I think you can drop the "_" in spin_table.
> >>
> >> On 07/04/15 08:33, Chen Baozi wrote:
> >>> From: Chen Baozi <baozich@gmail.com>
> >>>
> >>> On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
> >>> cpus to stand-by when booting. Thus, using SEV is enough for the boot
> >>> cpu to kick other secondaries. Further more, the current implementation
> >>> of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
> >>> then lead a data fault on GICv3 send_SGI implementation.
> >>
> >> I'm not familiar with spin table on ARM64, so I will let Ian answer
> >> about it.
> > 
> > For arm32 it's sadly all a bit adhoc and not terribly well documented.
> > (If I'm wrong I'd love a pointer to the doc). But for arm64 it does seem
> > to be documented (linux/Documentation/arm64/booting.txt)
> 
> Thanks for the pointer.
> 
> [..]
> 
> > So, I dunno, it seems like there is a good chance we could remove this,
> > but that might break some random platform which we have forgotten about.
> 
> Hmmm... I looked the wrong code in Linux :/.
> The SGI is required for most of the platforms.

Could you give a pointer to that code please.

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Julien Grall]

On 15/04/2015 17:21, Ian Campbell wrote:
> On Wed, 2015-04-15 at 16:56 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 15/04/15 15:57, Ian Campbell wrote:
>>> On Wed, 2015-04-08 at 13:23 +0100, Julien Grall wrote:
>>>> Hi Chen,
>>>>
>>>> Subject: I think you can drop the "_" in spin_table.
>>>>
>>>> On 07/04/15 08:33, Chen Baozi wrote:
>>>>> From: Chen Baozi <baozich@gmail.com>
>>>>>
>>>>> On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
>>>>> cpus to stand-by when booting. Thus, using SEV is enough for the boot
>>>>> cpu to kick other secondaries. Further more, the current implementation
>>>>> of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
>>>>> then lead a data fault on GICv3 send_SGI implementation.
>>>>
>>>> I'm not familiar with spin table on ARM64, so I will let Ian answer
>>>> about it.
>>>
>>> For arm32 it's sadly all a bit adhoc and not terribly well documented.
>>> (If I'm wrong I'd love a pointer to the doc). But for arm64 it does seem
>>> to be documented (linux/Documentation/arm64/booting.txt)
>>
>> Thanks for the pointer.
>>
>> [..]
>>
>>> So, I dunno, it seems like there is a good chance we could remove this,
>>> but that might break some random platform which we have forgotten about.
>>
>> Hmmm... I looked the wrong code in Linux :/.
>> The SGI is required for most of the platforms.
>
> Could you give a pointer to that code please.
>
>

-- 
Julien Grall

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Julien Grall]

Hi Ian,

On 15/04/2015 17:21, Ian Campbell wrote:

>>
>> Hmmm... I looked the wrong code in Linux :/.
>> The SGI is required for most of the platforms.
>
> Could you give a pointer to that code please.

The call to arch_send_wakeup_ipi_mask is done per-platform.
For the platforms supported by Xen:

arndale: mach-exynos/platsmp.c
omap5: mach-omap2/omap-smp.c

I wasn't able to find if RCAR (CC Iurii for it) and Vexpress really 
require an SGI to bring up secondary CPUs.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Julien Grall]

Hi Chen,

On 07/04/15 08:33, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
> 
> On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
> cpus to stand-by when booting. Thus, using SEV is enough for the boot
> cpu to kick other secondaries. Further more, the current implementation
> of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
> then lead a data fault on GICv3 send_SGI implementation.

I've sent a patch to fix send_SGI on GICv3 [1]. You should not see the
segfault anymore.

Although, you were saying that the SGI is not necessary for the spin
table. Right?

If so, it would be nice to avoid it at least to respect the behavior
with spin table.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Ian Campbell]

On Tue, 2015-04-07 at 15:33 +0800, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
> 
> On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
> cpus to stand-by when booting. Thus, using SEV is enough for the boot
> cpu to kick other secondaries. Further more, the current implementation
> of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
> then lead a data fault on GICv3 send_SGI implementation.
> 
> Signed-off-by: Chen Baozi <baozich@gmail.com>

I think we concluded this was correct for arm64, so acked + applied.

Re: [PATCH] xen/arm64: Avoid sending SGI when kicking secondary cpus with spin_table

[Chen Baozi]

Hi Julien,

On Fri, May 08, 2015 at 05:38:47PM +0100, Julien Grall wrote:
> Hi Chen,
> 
> On 07/04/15 08:33, Chen Baozi wrote:
> > From: Chen Baozi <baozich@gmail.com>
> > 
> > On arm64, either firmware or xen's smp_up_cpu gate uses WFE on secondary
> > cpus to stand-by when booting. Thus, using SEV is enough for the boot
> > cpu to kick other secondaries. Further more, the current implementation
> > of cpu_up_send_sgi would pass a NULL cpumask pointer to send_SGI, which
> > then lead a data fault on GICv3 send_SGI implementation.
> 
> I've sent a patch to fix send_SGI on GICv3 [1]. You should not see the
> segfault anymore.

Yes, I have. And sorry for not being able to test&reply it in time. I was
busy for other stuffs last few weeks. I'll try the patch as soon as possible.

> 
> Although, you were saying that the SGI is not necessary for the spin
> table. Right?

Yes. I have checked the firmware, linux and xen. They all use WFE on secondary
CPUs to stand-by when booting.

Cheers,

Baozi.