Patchwork [V2,1/3] efi: Introduce efi_memmap_free() and efi_memmap_unmap_and_free()

login
register
mail settings
Submitter Sai Praneeth Prakhya
Date Dec. 5, 2018, 12:33 a.m.
Message ID <20181205003357.31077-2-sai.praneeth.prakhya@intel.com>
Download mbox | patch
Permalink /patch/672487/
State New
Headers show

Comments

Sai Praneeth Prakhya - Dec. 5, 2018, 12:33 a.m.
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.prakhya@intel.com>
Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@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(-)
Ingo Molnar - Dec. 5, 2018, 7:22 a.m.
* Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> 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", &start, &end);

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
Bhupesh Sharma - Dec. 5, 2018, 9:11 a.m.
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.prakhya@intel.com>
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Bhupesh Sharma <bhsharma@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@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
Sai Praneeth Prakhya - Dec. 5, 2018, 6:36 p.m.
> > +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", &start, &end);
> 
> 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
Sai Praneeth Prakhya - Dec. 5, 2018, 7:21 p.m.
> > +/**

> > + * 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

Patch

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)
+{
+	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);
+	} 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");
+		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);