On 18 February 2016 at 14:28, Matt Fleming [off-list ref] wrote:

On Thu, 18 Feb, at 01:16:05PM, Ard Biesheuvel wrote:

quoted

On 18 February 2016 at 11:44, Matt Fleming [off-list ref] wrote:

quoted

On Mon, 15 Feb, at 12:32:32PM, Ard Biesheuvel wrote:

quoted

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 <pjones@redhat.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
 drivers/firmware/efi/esrt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[...]

quoted

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

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.