[U-Boot] [PATCH 6/8] ARMv8: PSCI: Fixup the device tree for PSCI v0.2

[Arnab Basu]

Hi Mark

On 08/28/2014 06:14 PM, Mark Rutland wrote:
> Hi,
>
> On Wed, Aug 27, 2014 at 09:29:59PM +0100, Arnab Basu wrote:
>> Set the enable-method in the cpu node to psci, create the psci
>> device tree node and also add a reserve section for the psci code
>> that lives in in normal RAM, so that the kernel leaves it alone
>>
>> Signed-off-by: Arnab Basu <arnab.basu at freescale.com>
>> Reviewed-by: Bhupesh Sharma <bhupesh.sharma at freescale.com>
>> Cc: Marc Zyngier <marc.zyngier at arm.com>
>> ---
>>   arch/arm/cpu/armv8/cpu-dt.c   |   67 +++++++++++++++++++++++++++++++++++++++++
>>   arch/arm/include/asm/system.h |    4 ++
>>   2 files changed, 71 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv8/cpu-dt.c b/arch/arm/cpu/armv8/cpu-dt.c
>> index 9792bc0..c2c8fe7 100644
>> --- a/arch/arm/cpu/armv8/cpu-dt.c
>> +++ b/arch/arm/cpu/armv8/cpu-dt.c
>> @@ -9,7 +9,69 @@
>>   #include <fdt_support.h>
>>
>>   #ifdef CONFIG_MP
>> +#ifdef CONFIG_ARMV8_PSCI
>>
>> +static void psci_reserve_mem(void *fdt)
>> +{
>> +#ifndef CONFIG_ARMV8_SECURE_BASE
>> +	/* secure code lives in RAM, keep it alive */
>> +	fdt_add_mem_rsv(fdt, (unsigned long)__secure_start,
>> +			__secure_end - __secure_start);
>> +#endif
>> +}
>
> With PSCI I'd be worried about telling the OS about this memory at all.
>
> If the OS maps the memory we could encounter issues with mismatched
> aliases and/or unexpected hits in the D-cache, which can result in a
> loss of ordering and/or visbility guarantees, which could break the PSCI
> implementation.
>
> With the KVM or trusted firmware PSCI implementations the (guest) OS
> cannot map the implementation's memory, preventing such problems. The
> arm64 Linux boot-wrapper is dodgy in this regard currently.
>

The idea here is that if there is no PSCI specific (most likely secure) 
memory allocated in the system, the macro "CONFIG_ARMV8_SECURE_BASE" 
will not be defined. In this case the PSCI vector table and its support 
code will be in DDR and will be protected from Linux using memreserve.

If this macro is defined the assumption is that it points to some 
non-ddr location, say secure OCRAM. In this case U-Boot will copy the 
PSCI vector table and its support code to that region and we are hoping 
that this address space is not visible to the OS in the first place.

This is my understanding of the code, maybe Marc would like to comment 
on if this was the thinking in ARMv7.

I realise that this probably needs to be documented.

>> +static int cpu_update_dt_psci(void *fdt)
>> +{
>> +	int nodeoff;
>> +	int tmp;
>> +
>> +	psci_reserve_mem(fdt);
>> +
>> +	nodeoff = fdt_path_offset(fdt, "/cpus");
>> +	if (nodeoff < 0) {
>> +		printf("couldn't find /cpus\n");
>> +		return nodeoff;
>> +	}
>> +
>> +	/* add 'enable-method = "psci"' to each cpu node */
>> +	for (tmp = fdt_first_subnode(fdt, nodeoff);
>> +	     tmp >= 0;
>> +	     tmp = fdt_next_subnode(fdt, tmp)) {
>> +		const struct fdt_property *prop;
>> +		int len;
>> +
>> +		prop = fdt_get_property(fdt, tmp, "device_type", &len);
>> +		if (!prop)
>> +			continue;
>> +		if (len < 4)
>> +			continue;
>> +		if (strcmp(prop->data, "cpu"))
>> +			continue;
>> +
>> +		fdt_setprop_string(fdt, tmp, "enable-method", "psci");
>
> Do we need to check the return code here, as we do when setting up the
> psci node?
>

Probably, I'll add it.

>> +	}
>> +
>> +	nodeoff = fdt_path_offset(fdt, "/psci");
>
> We might need to search by compatible string. All psci nodes so far have
> been called /psci, but that's not guaranteed. Linux looks for nodes
> compatible with "arm,psci" and/or "arm,psci-0.2".
>

I see that it is called "Main node" in the kernel documentation. Any 
reason it's name has not been fixed to "psci"? Is it too late to do that 
and save myself some work here? :)

>> +	if (nodeoff < 0) {
>> +		nodeoff = fdt_path_offset(fdt, "/");
>> +		if (nodeoff < 0)
>> +			return nodeoff;
>> +
>> +		nodeoff = fdt_add_subnode(fdt, nodeoff, "psci");
>> +		if (nodeoff < 0)
>> +			return nodeoff;
>> +	}
>> +
>> +	tmp = fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci-0.2");
>> +	if (tmp)
>> +		return tmp;
>> +	tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc");
>> +	if (tmp)
>> +		return tmp;
>> +
>> +	return 0;
>> +}
>
> Otherwise this looks fine.

Thanks

>
> Mark.
>