Re: [PATCH v2 0/6]

From: Andre Przywara <andre.przywara@linaro.org>
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: xen-devel@lists.xenproject.org, julien.grall@linaro.org,
	patches@linaro.org, stefano.stabellini@eu.citrix.com
Subject: Re: [PATCH v2 0/6]
Date: Wed, 04 Dec 2013 13:16:04 +0100	[thread overview]
Message-ID: <529F1D04.8070008@linaro.org> (raw)
In-Reply-To: <1385995964.7108.98.camel@kazak.uk.xensource.com>

On 12/02/2013 03:52 PM, Ian Campbell wrote:
> On Mon, 2013-12-02 at 12:08 +0100, Andre Przywara wrote:
>> This is a major rework of last week's ARM PSCI host support. The code
>> has been moved into a separate file and the code flow has been
>> changed substantially (see below for more details).
>>    ---------
>>
>> Xen did not make use of the host provided ARM PSCI (Power State
>> Coordination Interface [1]) functionality so far, but relied on
>> platform specific SMP bringup functions.
>> This series adds support for PSCI on the host by reading the required
>> information from the DTB and invoking the appropriate handler when
>> bringing up each single CPU.
>> Since PSCI is defined for both ARM32 and ARM64, I added code for both
>> architectures, though only ARM32 is tested on Midway and VExpress
>> (without any PSCI support on the latter, just a regression test).
>> ARM64 code was compile tested only.
>>
>> The ARM32 SMP boot flow is now as following:
>> The DTB is scanned for a node announcing PSCI support. If that is
>> available, call the PSCI handler via SMC to bringup the secondary
>> cores. If no PSCI has been found, call the platform specific cpu_up()
>> function.
>
> Is this the most useful way round? If both PSCI and a hook are present
> might it be expected that the hook might want to make the choice to
> either go manual or call back to PSCI?
>
> Perhaps to handle some quirk (aka bug) in the platform which needs
> something else before/after the PSCI stuff.

I thought about that too and had it actually that way in the first place.
My reason to change it was: What happens if platforms get PSCI support?
Thinking about sunxi or Arndale. If the device tree advertises PSCI, 
then this is supposed to work. If it doesn't work, the DTB shouldn't 
carry the PSCI node or u-boot & friends have to remove it.

In general I agree on the "fix" idea, but I'd ask for that any bug in 
PSCI kicking should be fixed in the PSCI firmware and not in Xen or 
other kernels. If we are really in need for SMP fixes, we could still 
put them into smp_init().

I can easily change it to prefer platform code, of course.

>
>>   It is now the responsibility of those functions to do the
>> GIC SGI call to kick the secondary CPUs. For that a function is now
>> provided which does this, so three platforms now reference this
>> generic function instead of coding up an empty one and relying on the
>> GIC kick in Xen code later.
>
> Note that the existing kick serves two purposes. The first is to wake up
> the processor from the firmware on platforms which need that. The second
> is to wake up secondaries from the loop in head.S under:
>          /* Non-boot CPUs wait here until __cpu_up is ready for them */
> where they wait for smp_up_cpu to become them.
>
> I should go read the patches to see what you've actually done here.

But currently there is only one kick per CPU, right? I didn't change 
anything in this respect, just moved the code into a function and called 
that a bit earlier.

Regards,
Andre.

>
>> The ARM64 SMP boot flow is different: PSCI is not only described in a
>> root DTB node, but also advertised in each core's node as the
>> preferred SMP bringup method. So the PSCI call wrapper function is
>> registered as the cpu_up function when the DTB is scanned.
>>
>> This patch series is split up as follows:
>> 1/6) rename psci.c to vpsci.c to make room for the new, host-related
>>       code
>> 2/6) move the GIC SGI kick into a separate function and call it now
>>       directly from the platform code, removing the empty cpu_up()
>> 3/6) parse the DTB for a PSCI node and store the needed function index
>> 4/6) add a function to actually invoke the firmware's PSCI handler
>> 5/6) enable PSCI on ARM32 by adding the call to the PSCI handler
>> 6/6) enable PSCI on ARM64 by registering the PSCI handler function
>>
>>
>> Changes from v1:
>> - much rework, PSCI host DTB scanning unchanged (v1: 1/4, v2: 3/6)
>> - moved host PSCI code to psci.c, renaming the old one to vpsci.c
>> - have a separate psci_available variable
>> - removing explicit "host" from function names
>> - returning standard error codes on DTB scanning
>> - do the ARM64 PSCI call from a registered function pointer
>> - move the GIC kick into a separate function
>> - more change to the code flow in general
>>
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>
>> [1]: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022b/index.html
>>
>> Andre Przywara (6):
>>    arm: rename xen/arch/arm/psci.c into vpsci.c
>>    arm: move GIC SGI kicking into separate function
>>    arm: parse PSCI node from the host device-tree
>>    arm: add a function to invoke the PSCI handler
>>    arm32: enable PSCI secondary CPU bringup
>>    arm64: enable PSCI secondary CPU bringup
>>
>>   xen/arch/arm/Makefile             |   1 +
>>   xen/arch/arm/arm32/smpboot.c      |   4 +-
>>   xen/arch/arm/arm64/smpboot.c      |  23 ++++++-
>>   xen/arch/arm/platform.c           |   6 +-
>>   xen/arch/arm/platforms/exynos5.c  |  11 +---
>>   xen/arch/arm/platforms/omap5.c    |  11 +---
>>   xen/arch/arm/platforms/vexpress.c |  10 +--
>>   xen/arch/arm/psci.c               | 127 +++++++++++++++++++-------------------
>>   xen/arch/arm/smpboot.c            |  22 +++++--
>>   xen/arch/arm/vpsci.c              |  93 ++++++++++++++++++++++++++++
>>   xen/include/asm-arm/psci.h        |   7 +++
>>   xen/include/asm-arm/smp.h         |   2 +
>>   12 files changed, 215 insertions(+), 102 deletions(-)
>>   create mode 100644 xen/arch/arm/vpsci.c
>>
>
>