arm64/efi: improve TEXT_OFFSET handling

From: Mark Rutland <[email protected]>

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 <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

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

Quoted 21 lines · Will Deacon

Quoted 19 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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 <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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.

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 26 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 34 lines · Mark Rutland

Quoted 32 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 35 lines · Will Deacon

Quoted 34 lines · Mark Rutland

Quoted 32 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 2 lines

I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case.

Quoted 1 line

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?

Quoted 46 lines · Mark Rutland

Quoted 35 lines · Will Deacon

Quoted 34 lines · Mark Rutland

Quoted 32 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 2 lines

I'll need to have a think about that, I'm not sure on the architected cache behaviour in such a case.

Quoted 8 lines

Quoted 1 line

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.

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

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

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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.

Quoted 26 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 6 lines

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

Quoted 34 lines · Will Deacon

Quoted 26 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 48 lines · Mark Rutland

Quoted 34 lines · Will Deacon

Quoted 26 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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?

Will

Quoted 48 lines · Mark Rutland

Quoted 34 lines · Will Deacon

Quoted 26 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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?

Quoted 49 lines · Will Deacon

Quoted 48 lines · Mark Rutland

Quoted 34 lines · Will Deacon

Quoted 26 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 44 lines · Mark Rutland

Quoted 34 lines · Will Deacon

Quoted 26 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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?

Quoted 26 lines · Ard Biesheuvel

Quoted 21 lines · Will Deacon

Quoted 12 lines · Ard Biesheuvel

From: Mark Rutland <[email protected]>

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.

Quoted 7 lines

Signed-off-by Mark Rutland <[email protected]>

Tested-by Mark Salter <[email protected]>

[ardb: added (__force void *) cast]

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Quoted 3 lines

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?

Quoted 6 lines

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

The static memory footprint of a kernel Image at boot is larger than the Image file itself. Things like .bss data and initial page tables are allocated statically but populated dynamically so their content is not contained in the Image file.

However, if EFI (or GRUB) has loaded the Image at precisely the desired offset of base of DRAM + TEXT_OFFSET, the Image will be booted in place, and we have to make sure that the allocation done by the PE/COFF loader is large enough.

Fix this by growing the PE/COFF .text section to cover the entire static memory footprint. The part of the section that is not covered by the payload will be zero initialised by the PE/COFF loader.

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Mark Salter <[email protected]>

---
 arch/arm64/kernel/head.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/arm64/kernel/head.S +3 -3

index 144f10567f82..b6ca95aee348 100644
@@ -151,7 +151,7 @@ optional_header:
 	.short	0x20b				// PE32+ format
 	.byte	0x02				// MajorLinkerVersion
 	.byte	0x14				// MinorLinkerVersion
-	.long	_edata - stext			// SizeOfCode
+	.long	_end - stext			// SizeOfCode
 	.long	0				// SizeOfInitializedData
 	.long	0				// SizeOfUninitializedData
 	.long	efi_stub_entry - efi_head	// AddressOfEntryPoint

@@ -169,7 +169,7 @@ extra_header_fields:
 	.short	0				// MinorSubsystemVersion
 	.long	0				// Win32VersionValue
 
-	.long	_edata - efi_head		// SizeOfImage
+	.long	_end - efi_head			// SizeOfImage
 
 	// Everything before the kernel image is considered part of the header
 	.long	stext - efi_head		// SizeOfHeaders

@@ -216,7 +216,7 @@ section_table:
 	.byte	0
 	.byte	0
 	.byte	0        		// end of 0 padding of section name
-	.long	_edata - stext		// VirtualSize
+	.long	_end - stext		// VirtualSize
 	.long	stext - efi_head	// VirtualAddress
 	.long	_edata - stext		// SizeOfRawData
 	.long	stext - efi_head	// PointerToRawData

Signature · 6 lines

1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise.

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Mark Salter <[email protected]>

---
 arch/arm64/kernel/efi-stub.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

arch/arm64/kernel/efi-stub.c +6 -10

index 1317fef8dde9..d27dd982ff26 100644
@@ -28,20 +28,16 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
 	kernel_size = _edata - _text;
 	if (*image_addr != (dram_base + TEXT_OFFSET)) {
 		kernel_memsize = kernel_size + (_end - _edata);
-		status = efi_relocate_kernel(sys_table, image_addr,
-					     kernel_size, kernel_memsize,
-					     dram_base + TEXT_OFFSET,
-					     PAGE_SIZE);
+		status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
+				       SZ_2M, reserve_addr);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Failed to relocate kernel\n");
 			return status;
 		}
-		if (*image_addr != (dram_base + TEXT_OFFSET)) {
-			pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
-			efi_free(sys_table, kernel_memsize, *image_addr);
-			return EFI_LOAD_ERROR;
-		}
-		*image_size = kernel_memsize;
+		memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
+		       kernel_size);
+		*image_addr = *reserve_addr + TEXT_OFFSET;
+		*reserve_size = kernel_memsize + TEXT_OFFSET;
 	}
 
 

Signature · 1 line

1.8.3.2

Quoted 7 lines · Ard Biesheuvel

If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise.

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Mark Salter <[email protected]>

Acked-by Mark Rutland <[email protected]>

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Ard Biesheuvel

If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Mark Salter <[email protected]>

Acked-by Mark Rutland <[email protected]>

Ard, who's picking this up?

Quoted 7 lines · Matt Fleming

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Ard Biesheuvel

If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Mark Salter <[email protected]>

Acked-by Mark Rutland <[email protected]>

Ard, who's picking this up?

Will's already taken this into arm64/devel [1,2] with the intention of waiting for v3.18 [3]. Per Leif's comment [4] that might have to be bumped.

Will?

Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/commit/?h=devel
[2] https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/commit/?h=devel&id=12f002aa8d96b90e6e37cc2b0d9a6a3cacdf8ef5
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/279655.html
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/279771.html

Hey Matt,

These patches mostly touch non-EFI specific files under arch/arm64, so to prevent conflicts, it makes sense for the arm64 tree to take them.

Quoted 10 lines · Matt Fleming

Quoted 9 lines · Mark Rutland

Quoted 7 lines · Ard Biesheuvel

If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise.

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Mark Salter <[email protected]>

Quoted 3 lines

Quoted 1 line

Acked-by Mark Rutland <[email protected]>

Ard, who's picking this up?

Will's already taken this into arm64/devel [1,2] with the intention of waiting for v3.18 [3]. Per Leif's comment [4] that might have to be bumped.

Quoted 15 lines · Mark Rutland

Quoted 13 lines · Matt Fleming

Quoted 10 lines · Mark Rutland

Quoted 7 lines · Ard Biesheuvel

If we cannot relocate the kernel Image to its preferred offset of base of DRAM plus TEXT_OFFSET, instead relocate it to the lowest available 2 MB boundary plus TEXT_OFFSET. We may lose a bit of memory at the low end, but we can still proceed normally otherwise.

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Mark Salter <[email protected]>

Acked-by Mark Rutland <[email protected]>

Ard, who's picking this up?

Quoted 3 lines

Will's already taken this into arm64/devel [1,2] with the intention of waiting for v3.18 [3]. Per Leif's comment [4] that might have to be bumped.

So what's the plan with this series? Waiting for 3.18? The problem is that this patch series needs to be pulled for any platform with an EFI firmware located at DRAM base (e.g. AMD Seattle) as was noted before.

Resending this series sent out yesterday with only minor changes and acks etc added.

In summary: patch #3 relaxes the requirements imposed by the EFI stub on where Image may be loaded, but this breaks APM Mustang (if booting via UEFI) if patch #1 does not go in first. Patch #2 prevents potential boot issues when Image is loaded such that the stub does not have to relocate it.

@Will: as discussed on the list yesterday, these patches should be kept in sequence when going upstream, so it is best to take them through a single tree. However, patch #3 will not apply cleanly to the arm64 tree until after 3.16-rc1 is released as it depends on a trivial change going in through x86/tip [efi].
[https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=next&id=6091c9c447370c4717ec9975813c874af490eb36]

If you are ok with these patches, how would you like to proceed? Patches #1 and #2 could go in straight away (through arm64), and I can send Catalin and you a gentle reminder once -rc1 is released to take #3? Or instead, ack them and ask Matt to queue them for 3.17-late? It would be nice if this makes 3.17 as it fixes actual boot problems on hardware that is under development.

Changes in v2:
- add (__force void *) cast to patch #1, as suggested in LAKML discussion
- add tested-by/acked-by lines
- rebased patch #3 onto efi-next

Mark Rutland (1):
arm64: spin-table: handle unmapped cpu-release-addrs

Ard Biesheuvel (2):
arm64/efi: efistub: cover entire static mem footprint in PE/COFF .text arm64/efi: efistub: don't abort if base of DRAM is occupied

 arch/arm64/kernel/efi-stub.c       | 18 ++++++------------
 arch/arm64/kernel/head.S           |  6 +++---
 arch/arm64/kernel/smp_spin_table.c | 21 ++++++++++++++++-----
 3 files changed, 25 insertions(+), 20 deletions(-)

Signature · 1 line

1.8.3.2