Hi Sai,
Thanks for the patch. Some review comments in-line:
On 12/05/2018 06:03 AM, Sai Praneeth Prakhya wrote:
Presently, in EFI subsystem of kernel, every time kernel allocates memory
for a new EFI memory map, it forgets to free the memory occupied by old EFI
memory map. Hence, introduce efi_memmap_free() that frees up the memory
occupied by an EFI memory map.
Introduce __efi_memmap_unmap(), so that it could be used to unmap an EFI
memory map and have wrappers around it (namely efi_memmap_unmap() and
efi_memmap_unmap_and_free()) to specifically deal with efi.memmap. There
are two variants of wrappers (unmap and free) because there are use cases
where the kernel just needs to unmap the memory map (see efi_init() in arm
and kexec_enter_virtual_mode()) but not free it.
Apart from introducing the above functions, improve the cases where the
kernel decides to turn off EFI runtime services during boot by unmapping
and freeing the EFI memory map rather than just unmapping the EFI memory
map.
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prak...@intel.com>
Suggested-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Borislav Petkov <b...@alien8.de>
Cc: Bhupesh Sharma <bhsha...@redhat.com>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
---
arch/x86/platform/efi/efi.c | 4 +-
arch/x86/platform/efi/quirks.c | 2 +-
drivers/firmware/efi/arm-init.c | 2 +-
drivers/firmware/efi/memmap.c | 72 +++++++++++++++++++++++++++++----
include/linux/efi.h | 1 +
5 files changed, 70 insertions(+), 11 deletions(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e1cb01a22fa8..715601d1c581 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -532,7 +532,7 @@ void __init efi_init(void)
pr_info("No EFI runtime due to 32/64-bit mismatch with
kernel\n");
else {
if (efi_runtime_disabled() || efi_runtime_init()) {
- efi_memmap_unmap();
+ efi_memmap_unmap_and_free();
return;
}
}
@@ -833,7 +833,7 @@ static void __init kexec_enter_virtual_mode(void)
* have been mapped at these virtual addresses.
*/
if (!efi_is_native() || efi_enabled(EFI_OLD_MEMMAP)) {
- efi_memmap_unmap();
+ efi_memmap_unmap_and_free();
clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
return;
}
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 09e811b9da26..ce6dcd40dd6c 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -556,7 +556,7 @@ void __init efi_apply_memmap_quirks(void)
*/
if (!efi_runtime_supported()) {
pr_info("Setup done, disabling due to 32/64-bit mismatch\n");
- efi_memmap_unmap();
+ efi_memmap_unmap_and_free();
}
/* UV2+ BIOS has a fix for this issue. UV1 still needs the quirk. */
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 1a6a77df8a5e..f32ff5c580f6 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -253,7 +253,7 @@ void __init efi_init(void)
efi.memmap.desc_version);
if (uefi_init() < 0) {
- efi_memmap_unmap();
+ efi_memmap_unmap_and_free();
return;
}
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 38b686c67b17..4318a69bdbbf 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -49,6 +49,29 @@ phys_addr_t __init efi_memmap_alloc(unsigned int num_entries)
return __efi_memmap_alloc_early(size);
}
+/**
+ * efi_memmap_free - Free memory pointed by new_memmap.map
+ * @new_memmap: Structure that describes EFI memory map.
+ *
+ * Memory is freed depending on the type of allocation performed.
+ */
+static void __init efi_memmap_free(struct efi_memory_map new_memmap)
^^ Its not very clear what you mean by the term 'new_memmap' here.
Also can we pass a pointer to struct efi_memory_map to this function
rather than passing the entire struct.
+{
+ phys_addr_t start, end;
+ unsigned long size = new_memmap.nr_map * new_memmap.desc_size;
+ unsigned int order = get_order(size);
+
+ start = new_memmap.phys_map;
+ end = start + size;
+ if (new_memmap.late) {
+ __free_pages(pfn_to_page(PHYS_PFN(start)), order);
+ return;
+ }
+
+ if (memblock_free(start, size))
+ pr_err("Failed to free mem from %pa to %pa\n", &start, &end);
+}
+
/**
* __efi_memmap_init - Common code for mapping the EFI memory map
* @data: EFI memory map data
@@ -116,21 +139,56 @@ int __init efi_memmap_init_early(struct
efi_memory_map_data *data)
return __efi_memmap_init(data, false);
}
+/**
+ * __efi_memmap_unmap - Unmap the region pointed by new_memmap.map
+ * @new_memmap: Structure that describes EFI memory map.
+ *
+ * Use to unmap *newly* created EFI memmap and should *not* be used directly to
+ * unmap efi.memmap because "EFI_MEMMAP" flag is not cleared here. Instead, use
+ * efi_memmap_unmap*() variants accordingly. Also, the check for "EFI_MEMMAP"
+ * flag is done in efi_memmap_unmap*() variants because they are the ones who
+ * unmap efi.memmap and not this function.
+ */
+static void __init __efi_memmap_unmap(struct efi_memory_map new_memmap)
+{
+ if (!new_memmap.late) {
+ unsigned long size;
+
+ size = new_memmap.desc_size * new_memmap.nr_map;
+ early_memunmap(new_memmap.map, size);
Nitpick: May be we can avoid the extra local variable size and directly
pass 'new_memmap.desc_size * new_memmap.nr_map' while calling
early_memunmap(). I think the code would still be readable and the
calculation seems self-explanatory.
+ } else {
+ memunmap(new_memmap.map);
+ }
+}
+
+/**
+ * Unmap existing EFI Memory Map i.e. efi.memmap
+ */
void __init efi_memmap_unmap(void)
{
- if (!efi_enabled(EFI_MEMMAP))
+ if (!efi_enabled(EFI_MEMMAP)) {
+ WARN(1, "EFI_MEMMAP is not enabled");
return;
+ }
- if (!efi.memmap.late) {
- unsigned long size;
+ __efi_memmap_unmap(efi.memmap);
+ efi.memmap.map = NULL;
+ clear_bit(EFI_MEMMAP, &efi.flags);
+}
- size = efi.memmap.desc_size * efi.memmap.nr_map;
- early_memunmap(efi.memmap.map, size);
- } else {
- memunmap(efi.memmap.map);
+/**
+ * Unmap and free existing EFI Memory Map i.e. efi.memmap
+ */
+void __init efi_memmap_unmap_and_free(void)
+{
+ if (!efi_enabled(EFI_MEMMAP)) {
+ WARN(1, "EFI_MEMMAP is not enabled");
Nitpick: Do we really need a WARN() here? May be we can do away with the
same.
+ return;
}
+ __efi_memmap_unmap(efi.memmap);
efi.memmap.map = NULL;
+ efi_memmap_free(efi.memmap);
clear_bit(EFI_MEMMAP, &efi.flags);
}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index becd5d76a207..752260e02ae7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1019,6 +1019,7 @@ extern phys_addr_t __init efi_memmap_alloc(unsigned int
num_entries);
extern int __init efi_memmap_init_early(struct efi_memory_map_data *data);
extern int __init efi_memmap_init_late(phys_addr_t addr, unsigned long size);
extern void __init efi_memmap_unmap(void);
+extern void __init efi_memmap_unmap_and_free(void);
extern int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map);
extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
struct range *range);
Thanks,
Bhupesh