Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 31 July 2014 12:39, Mark Rutland mark.rutl...@arm.com wrote: On Thu, Jul 31, 2014 at 11:04:39AM +0100, Will Deacon wrote: On Thu, Jul 31, 2014 at 10:58:54AM +0100, Mark Rutland wrote: On Thu, Jul 31, 2014 at 10:45:15AM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 08:17:02PM +0100, Ard Biesheuvel wrote: ]On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? Actually, booting.txt requires cpu-release-addr to point to a /memreserve/d part of memory, which implies DRAM (or you wouldn't have to memreserve it) That means it should always be covered by the linear mapping, unless it is located before Image in DRAM, which is the case addressed by this patch. But if it's located before before the Image in DRAM and isn't covered by the linear mapping, then surely the /memreserve/ is pointless too? In which case, this looks like we're simply trying to cater for platforms that aren't following booting.txt (which may need updating if we need to handle this). No. The DT is describing the memory which is present, and the subset thereof which should not be used under normal circumstances. That's a static property of the system. Where the OS happens to get loaded and what it is able to address is a dynamic property of the OS (and possibly the bootloader). The DT cannot have knowledge of this. It's always true that the OS should not blindly use memreserve'd memory. The fact that it cannot address it in the linear mapping is orthogonal. In which case, I think asserting that /memreserve/ implies DRAM is pretty fragile and not actually enforced anywhere. Sure, we can say `don't do that', but I'd prefer to have the kernel detect this dynamically. I think the boot protocol needs an update to allow a cpu-release-addr not covered by linear mapping. There are reasons that the kernel might not be loaded at the start of RAM, and I think relying on the cpu-release-addr addresses lying in the linear mapping is a limitation we need to address. Given that I also think we should allow for cpu-release-addrs outside of the range desribed by memory nodes (and therefore not requiring any /mremreserve/). While I agree that it would be a nice thing to get that requirement relaxed, do we necessarily need to address both issues at once? In a sense, this patch is a bug fix: even if a platform fully adheres to booting.txt, by putting the cpu-release-addr in a memreserved part of DRAM and loading Image at a 2 meg offset + TEXT_OFFSET, SMP will be broken in some cases, and that needs to be fixed. This issue is not imaginary, as TEXT_OFFSET fuzzing may well result in boot failures on APM Mustang, and the fix for /that/ (loading at the next 2 meg boundary (+ TEXT_OFFSET) up, a thing which booting.txt specifically allows) triggers the issue that this patch addresses. I do not think we should rely on being able to address the cpu-release-addr with a normal cacheable mapping. If the cpu-release-addr falls outside of the memory described by the memory node(s) then we have no idea where it lives. Currently this falls in normal memory, but mandating that feels odd. We have the luxury that all existing working implementations have cpu-release-addr inside the linear mapping (or SMP would already be broken). Why makes our lives complicated by allowing things that nobody has asked for yet? Including dedicated SRAM patches makes sense, since anything that can tolerate being mapped MT_NORMAL using 64k granule can be supported with your current code, but beyond that, what is the use case? The sole purpose of /memreserve/ is to describe areas in physical memory that memory should not be used for general allocation. I don't think it makes any
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On Wed, Jul 30, 2014 at 08:17:02PM +0100, Ard Biesheuvel wrote: ]On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? Actually, booting.txt requires cpu-release-addr to point to a /memreserve/d part of memory, which implies DRAM (or you wouldn't have to memreserve it) That means it should always be covered by the linear mapping, unless it is located before Image in DRAM, which is the case addressed by this patch. But if it's located before before the Image in DRAM and isn't covered by the linear mapping, then surely the /memreserve/ is pointless too? In which case, this looks like we're simply trying to cater for platforms that aren't following booting.txt (which may need updating if we need to handle this). Will -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On Thu, Jul 31, 2014 at 10:45:15AM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 08:17:02PM +0100, Ard Biesheuvel wrote: ]On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? Actually, booting.txt requires cpu-release-addr to point to a /memreserve/d part of memory, which implies DRAM (or you wouldn't have to memreserve it) That means it should always be covered by the linear mapping, unless it is located before Image in DRAM, which is the case addressed by this patch. But if it's located before before the Image in DRAM and isn't covered by the linear mapping, then surely the /memreserve/ is pointless too? In which case, this looks like we're simply trying to cater for platforms that aren't following booting.txt (which may need updating if we need to handle this). No. The DT is describing the memory which is present, and the subset thereof which should not be used under normal circumstances. That's a static property of the system. Where the OS happens to get loaded and what it is able to address is a dynamic property of the OS (and possibly the bootloader). The DT cannot have knowledge of this. It's always true that the OS should not blindly use memreserve'd memory. The fact that it cannot address it in the linear mapping is orthogonal. Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 31 July 2014 12:04, Will Deacon will.dea...@arm.com wrote: On Thu, Jul 31, 2014 at 10:58:54AM +0100, Mark Rutland wrote: On Thu, Jul 31, 2014 at 10:45:15AM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 08:17:02PM +0100, Ard Biesheuvel wrote: ]On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? Actually, booting.txt requires cpu-release-addr to point to a /memreserve/d part of memory, which implies DRAM (or you wouldn't have to memreserve it) That means it should always be covered by the linear mapping, unless it is located before Image in DRAM, which is the case addressed by this patch. But if it's located before before the Image in DRAM and isn't covered by the linear mapping, then surely the /memreserve/ is pointless too? In which case, this looks like we're simply trying to cater for platforms that aren't following booting.txt (which may need updating if we need to handle this). No. The DT is describing the memory which is present, and the subset thereof which should not be used under normal circumstances. That's a static property of the system. Where the OS happens to get loaded and what it is able to address is a dynamic property of the OS (and possibly the bootloader). The DT cannot have knowledge of this. It's always true that the OS should not blindly use memreserve'd memory. The fact that it cannot address it in the linear mapping is orthogonal. In which case, I think asserting that /memreserve/ implies DRAM is pretty fragile and not actually enforced anywhere. Sure, we can say `don't do that', but I'd prefer to have the kernel detect this dynamically. The point is whether we can assume that cpu-release-addr always resides in DRAM, not whether /memreserve/ implies DRAM. The former should be the case for all current implementations, because we only ever access it through the linear mapping. This means that rather than worrying about all the corner cases where cpu-release-addr may share its 64k physical frame with device registers etc, couldn't we just update booting.txt to state that cpu-release-addr should be chosen such that it can be mapped with a 64k granule MT_NORMAL mapping? -- Ard. Does dtc check that the /memreserve/ region is actually a subset of the memory node? Will -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On Thu, Jul 31, 2014 at 11:04:39AM +0100, Will Deacon wrote: On Thu, Jul 31, 2014 at 10:58:54AM +0100, Mark Rutland wrote: On Thu, Jul 31, 2014 at 10:45:15AM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 08:17:02PM +0100, Ard Biesheuvel wrote: ]On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? Actually, booting.txt requires cpu-release-addr to point to a /memreserve/d part of memory, which implies DRAM (or you wouldn't have to memreserve it) That means it should always be covered by the linear mapping, unless it is located before Image in DRAM, which is the case addressed by this patch. But if it's located before before the Image in DRAM and isn't covered by the linear mapping, then surely the /memreserve/ is pointless too? In which case, this looks like we're simply trying to cater for platforms that aren't following booting.txt (which may need updating if we need to handle this). No. The DT is describing the memory which is present, and the subset thereof which should not be used under normal circumstances. That's a static property of the system. Where the OS happens to get loaded and what it is able to address is a dynamic property of the OS (and possibly the bootloader). The DT cannot have knowledge of this. It's always true that the OS should not blindly use memreserve'd memory. The fact that it cannot address it in the linear mapping is orthogonal. In which case, I think asserting that /memreserve/ implies DRAM is pretty fragile and not actually enforced anywhere. Sure, we can say `don't do that', but I'd prefer to have the kernel detect this dynamically. I think the boot protocol needs an update to allow a cpu-release-addr not covered by linear mapping. There are reasons that the kernel might not be loaded at the start of RAM, and I think relying on the cpu-release-addr addresses lying in the linear mapping is a limitation we need to address. Given that I also think we should allow for cpu-release-addrs outside of the range desribed by memory nodes (and therefore not requiring any /mremreserve/). I do not think we should rely on being able to address the cpu-release-addr with a normal cacheable mapping. If the cpu-release-addr falls outside of the memory described by the memory node(s) then we have no idea where it lives. Currently this falls in normal memory, but mandating that feels odd. The sole purpose of /memreserve/ is to describe areas in physical memory that memory should not be used for general allocation. I don't think it makes any sense to derive any information from /memreserve/ other than the fact said addresses shouldn't be poked arbitarily. If we allow cpu-release-addrs outside of memory, then we won't have a /memreserve/ anyhow. So the question becomes can or can't we always detect when we already have a mapping that covers a cpu-release-addr? Does dtc check that the /memreserve/ region is actually a subset of the memory node? I don't beleive it does. It's probably a sensible warning, but as far as I am aware the only time the memory reservation table will be read in any OS is to poke holes in its memory allocation pool(s). Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On Thu, 2014-07-31 at 11:04 +0100, Will Deacon wrote: On Thu, Jul 31, 2014 at 10:58:54AM +0100, Mark Rutland wrote: On Thu, Jul 31, 2014 at 10:45:15AM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 08:17:02PM +0100, Ard Biesheuvel wrote: ]On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? Actually, booting.txt requires cpu-release-addr to point to a /memreserve/d part of memory, which implies DRAM (or you wouldn't have to memreserve it) That means it should always be covered by the linear mapping, unless it is located before Image in DRAM, which is the case addressed by this patch. But if it's located before before the Image in DRAM and isn't covered by the linear mapping, then surely the /memreserve/ is pointless too? In which case, this looks like we're simply trying to cater for platforms that aren't following booting.txt (which may need updating if we need to handle this). No. The DT is describing the memory which is present, and the subset thereof which should not be used under normal circumstances. That's a static property of the system. Where the OS happens to get loaded and what it is able to address is a dynamic property of the OS (and possibly the bootloader). The DT cannot have knowledge of this. It's always true that the OS should not blindly use memreserve'd memory. The fact that it cannot address it in the linear mapping is orthogonal. In which case, I think asserting that /memreserve/ implies DRAM is pretty fragile and not actually enforced anywhere. Sure, we can say `don't do that', but I'd prefer to have the kernel detect this dynamically. Does dtc check that the /memreserve/ region is actually a subset of the memory node? The handling of /memreserve/ in drivers/of/fdt.c uses the memblock API to reserve. And that means it is assumed that /memreserve/ is something which can be covered by the normal kernel RAM mapping. I suspect having /memreserve/ outside the kernel mapping would cause problems for the mm code. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
From: Mark Rutland mark.rutl...@arm.com
In certain cases the cpu-release-addr of a CPU may not fall in the
linear mapping (e.g. when the kernel is loaded above this address due to
the presence of other images in memory). This is problematic for the
spin-table code as it assumes that it can trivially convert a
cpu-release-addr to a valid VA in the linear map.
This patch modifies the spin-table code to use a temporary cached
mapping to write to a given cpu-release-addr, enabling us to support
addresses regardless of whether they are covered by the linear mapping.
Signed-off-by: Mark Rutland mark.rutl...@arm.com
Tested-by: Mark Salter msal...@redhat.com
[ardb: added (__force void *) cast]
Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
---
arch/arm64/kernel/smp_spin_table.c | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/kernel/smp_spin_table.c
b/arch/arm64/kernel/smp_spin_table.c
index 0347d38eea29..4f93c67e63de 100644
--- a/arch/arm64/kernel/smp_spin_table.c
+++ b/arch/arm64/kernel/smp_spin_table.c
@@ -20,6 +20,7 @@
#include linux/init.h
#include linux/of.h
#include linux/smp.h
+#include linux/types.h
#include asm/cacheflush.h
#include asm/cpu_ops.h
@@ -65,12 +66,21 @@ static int smp_spin_table_cpu_init(struct device_node *dn,
unsigned int cpu)
static int smp_spin_table_cpu_prepare(unsigned int cpu)
{
- void **release_addr;
+ __le64 __iomem *release_addr;
if (!cpu_release_addr[cpu])
return -ENODEV;
- release_addr = __va(cpu_release_addr[cpu]);
+ /*
+* The cpu-release-addr may or may not be inside the linear mapping.
+* As ioremap_cache will either give us a new mapping or reuse the
+* existing linear mapping, we can use it to cover both cases. In
+* either case the memory will be MT_NORMAL.
+*/
+ release_addr = ioremap_cache(cpu_release_addr[cpu],
+sizeof(*release_addr));
+ if (!release_addr)
+ return -ENOMEM;
/*
* We write the release address as LE regardless of the native
@@ -79,15 +89,17 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)
* boot-loader's endianess before jumping. This is mandated by
* the boot protocol.
*/
- release_addr[0] = (void *) cpu_to_le64(__pa(secondary_holding_pen));
-
- __flush_dcache_area(release_addr, sizeof(release_addr[0]));
+ writeq_relaxed(__pa(secondary_holding_pen), release_addr);
+ __flush_dcache_area((__force void *)release_addr,
+ sizeof(*release_addr));
/*
* Send an event to wake up the secondary CPU.
*/
sev();
+ iounmap(release_addr);
+
return 0;
}
--
1.8.3.2
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? Will -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 30 July 2014 14:00, Ard Biesheuvel ard.biesheu...@linaro.org wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain Clarification: every Runtime Services region known to it as being normal DRAM, which may cover this area ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On Wed, Jul 30, 2014 at 01:00:40PM +0100, Ard Biesheuvel wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? If we're careful about handling mismatched attributes we might be able to get away with always using a device mapping. I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On Wed, Jul 30, 2014 at 01:30:29PM +0100, Mark Rutland wrote: On Wed, Jul 30, 2014 at 01:00:40PM +0100, Ard Biesheuvel wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? If we're careful about handling mismatched attributes we might be able to get away with always using a device mapping. Even then, I think ioremap hits a WARN_ON if pfn_valid. I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case. Of we just skip the cache flush if !pfn_valid. Will -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On Wed, Jul 30, 2014 at 01:42:58PM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 01:30:29PM +0100, Mark Rutland wrote: On Wed, Jul 30, 2014 at 01:00:40PM +0100, Ard Biesheuvel wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? If we're careful about handling mismatched attributes we might be able to get away with always using a device mapping. Even then, I think ioremap hits a WARN_ON if pfn_valid. Ok, that's that idea dead then. I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case. Of we just skip the cache flush if !pfn_valid. I don't think that's always safe given Ard's comment that the EFI code will possibly have a mapping covering the region created by ioremap_cache. Ard, what exactly does the EFI code map with ioremap_cache, and why? Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 30 July 2014 14:49, Mark Rutland mark.rutl...@arm.com wrote: On Wed, Jul 30, 2014 at 01:42:58PM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 01:30:29PM +0100, Mark Rutland wrote: On Wed, Jul 30, 2014 at 01:00:40PM +0100, Ard Biesheuvel wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? If we're careful about handling mismatched attributes we might be able to get away with always using a device mapping. Even then, I think ioremap hits a WARN_ON if pfn_valid. Ok, that's that idea dead then. I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case. Of we just skip the cache flush if !pfn_valid. I don't think that's always safe given Ard's comment that the EFI code will possibly have a mapping covering the region created by ioremap_cache. Ard, what exactly does the EFI code map with ioremap_cache, and why? Actually, after re-reading the spec and the code, perhaps this is not an issue. The EFI __init code calls ioremap_cache() for all regions described by the UEFI memory map as requiring a virtual mapping (EFI_MEMORY_RUNTIME): this is primarily runtime services code and data regions and perhaps some I/O mappings for flash or other peripherals that UEFI owns and needs to access during Runtime Services calls. Mark Salter mentioned that APM Mustang's spin table lives in an EFI_RESERVED_TYPE region, which presumably would not have the EFI_MEMORY_RUNTIME attribute set, as it has nothing to do with the UEFI Runtime Services. This means that no cached mapping should already exist for that region. -- Ard. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] arm64: spin-table: handle unmapped cpu-release-addrs
On 30 July 2014 15:10, Ard Biesheuvel ard.biesheu...@linaro.org wrote: On 30 July 2014 14:49, Mark Rutland mark.rutl...@arm.com wrote: On Wed, Jul 30, 2014 at 01:42:58PM +0100, Will Deacon wrote: On Wed, Jul 30, 2014 at 01:30:29PM +0100, Mark Rutland wrote: On Wed, Jul 30, 2014 at 01:00:40PM +0100, Ard Biesheuvel wrote: On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote: On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote: From: Mark Rutland mark.rutl...@arm.com In certain cases the cpu-release-addr of a CPU may not fall in the linear mapping (e.g. when the kernel is loaded above this address due to the presence of other images in memory). This is problematic for the spin-table code as it assumes that it can trivially convert a cpu-release-addr to a valid VA in the linear map. This patch modifies the spin-table code to use a temporary cached mapping to write to a given cpu-release-addr, enabling us to support addresses regardless of whether they are covered by the linear mapping. Signed-off-by: Mark Rutland mark.rutl...@arm.com Tested-by: Mark Salter msal...@redhat.com [ardb: added (__force void *) cast] Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org --- arch/arm64/kernel/smp_spin_table.c | 22 +- 1 file changed, 17 insertions(+), 5 deletions(-) I'm nervous about this. What if the spin table sits in the same physical 64k frame as a read-sensitive device and we're running with 64k pages? I see what you mean. This is potentially hairy, as EFI already ioremap_cache()s everything known to it as normal DRAM, so using plain ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN may still result in mappings with different attributes for the same region. So how should we decide whether to call ioremap() or ioremap_cache() in this case? If we're careful about handling mismatched attributes we might be able to get away with always using a device mapping. Even then, I think ioremap hits a WARN_ON if pfn_valid. Ok, that's that idea dead then. I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case. Of we just skip the cache flush if !pfn_valid. I don't think that's always safe given Ard's comment that the EFI code will possibly have a mapping covering the region created by ioremap_cache. Ard, what exactly does the EFI code map with ioremap_cache, and why? Actually, after re-reading the spec and the code, perhaps this is not an issue. The EFI __init code calls ioremap_cache() for all regions described by the UEFI memory map as requiring a virtual mapping (EFI_MEMORY_RUNTIME): this is primarily runtime services code and data regions and perhaps some I/O mappings for flash or other peripherals that UEFI owns and needs to access during Runtime Services calls. Mark Salter mentioned that APM Mustang's spin table lives in an EFI_RESERVED_TYPE region, which presumably would not have the EFI_MEMORY_RUNTIME attribute set, as it has nothing to do with the UEFI Runtime Services. This means that no cached mapping should already exist for that region. That said, there is another potential snag: the UEFI spec for AArch64 does not allow regions residing in the same 64k phys frame to have different memory attributes, and in order to meet this requirement, an EFI_RESERVED_TYPE region could supposedly still be described with a EFI_MEMORY_WB (cacheable) attribute set (e.g. if it shares the 64k phys frame with Runtime Services Code or Data) -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
