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

[Mark Rutland <mark.rutland@arm.com>]

Hi,

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

Sure, this will prevent the OS from explicitly modifying this memory.

However, the OS will still map the memory. This renders the protection
incomplete due to the possibility of mismatched attributes and/or
unexpected cache hits resulting in nasty coherency problems. We are
likely to get away with this most of the time (if the kernel and U-Boot
use the same attributes), but it would be very easy to blow things up
accidentally.

The only way to prevent that is to completely remove a portion of the
memory from the view of the OS, such that it doesn't map the memory at
all.

> 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 makes sense, but was not the issue I was referring to.

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

If we're doing this on ARMv7 then it is dodgy there too.

Marc, thoughts?

[...]

> >> +	}
> >> +
> >> +	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? :)

Unfortunately the canonical way to find the PSCI node is by compatible
string, and that's what Linux does. While we might be able to ensure all
in-tree dts follow this convention, it's not something that should be
relied upon.

Sorry :(

Cheers,
Mark.