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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-12-04 at 13:33 +0100, Andre Przywara wrote:
> 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 firmware uses wfe while the Xen gate uses wfi so infact both
are needed.

> > 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.

Great, I should have checked the code before replying.

Thanks,
Ian.

> 
> 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);
> >>>>
> >>>
> >>>
> >>
> >
> >
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel