[U-Boot] [PATCH 0/9] ARMv7: add PSCI support to u-boot

[Andre Przywara]

On 12/06/2013 01:12 PM, Marc Zyngier wrote:
> Hi Andre,
>
> On 06/12/13 11:43, Andre Przywara wrote:
>> On 11/21/2013 09:59 AM, Marc Zyngier wrote:
>>> PSCI is an ARM standard that provides a generic interface that
>>> ...
>>
>> Thanks again for posting this, I like the idea of adding PSCI handlers
>> to u-boot for several platforms very much.
>>
>>> This patch series contains 3 parts:
>>> - the first four patches are just bug fixes
>>
>> Those are fine, I already acked those patches.
>>
>>> - the next three contain the generic PSCI code and build infrastructure
>>
>> As I heard you will rework these anyway, I will refrain from commenting
>> in detail, just some generic comments on the approach:
>>
>> * Is the creation of a top-level psci directory for holding the PSCI
>> binaries really necessary? Should this mimic the spl approach?
>> Can you consider to move this at least into the arch/arm directory, as
>> PSCI is ARM specific? Or add it to the SPL directory, as this serves a
>> similar purpose? But maybe your new approach renders this all moot.
>
> The code base I have at the moment completely gets rid of the psci
> directory, and make that code a separate section that can be relocated
> to secure memory or simply marked as reserved.

Nice!

>
>> * Can you keep the SMP bringup code in place and re-use it from the PSCI
>> handlers instead of "#ifndef PSCI"ing it? So maybe rename
>> arch/arm/cpu/armv7/sunxi/psci.S to .../sunxi/smp.S?
>> My idea here is to make PSCI an option in addition to the current SMP
>> HYP mode code. So that for instance on VExpress (or better: Arndale) you
>> could either use the existing code using the kernel's platform SMP code
>> or enable PSCI in u-boot and let the kernel use that, too.
>
> The main problem is the SMP wake-up code in u-boot. With PSCI, you
> really don't want to wake up the secondaries at all, and you wait until
> the kernel does an SMC. The current code wakes up the CPUs
> unconditionnally,

Yes, and I want to do away with this if PSCI is defined - as this is not 
needed. I just asked for keeping the SMP wakeup code as generic as 
possible and neither tie it to PSCI nor HYP mode, so that either of them 
can reference that.
But, ...

> and put them in a separate pen, and I'd really like to
> avoid dealing with a pen in the PSCI case (stupid platforms like
> VExpress might still require it, but that's an orthogonal discussion).

Yes, there is indeed this problem with the pen. Maybe one can use your 
upcoming relocation code to move the pen to a more secure place (defined 
per platform).
With "avoid dealing with a pen" you mean to wakeup from the system's pen 
and immediately jump to the OS init code, without staying in a wfi loop 
in some special memory?

>
>> I maybe ask too much for the first incarnation of the code, but that is
>> how I would like to eventually see it. AFAIK Linux prefers PSCI over
>> platform-defined SMP code, so this should work out of the box.
>
> Not really. So far, it will use the the platform SMP code if it is
> defined, and fallback to PSCI otherwise.

Indeed. So I was mistaken on this. I thought it worked this way once at 
least with Highbank/Midway (could have been an internal branch, though).

> There were patches from Stephen
> Boyd to use the "enable-method" property in the cpu node to select PSCI
> though. I need to ping him on that.

That one that is mandatory on ARM64? Makes sense, then.

Regards,
Andre.

>
>> * Is the use of TPIDRPRW & Co. really safe? It looks like as we seem to
>> be the only secure user (and they are banked), but I am just curious
>> whether there is any "prior art" in using those registers temporarily.
>
> As you said, we own secure mode entirely. So they really are scratch
> registers, free to be used.
>
>>> ...
>>> The kernel now boots in HYP mode, finds its secondary CPU without any
>>> additional SMP code, and runs KVM out of the box. Hopefully, the
>>> Xen/ARM guys can do the same fairly easily.
>>
>> BTW: Yesterday my PSCI host patches for Xen have been committed, so Xen
>> should be able to use that feature just like the kernel does.
>
> Excellent! I really need to sort these patches out and repost the whole
> series...
>
> Thanks,
>
> 	M.
>