Re: [PATCH 06/10] arm64/efi: use UEFI memory map unconditionally if available

[Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Wed, 2014-10-22 at 16:21 +0200, Ard Biesheuvel wrote:
> On systems that boot via UEFI, all memory nodes are deleted from the
> device tree, and instead, the size and location of system RAM is derived
> from the UEFI memory map. This is handled by reserve_regions, which not only
> reserves parts of memory that UEFI declares as reserved, but also installs
> the memblocks that cover the remaining usable memory.
> 
> Currently, reserve_regions() is only called if uefi_init() succeeds.
> However, it does not actually depend on anything that uefi_init() does,
> and not calling reserve_regions() results in a broken boot, so it is
> better to just call it unconditionally.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/kernel/efi.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 51522ab0c6da..4cec21b1ecdd 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -313,8 +313,7 @@ void __init efi_init(void)
>  	memmap.desc_size = params.desc_size;
>  	memmap.desc_version = params.desc_ver;
>  
> -	if (uefi_init() < 0)
> -		return;
> +	WARN_ON(uefi_init() < 0);
>  
>  	reserve_regions();
>  }

It also looks like EFI_BOOT flag will be set even if uefi_init fails.
If uefi_init fails, we only need reserve_regions() for the purpose
of adding memblocks. Otherwise, we end up wasting a lot of memory.
So, something like this would also be needed:

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 03aaa99..5a6e189 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -81,9 +81,6 @@ static int __init uefi_init(void)
 		return -ENOMEM;
 	}
 
-	set_bit(EFI_BOOT, &efi.flags);
-	set_bit(EFI_64BIT, &efi.flags);
-
 	/*
 	 * Verify the EFI Table
 	 */
@@ -109,6 +106,9 @@ static int __init uefi_init(void)
 		efi.systab->hdr.revision >> 16,
 		efi.systab->hdr.revision & 0xffff, vendor);
 
+	set_bit(EFI_BOOT, &efi.flags);
+	set_bit(EFI_64BIT, &efi.flags);
+
 	retval = efi_config_init(NULL);
 	if (retval == 0)
 		set_bit(EFI_CONFIG_TABLES, &efi.flags);
@@ -177,9 +177,10 @@ static __init void reserve_regions(void)
 		if (is_normal_ram(md))
 			early_init_dt_add_memory_arch(paddr, size);
 
-		if (is_reserve_region(md) ||
-		    md->type == EFI_BOOT_SERVICES_CODE ||
-		    md->type == EFI_BOOT_SERVICES_DATA) {
+		if (efi_enabled(EFI_BOOT) &&
+		    (is_reserve_region(md) ||
+		     md->type == EFI_BOOT_SERVICES_CODE ||
+		     md->type == EFI_BOOT_SERVICES_DATA)) {
 			memblock_reserve(paddr, size);
 			if (uefi_debug)
 				pr_cont("*");

> @@ -374,15 +373,13 @@ static int __init arm64_enter_virtual_mode(void)
>  	int count = 0;
>  	unsigned long flags;
>  
> -	if (!efi_enabled(EFI_BOOT)) {
> -		pr_info("EFI services will not be available.\n");
> -		return -1;
> -	}
> +	if (!efi_enabled(EFI_MEMMAP))
> +		return 0;
>  
>  	mapsize = memmap.map_end - memmap.map;
>  	early_memunmap(memmap.map, mapsize);
>  
> -	if (efi_runtime_disabled()) {
> +	if (!efi_enabled(EFI_BOOT) || efi_runtime_disabled()) {
>  		pr_info("EFI runtime services will be disabled.\n");
>  		return -1;
>  	}