Re: [U-Boot] [PATCH 6/8] ARMv8: PSCI: Fixup the device tree for PSCI v0.2
On Tue, Sep 02, 2014 at 04:21:24PM +0100, Stuart Yoder wrote: 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. Can't this be done by simply removing that secure portion of memory from the memory advertised in the memory node of the device tree passed to the non-secure OS? ...should prevent the OS from mapping the memory. Yes, removing such memory entirely from the memory nodes would work. The only caveat (I believe) is that it would be necessary to remove such memory in 2MB naturally-aligned chunks due to the way Linux maps memory. I intend to at some point decouple the Linux linear mapping from the text mapping, so that Linux can address meemory below it. So it's vital to remove the memory enitrely from the view of the kernel rather than just loading the kernel 2MB higher. Mark. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 6/8] ARMv8: PSCI: Fixup the device tree for PSCI v0.2
On Mon, Sep 01, 2014 at 07:43:18PM +0100, Mark Rutland wrote:
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.
To clarify:
If the PSCI implementation uses some memory not described to the OS
there is no problem. Ideally this would be some secure SRAM somwhere,
which the OS can never map. So if you are using some secure RAM then
there is no issue.
If the memory is described to the non-secure OS, then there can be
coherency issues unless either:
* The caches are not in use at EL3. This necessitates something like
bakery locks for synchronization.
* The memory is mapped at EL3 as secure, and the core makes a
distinction between secure and non-secure memory (see
ID_AA64MMFR0_EL1.SNSMem). Otherwise misatched attributes can cause
coherency issues (see B2.9 Mismatched memory attributes in the ARM
ARM).
Thanks,
Mark.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 6/8] ARMv8: PSCI: Fixup the device tree for PSCI v0.2
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. Can't this be done by simply removing that secure portion of memory from the memory advertised in the memory node of the device tree passed to the non-secure OS? ...should prevent the OS from mapping the memory. Stuart ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 6/8] ARMv8: PSCI: Fixup the device tree for PSCI v0.2
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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 6/8] ARMv8: PSCI: Fixup the device tree for PSCI v0.2
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.b...@freescale.com
Reviewed-by: Bhupesh Sharma bhupesh.sha...@freescale.com
Cc: Marc Zyngier marc.zyng...@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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 6/8] ARMv8: PSCI: Fixup the device tree for PSCI v0.2
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.b...@freescale.com
Reviewed-by: Bhupesh Sharma bhupesh.sha...@freescale.com
Cc: Marc Zyngier marc.zyng...@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.
+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?
+ }
+
+ 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.
+ 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.
Mark.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 6/8] ARMv8: PSCI: Fixup the device tree for PSCI v0.2
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.b...@freescale.com
Reviewed-by: Bhupesh Sharma bhupesh.sha...@freescale.com
Cc: Marc Zyngier marc.zyng...@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
+}
+
+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);
+ }
+
+ nodeoff = fdt_path_offset(fdt, /psci);
+ 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;
+}
+#else
__weak u64 arch_get_release_addr(u64 cpu_id)
{
return 0;
@@ -51,11 +113,16 @@ static void cpu_update_dt_spin_table(void *blob)
arch_spin_table_reserve_mem(blob);
}
#endif
+#endif
int cpu_update_dt(void *fdt)
{
#ifdef CONFIG_MP
+#ifdef CONFIG_ARMV8_PSCI
+ cpu_update_dt_psci(fdt);
+#else
cpu_update_dt_spin_table(fdt);
#endif
+#endif
return 0;
}
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index d51ba66..a1066d4 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -77,6 +77,10 @@ void gic_init(void);
void gic_send_sgi(unsigned long sgino);
void wait_for_wakeup(void);
void smp_kick_all_cpus(void);
+#ifdef CONFIG_ARMV8_PSCI
+extern char __secure_start[];
+extern char __secure_end[];
+#endif
void flush_l3_cache(void);
--
1.7.7.4
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
