RE: [PATCH V2 1/3] efi: Introduce efi_memmap_free() and efi_memmap_unmap_and_free()
> > +/**
> > + * 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.
Sure! I will change it.
> > +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.
Yes, we could do that but that would make early_memunmap() exceed 80 characters
limit.
Personally, I feel single line code is more readable when compared to split
lines
and this local variable would anyways be gone once the function returns.
> > +/**
> > + * 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.
My thought behind adding a WARN() is to let the developer know that he's using
the
API in wrong scenario i.e. trying to unmap/free memory map when it's not
present.
If we just return, the developer might think that his code isn't buggy when he
tries to
unmap an non-existent EFI memory map.
Regards,
Sai
RE: [PATCH V2 1/3] efi: Introduce efi_memmap_free() and efi_memmap_unmap_and_free()
> > +static void __init efi_memmap_free(struct efi_memory_map new_memmap)
> > +{
> > + 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", , );
>
> Why is this rather large structure passed in by value and not by reference?
My bad.. I will change it to pass by reference.
> Also, 'new_memmap' naming is confusing - by this time this is a rather old
> memmap table we are going to free, right? So naming it 'memmap' would be
> fine, right?
Yes, that should be fine. I will change it.
Regards,
Sai
Re: [PATCH V2 1/3] efi: Introduce efi_memmap_free() and efi_memmap_unmap_and_free()
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
Suggested-by: Ard Biesheuvel
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: Bhupesh Sharma
Cc: Ard Biesheuvel
---
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, );
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", , );
+}
+
/**
* __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
Re: [PATCH V2 1/3] efi: Introduce efi_memmap_free() and efi_memmap_unmap_and_free()
* Sai Praneeth Prakhya wrote:
> +/**
> + * 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)
> +{
> + 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", , );
Why is this rather large structure passed in by value and not by
reference?
Also, 'new_memmap' naming is confusing - by this time this is a rather
old memmap table we are going to free, right? So naming it 'memmap' would
be fine, right?
In a similar vein the 'old_memmap' function parameter in
efi_memmap_insert() should probably be renamed to 'memmap' too, in a
separate patch. Also, I find efi_memmap_insert() rather confusing to read
- would it be possible to clean it up? Here's the various problems I can
see:
- 'new' is rather confusingly named, and because it's a void its exact
purpose and role is not clear.
- m_start/end_attr could be renamed to mem_start/end/attr - there's very
little point in abbreviating that.
- All around the names do not help readability. Why is the new range
being inserted just named 'mem'? Why is the descriptor table we insert
it into named old_memmap but the actual *new* descriptor table passed
in via an opaque, misleadingly named void pointer named 'buf'?
etc.
This function needs a lot of help to make it reviewable easily, before we
complicate the EFI memory descriptor table code with freeing logic...
Thanks,
Ingo
