Re: [PATCH v2 2/6] arm: move GIC SGI kicking into separate function

[Andre Przywara <andre.przywara@linaro.org>]

On 12/04/2013 01:28 PM, Ian Campbell wrote:
> On Wed, 2013-12-04 at 13:15 +0100, Andre Przywara wrote:
>> On 12/02/2013 04:01 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
>>>> Currently we unconditionally send SGIs to all cores on SMP bringup.
>>>> PSCI will not need this, so we move this into a function and call it
>>>> explicitly from the platforms that need it. This gets us get rid of
>>>> the empty cpu_up() platform functions in ARM32 and the comment in
>>>> there.
>>>
>>> I don't think this is quite true -- even on a PSCI system the kick is
>>> required to get past the gate in head.S.
>>
>> Right, but this is the responsibility of the PSCI handler in the
>> firmware, right?
>
> Note that I am talking about a gate which is implemented in Xen's
> head.S, not in the firmware. It is Xen's reponsibility to get the CPU
> past that point, which is somewhat independent from the firmware wakeup,
> except in reality it is intertwined because they use the same mechanism.
>
> However, I think I was mistaken. In the case of PSCI we are able to wake
> up specific targetted CPUs individually and we do so having already
> opened the gate in out head.S, so the CPU must necessarily fall through
> without waiting. I think this is worth mentioning in the commit log if
> you don't mind.
>
>>   I was under the assumption that the semantics of cpu_on
>> is to start executing code at the given address, whatever this takes
>> internally.
>
> Right, the issue here is a gate which we have subsequent to that
> happening.
>
>> Calxeda firmware for instance does the SGI kick.
>>
>>>
>>> I wonder how this interacts with PSCI implementations which use an SGI
>>> themselves internally...
>>>
>>>> @@ -376,11 +386,6 @@ int __cpu_up(unsigned int cpu)
>>>>            return rc;
>>>>        }
>>>>
>>>> -    /* We don't know the GIC ID of the CPU until it has woken up, so just signal
>>>> -     * everyone and rely on our own smp_up_cpu gate to ensure only the one we
>>>> -     * want gets through. */
>>>> -    send_SGI_allbutself(GIC_SGI_EVENT_CHECK);
>>>> -
>>>
>>> So, I was saying in the 00 mail I'm not sure we can get rid of this
>>> altogether.
>>
>> Please note that we do not get rid of this, but just move it. ARM64
>> calls it in arm64/smpboot.c,
>
> How does this interact with the sev() In smp_spin_table_cpu_up I wonder.

Me, too ;-)
The original code does the sev() in arm/arm64/smpboot.c, then returns to 
the caller in arm/smpboot.c, which does the GIC kick. I was already 
wondering if that is correct, I guess you could answer this much better.

> I think the GIC send should be only for the non-PSCI case here too.

It is. The arm64 code has the GIC kick moved into the spin_table 
function only. PSCI is a different beast, not using the GIC. Also arm32 
does it only in platform specific functions, which don't get called when 
PSCI is available.

Regards,
Andre.

>
>>   ARM32 non-PSCI platforms call this now
>> explicitly by pointing to that function in their platforms/foo.c file.
>
> OK.
>
>>
>>>
>>> But I suppose it is the intention that the platform code always has both
>>> its own logic and this SGI kick (possibly coalesced) in such
>>> circumstances? Which is probably ok?
>>
>> That was my thinking, yes.
>>
>> Regards,
>> Andre.
>>
>>
>>>>        while ( !cpu_online(cpu) )
>>>>        {
>>>>            cpu_relax();
>>>> diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
>>>> index 1485cc6..a1de03c 100644
>>>> --- a/xen/include/asm-arm/smp.h
>>>> +++ b/xen/include/asm-arm/smp.h
>>>> @@ -21,6 +21,8 @@ extern int arch_smp_init(void);
>>>>    extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
>>>>    extern int arch_cpu_up(int cpu);
>>>>
>>>> +int cpu_up_send_sgi(int cpu);
>>>> +
>>>>    /* Secondary CPU entry point */
>>>>    extern void init_secondary(void);
>>>>
>>>
>>>
>>
>
>