efi: ARM/arm64: wire up ESRT table

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/firmware/efi/esrt.c +8 -8

index 22c5285f7705..f096a0a26dbd 100644
@@ -16,6 +16,7 @@
 #include <linux/device.h>
 #include <linux/efi.h>
 #include <linux/init.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/kobject.h>
 #include <linux/list.h>

@@ -385,15 +386,15 @@ static void cleanup_entry_list(void)
 static int __init esrt_sysfs_init(void)
 {
 	int error;
-	struct efi_system_resource_table __iomem *ioesrt;
+	struct efi_system_resource_table *memesrt;
 
 	pr_debug("esrt-sysfs: loading.\n");
 	if (!esrt_data || !esrt_data_size)
 		return -ENOSYS;
 
-	ioesrt = ioremap(esrt_data, esrt_data_size);
-	if (!ioesrt) {
-		pr_err("ioremap(%pa, %zu) failed.\n", &esrt_data,
+	memesrt = memremap(esrt_data, esrt_data_size, MEMREMAP_WB);
+	if (!memesrt) {
+		pr_err("memremap(%pa, %zu, MEMREMAP_WB) failed.\n", &esrt_data,
 		       esrt_data_size);
 		return -ENOMEM;
 	}

@@ -401,11 +402,12 @@ static int __init esrt_sysfs_init(void)
 	esrt = kmalloc(esrt_data_size, GFP_KERNEL);
 	if (!esrt) {
 		pr_err("kmalloc failed. (wanted %zu bytes)\n", esrt_data_size);
-		iounmap(ioesrt);
+		memunmap(memesrt);
 		return -ENOMEM;
 	}
 
-	memcpy_fromio(esrt, ioesrt, esrt_data_size);
+	memcpy(esrt, memesrt, esrt_data_size);
+	memunmap(memesrt);
 
 	esrt_kobj = kobject_create_and_add("esrt", efi_kobj);
 	if (!esrt_kobj) {

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
 	if (error)
 		goto err_cleanup_list;
 
-	memblock_remove(esrt_data, esrt_data_size);
-
 	pr_debug("esrt-sysfs: loaded.\n");
 
 	return 0;

Signature · 1 line

2.5.0

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
  drivers/firmware/efi/esrt.c | 16 ++++++++--------
  1 file changed, 8 insertions(+), 8 deletions(-)

drivers/firmware/efi/esrt.c +8 -6

index 22c5285f7705..f096a0a26dbd 100644
@@ -16,6 +16,7 @@
  #include <linux/device.h>
  #include <linux/efi.h>
  #include <linux/init.h>
+#include <linux/io.h>
  #include <linux/kernel.h>
  #include <linux/kobject.h>
  #include <linux/list.h>

@@ -385,15 +386,15 @@ static void cleanup_entry_list(void)
  static int __init esrt_sysfs_init(void)
  {
  	int error;
-	struct efi_system_resource_table __iomem *ioesrt;
+	struct efi_system_resource_table *memesrt;
 
  	pr_debug("esrt-sysfs: loading.\n");
  	if (!esrt_data || !esrt_data_size)
  		return -ENOSYS;
 
-	ioesrt = ioremap(esrt_data, esrt_data_size);
-	if (!ioesrt) {
-		pr_err("ioremap(%pa, %zu) failed.\n", &esrt_data,
+	memesrt = memremap(esrt_data, esrt_data_size, MEMREMAP_WB);
+	if (!memesrt) {
+		pr_err("memremap(%pa, %zu, MEMREMAP_WB) failed.\n", &esrt_data,
  		       esrt_data_size);
  		return -ENOMEM;
  	}

@@ -401,11 +402,12 @@ static int __init esrt_sysfs_init(void)
  	esrt = kmalloc(esrt_data_size, GFP_KERNEL);
  	if (!esrt) {
  		pr_err("kmalloc failed. (wanted %zu bytes)\n", esrt_data_size);
-		iounmap(ioesrt);
+		memunmap(memesrt);
  		return -ENOMEM;
  	}
 
-	memcpy_fromio(esrt, ioesrt, esrt_data_size);
+	memcpy(esrt, memesrt, esrt_data_size);
+	memunmap(memesrt);
 
  	esrt_kobj = kobject_create_and_add("esrt", efi_kobj);
  	if (!esrt_kobj) {

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

Quoted 8 lines

drivers/firmware/efi/esrt.c +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 

drivers/firmware/efi/esrt.c static int __init esrt_sysfs_init(void) · line 439

Quoted 1 line

drivers/firmware/efi/esrt.c +0 -0

      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

Quoted 30 lines · Matt Fleming

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

Quoted 8 lines

drivers/firmware/efi/esrt.c +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 

Quoted 5 lines

drivers/firmware/efi/esrt.c static int __init esrt_sysfs_init(void) · line 439

Quoted 1 line

drivers/firmware/efi/esrt.c +0 -0

      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ...

Quoted 37 lines · Ard Biesheuvel

Quoted 35 lines · Matt Fleming

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

drivers/firmware/efi/esrt.c static int __init esrt_sysfs_init(void) · line 439

Quoted 9 lines

drivers/firmware/efi/esrt.c +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 
      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

Quoted 3 lines

I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ...

If we're going to reserve it why do we need to copy the data out at all in esrt_sysfs_init()?

Quoted 38 lines · Matt Fleming

Quoted 37 lines · Ard Biesheuvel

Quoted 35 lines · Matt Fleming

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

drivers/firmware/efi/esrt.c static int __init esrt_sysfs_init(void) · line 439

Quoted 9 lines

drivers/firmware/efi/esrt.c +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 
      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

Quoted 6 lines

Quoted 3 lines

I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ...

If we're going to reserve it why do we need to copy the data out at all in esrt_sysfs_init()?

Excellent question. I don't think there is any point to doing that.

Quoted 43 lines · Ard Biesheuvel

Quoted 42 lines · Matt Fleming

Quoted 40 lines · Ard Biesheuvel

Quoted 30 lines · Matt Fleming

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

Quoted 8 lines

drivers/firmware/efi/esrt.c +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 

Quoted 5 lines

drivers/firmware/efi/esrt.c static int __init esrt_sysfs_init(void) · line 439

Quoted 1 line

drivers/firmware/efi/esrt.c +0 -0

      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ...

Quoted 4 lines

Quoted 2 lines

If we're going to reserve it why do we need to copy the data out at all in esrt_sysfs_init()?

Excellent question. I don't think there is any point to doing that.

... Unless the data is contained in an EFI Boot Services region ;-)

Peter?

Quoted 47 lines · Matt Fleming

Quoted 46 lines · Ard Biesheuvel

Quoted 44 lines · Matt Fleming

Quoted 37 lines · Ard Biesheuvel

Quoted 30 lines · Matt Fleming

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

Quoted 8 lines

(quoted diff) +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 

Quoted 5 lines

Review static int __init esrt_sysfs_init(void) · line 439

Quoted 1 line

(quoted diff) +0 -0

      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

Quoted 3 lines

I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ...

If we're going to reserve it why do we need to copy the data out at all in esrt_sysfs_init()?

Quoted 5 lines

Quoted 1 line

Excellent question. I don't think there is any point to doing that.

... Unless the data is contained in an EFI Boot Services region ;-)

Peter?

Yes, it usually is. Is that a problem?

Quoted 48 lines · Ard Biesheuvel

Quoted 47 lines · Matt Fleming

Quoted 46 lines · Ard Biesheuvel

Quoted 44 lines · Matt Fleming

Quoted 37 lines · Ard Biesheuvel

Quoted 30 lines · Matt Fleming

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

Quoted 8 lines

(quoted diff) +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 

Quoted 5 lines

Review static int __init esrt_sysfs_init(void) · line 439

Quoted 1 line

(quoted diff) +0 -0

      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

Quoted 3 lines

I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ...

If we're going to reserve it why do we need to copy the data out at all in esrt_sysfs_init()?

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Excellent question. I don't think there is any point to doing that.

... Unless the data is contained in an EFI Boot Services region ;-)

Peter?

Yes, it usually is. Is that a problem?

Yes, we free the Boot Services regions before hitting userspace on x86, see efi_free_boot_services(). We do this map/copy/unmap trick in the ACPI BGRT driver for that reason.

The Boot Services regions can be many gigabytes in size, which makes leaving them alone impractical.

For kexec on x86 we simply discard the BGRT table, which isn't the end of the world because who really needs access to the BGRT image on kexec reboot? However, I can see the value of preserving the ESRT.

I guess we've got two options, 1) copy out the chunks of Boot Services regions we're interested in and rewrite the EFI tables to point at these new allocations and free/discard all of the original Boot Services regions or 2) only selectively free the Boot Services regions.

I've always stayed clear of 2) in case there exists cross-region references in the data that isn't obvious. I'd like to think that would never happen, but, you know, dragons lurk here, etc.

Though actually, now I think about it, cross-region references can't possibly exist because they'd cause issues with the current code.

So maybe the best solution is actually 2), where we preserve the Boot Services regions if any of the drivers (ESRT, BGRT) request them but free all the others?

What are the lifetime rules for Boot Services regions on arm*?

We treat all Boot Services regions like Loader Code/Data or free
regions: it is all recorded in memblock as usable memory, and only the regions that are explicitly reserved are protected from further general use.

I am currently looking into the memory attribute table, and the use case is very similar. It would be very useful from our pov to simply memblock_reserve() the region right after having called efi_config_parse_tables(), and actually consume its data when we get around to it later. The ESRT handling is already split down the middle in the same way.

Quoted 78 lines · Matt Fleming

Quoted 48 lines · Ard Biesheuvel

Quoted 47 lines · Matt Fleming

Quoted 46 lines · Ard Biesheuvel

Quoted 44 lines · Matt Fleming

Quoted 37 lines · Ard Biesheuvel

Quoted 30 lines · Matt Fleming

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

Quoted 8 lines

(quoted diff) +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 

Quoted 5 lines

Review static int __init esrt_sysfs_init(void) · line 439

Quoted 1 line

(quoted diff) +0 -0

      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

Quoted 3 lines

I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ...

If we're going to reserve it why do we need to copy the data out at all in esrt_sysfs_init()?

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Excellent question. I don't think there is any point to doing that.

... Unless the data is contained in an EFI Boot Services region ;-)

Peter?

Yes, it usually is. Is that a problem?

Yes, we free the Boot Services regions before hitting userspace on x86, see efi_free_boot_services(). We do this map/copy/unmap trick in the ACPI BGRT driver for that reason.

The Boot Services regions can be many gigabytes in size, which makes leaving them alone impractical.

For kexec on x86 we simply discard the BGRT table, which isn't the end of the world because who really needs access to the BGRT image on kexec reboot? However, I can see the value of preserving the ESRT.

I guess we've got two options, 1) copy out the chunks of Boot Services regions we're interested in and rewrite the EFI tables to point at these new allocations and free/discard all of the original Boot Services regions or 2) only selectively free the Boot Services regions.

I've always stayed clear of 2) in case there exists cross-region references in the data that isn't obvious. I'd like to think that would never happen, but, you know, dragons lurk here, etc.

Quoted 6 lines

Though actually, now I think about it, cross-region references can't possibly exist because they'd cause issues with the current code.

So maybe the best solution is actually 2), where we preserve the Boot Services regions if any of the drivers (ESRT, BGRT) request them but free all the others?

Quoted 2 lines

What are the lifetime rules for Boot Services regions on arm*?

We treat all Boot Services regions like Loader Code/Data or free
regions: it is all recorded in memblock as usable memory, and only the regions that are explicitly reserved are protected from further general use.

I am currently looking into the memory attribute table, and the use case is very similar. It would be very useful from our pov to simply memblock_reserve() the region right after having called efi_config_parse_tables(), and actually consume its data when we get around to it later. The ESRT handling is already split down the middle in the same way.

Quoted 48 lines · Ard Biesheuvel

Quoted 47 lines · Matt Fleming

Quoted 46 lines · Ard Biesheuvel

Quoted 44 lines · Matt Fleming

Quoted 37 lines · Ard Biesheuvel

Quoted 30 lines · Matt Fleming

Quoted 18 lines · Ard Biesheuvel

On ARM and arm64, ioremap() and memremap() are not interchangeable like on x86, and the use of ioremap() on ordinary RAM is typically flagged as an error if the memory region being mapped is also covered by the linear mapping, since that would lead to aliases with conflicting cacheability attributes.

Since what we are dealing with is not an I/O region with side effects, using ioremap() here is arguably incorrect anyway, so let's replace it with memremap instead. Also add a missing unmap on the success path, and drop a memblock_remove() call which does not belong here, this far into the boot sequence.

Cc Peter Jones <[email protected]>

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

---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

Quoted 8 lines

(quoted diff) +0 -2

@@ -432,8 +434,6 @@ static int __init esrt_sysfs_init(void)
      if (error)
              goto err_cleanup_list;
 
-     memblock_remove(esrt_data, esrt_data_size);
-
      pr_debug("esrt-sysfs: loaded.\n");
 

Quoted 5 lines

Review static int __init esrt_sysfs_init(void) · line 439

Quoted 1 line

(quoted diff) +0 -0

      return 0;

Shouldn't we be replacing memblock_remove() with free_bootmem_late()? The original ESRT region is still reserved at this point, so we should do our best to release it to the page allocator.

Quoted 3 lines

I'd rather we keep it reserved. That way, the config table entry still points to something valid, which could be useful for kexec(), I think? At least, that is how I intended to handle config tables on ARM ...

If we're going to reserve it why do we need to copy the data out at all in esrt_sysfs_init()?

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Excellent question. I don't think there is any point to doing that.

... Unless the data is contained in an EFI Boot Services region ;-)

Peter?

Yes, it usually is. Is that a problem?

Yes, we free the Boot Services regions before hitting userspace on x86, see efi_free_boot_services(). We do this map/copy/unmap trick in the ACPI BGRT driver for that reason.

The Boot Services regions can be many gigabytes in size, which makes leaving them alone impractical.

For kexec on x86 we simply discard the BGRT table, which isn't the end of the world because who really needs access to the BGRT image on kexec reboot? However, I can see the value of preserving the ESRT.

I guess we've got two options, 1) copy out the chunks of Boot Services regions we're interested in and rewrite the EFI tables to point at these new allocations and free/discard all of the original Boot Services regions or 2) only selectively free the Boot Services regions.

I've always stayed clear of 2) in case there exists cross-region references in the data that isn't obvious. I'd like to think that would never happen, but, you know, dragons lurk here, etc.

Though actually, now I think about it, cross-region references can't possibly exist because they'd cause issues with the current code.

So maybe the best solution is actually 2), where we preserve the Boot Services regions if any of the drivers (ESRT, BGRT) request them but free all the others?

What are the lifetime rules for Boot Services regions on arm*?

So the question I have is: would you rather chop up regions and reserve the space, or allocate a new copy and update the configuration table (after ExitBootServices()) to point to it? The latter makes it pretty easy to do from an API that all these drivers can use, and it makes the kexec case completely transparent.

Up to you.

Quoted 8 lines · Peter Jones

So the question I have is: would you rather chop up regions and reserve the space, or allocate a new copy and update the configuration table (after ExitBootServices()) to point to it? The latter makes it pretty easy to do from an API that all these drivers can use, and it makes the kexec case completely transparent.

Up to you.

I think it makes the most sense to chop up the regions we care about (i.e. the ones we have drivers for) because not only does that save us the effort of copying out the data on every kexec reboot, it also prevents us from leaking each copy since we don't free them at the moment.

Then we just need to maintain a separate list of regions to free in efi_free_boot_services() or have some other way to distinguish between them.

Oh, and save_runtime_map() would need updating to save Boot Services regions too.

Quoted 20 lines · Matt Fleming

Quoted 8 lines · Peter Jones

So the question I have is: would you rather chop up regions and reserve the space, or allocate a new copy and update the configuration table (after ExitBootServices()) to point to it? The latter makes it pretty easy to do from an API that all these drivers can use, and it makes the kexec case completely transparent.

Up to you.

I think it makes the most sense to chop up the regions we care about (i.e. the ones we have drivers for) because not only does that save us the effort of copying out the data on every kexec reboot, it also prevents us from leaking each copy since we don't free them at the moment.

Then we just need to maintain a separate list of regions to free in efi_free_boot_services() or have some other way to distinguish between them.

Quoted 2 lines

Oh, and save_runtime_map() would need updating to save Boot Services regions too.

I have a very rough draft of patches that implement this strategy on the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is curious and wants to take a look. The series will be sent out soon.

Quoted 16 lines · Matt Fleming

Quoted 8 lines · Peter Jones

So the question I have is: would you rather chop up regions and reserve the space, or allocate a new copy and update the configuration table (after ExitBootServices()) to point to it? The latter makes it pretty easy to do from an API that all these drivers can use, and it makes the kexec case completely transparent.

Up to you.

I think it makes the most sense to chop up the regions we care about (i.e. the ones we have drivers for) because not only does that save us the effort of copying out the data on every kexec reboot, it also prevents us from leaking each copy since we don't free them at the moment.

Quoted 6 lines

Then we just need to maintain a separate list of regions to free in efi_free_boot_services() or have some other way to distinguish between them.

Oh, and save_runtime_map() would need updating to save Boot Services regions too.

I have a very rough draft of patches that implement this strategy on the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is curious and wants to take a look. The series will be sent out soon.

I wouldn't necessarily try running them or anything as there's a few things that I know are broken; ia64 build, EFI mixed mode boot, probably arm64 regressions.

The upshot of the patch series is that there's a whole lot of code we can share for manipulating memory maps between all EFI architectures and by fixing this "reserve boot services regions" in a generic way it should mean kexec under EFI on arm64 should be fairly trivial to implement, along with ESRT, etc.

Matt, thanks. One question is for the acpi patches you sent before, do you think they are still necessary as a fix for the bgrt problem and coexist with the "reserve useful boot services" proposal?

I have a very rough draft of patches that implement this strategy on the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is curious and wants to take a look. The series will be sent out soon.

I wouldn't necessarily try running them or anything as there's a few things that I know are broken; ia64 build, EFI mixed mode boot, probably arm64 regressions.

Quoted 7 lines

I have a very rough draft of patches that implement this strategy on the 'experiemntal/efi-memmap' branch in the EFI tree if anyone is curious and wants to take a look. The series will be sent out soon.

I wouldn't necessarily try running them or anything as there's a few things that I know are broken; ia64 build, EFI mixed mode boot, probably arm64 regressions.

Matt, I tried :). I see a build warning about print phys_addr_t without %pa anotation and booting with ioremap error messages about slot not found , and ioremap region leak messages.

Looking forward to your formal posts.

So what is the status here? I know Sai is working on x86 support for the memory attributes table, so it would make sense settle this matter as well.

Quoted 4 lines · Ard Biesheuvel

So what is the status here? I know Sai is working on x86 support for the memory attributes table, so it would make sense settle this matter as well.

This got stuck behind some other work, sorry about that.

Some of this was just merged by Linus (getting rid of the global memmap and switching everyone over to efi.memmap), a large chunk remains however.

I should be able to get those patches mailed out once the merge window closes.

Hi Matt,

Any progress here?

ESRT support is built by default for all architectures that define CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, since efi_esrt_init() was never called. So add the missing call.

Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies on efi.memmap having been assigned and populated completetely, add the missing assignments of efi.memmap and efi.memmap->nr_map.

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

---
 drivers/firmware/efi/arm-init.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/firmware/efi/arm-init.c +3 -0

index 9e15d571b53c..5c5e799bdb50 100644
@@ -197,10 +197,13 @@ void __init efi_init(void)
 	memmap.map_end = memmap.map + params.mmap_size;
 	memmap.desc_size = params.desc_size;
 	memmap.desc_version = params.desc_ver;
+	memmap.nr_map = params.mmap_size / params.desc_size;
+	efi.memmap = &memmap;
 
 	if (uefi_init() < 0)
 		return;
 
+	efi_esrt_init();
 	reserve_regions();
 	early_memunmap(memmap.map, params.mmap_size);
 	memblock_mark_nomap(params.mmap & PAGE_MASK,

Signature · 1 line

2.5.0

ESRT support is built by default for all architectures that define CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, since efi_esrt_init() was never called. So add the missing call.

Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies on efi.memmap having been assigned and populated completetely, add the missing assignments of efi.memmap and efi.memmap->nr_map.

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

---
  drivers/firmware/efi/arm-init.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/firmware/efi/arm-init.c +0 -0

index 9e15d571b53c..5c5e799bdb50 100644

Quoted 26 lines · Ard Biesheuvel

ESRT support is built by default for all architectures that define CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, since efi_esrt_init() was never called. So add the missing call.

Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies on efi.memmap having been assigned and populated completetely, add the missing assignments of efi.memmap and efi.memmap->nr_map.

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

---
  drivers/firmware/efi/arm-init.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/firmware/efi/arm-init.c +2 -0

index 9e15d571b53c..5c5e799bdb50 100644
@@ -197,10 +197,13 @@ void __init efi_init(void)
        memmap.map_end = memmap.map + params.mmap_size;
        memmap.desc_size = params.desc_size;
        memmap.desc_version = params.desc_ver;
+       memmap.nr_map = params.mmap_size / params.desc_size;
+       efi.memmap = &memmap;
        if (uefi_init() < 0)

drivers/firmware/efi/arm-init.c void __init efi_init(void) · line 204

Quoted 2 lines

drivers/firmware/efi/arm-init.c +0 -0

                return;
  +     efi_esrt_init();

This call to efi_esrt_init() is failing because efi.flags does not have EFI_MEMMAP set. This flag gets set at the end of reserve_regions(). I tested moving the set_bit() call from reserve_regions() to just before efi_esrt_init() and also tested moving efi_esrt_init() to just after reserve_regions(). Both of these options worked.

drivers/firmware/efi/arm-init.c void __init efi_init(void) · line 209

Quoted 4 lines

drivers/firmware/efi/arm-init.c +0 -0

        reserve_regions();
        early_memunmap(memmap.map, params.mmap_size);
        memblock_mark_nomap(params.mmap & PAGE_MASK,

Thanks,
Tyler

--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Quoted 24 lines · Ard Biesheuvel

ESRT support is built by default for all architectures that define CONFIG_EFI. However, this support was not wired up yet for ARM/arm64, since efi_esrt_init() was never called. So add the missing call.

Since efi_esrt_init() uses efi_mem_desc_lookup(), which in turn relies on efi.memmap having been assigned and populated completetely, add the missing assignments of efi.memmap and efi.memmap->nr_map.

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

---
  drivers/firmware/efi/arm-init.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/firmware/efi/arm-init.c +2 -0

index 9e15d571b53c..5c5e799bdb50 100644
@@ -197,10 +197,13 @@ void __init efi_init(void)
      memmap.map_end = memmap.map + params.mmap_size;
      memmap.desc_size = params.desc_size;
      memmap.desc_version = params.desc_ver;
+    memmap.nr_map = params.mmap_size / params.desc_size;
+    efi.memmap = &memmap;

drivers/firmware/efi/arm-init.c void __init efi_init(void)

Quoted 3 lines

drivers/firmware/efi/arm-init.c +0 -0

        if (uefi_init() < 0)
          return;
  +    efi_esrt_init();

This call to efi_esrt_init() is failing because efi.flags does not have EFI_MEMMAP set. This flag gets set at the end of reserve_regions(). I tested moving the set_bit() call from reserve_regions() to just before efi_esrt_init() and also tested moving efi_esrt_init() to just after reserve_regions(). Both of these options worked.

Quoted 3 lines

drivers/firmware/efi/arm-init.c +0 -0

      reserve_regions();
      early_memunmap(memmap.map, params.mmap_size);
      memblock_mark_nomap(params.mmap & PAGE_MASK,