diff mbox

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

Message ID 1413997616.2985.74.camel@deneb.redhat.com
State New
Headers show

Commit Message

Mark Salter Oct. 22, 2014, 5:06 p.m. UTC 
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@linaro.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:


> @@ -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;
>  	}

Comments

Ard Biesheuvel Oct. 22, 2014, 5:20 p.m. UTC | #1 
On 22 October 2014 19:06, Mark Salter <msalter@redhat.com> wrote:
> 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@linaro.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.

Indeed. But perhaps it would be cleaner to add the memblocks in a
separate function that gets called before uefi_init(), let
reserve_regions() do just what it name implies, and retain the above
hunk so that the reserve_regions() call is only performed if UEFI
initialized correctly.
Mark Salter Oct. 22, 2014, 5:29 p.m. UTC | #2 
On Wed, 2014-10-22 at 19:20 +0200, Ard Biesheuvel wrote:
> On 22 October 2014 19:06, Mark Salter <msalter@redhat.com> wrote:
> > 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@linaro.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.
> 
> Indeed. But perhaps it would be cleaner to add the memblocks in a
> separate function that gets called before uefi_init(), let
> reserve_regions() do just what it name implies, and retain the above
> hunk so that the reserve_regions() call is only performed if UEFI
> initialized correctly.
> 

That works for me.
Mark Rutland Oct. 23, 2014, 3:54 p.m. UTC | #3 
On Wed, Oct 22, 2014 at 06:06:56PM +0100, Mark Salter wrote:
> 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@linaro.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.

What memory are we wasting in that case? Surely the only items that we
could choose to throw away if we failed uefi_init are
EFI_ACPI_RECLAIM_MEMORY and EFI_MEMORY_RUNTIME?

We might want to keep those around so we can kexec into a kernel where
we can make use of them. Surely they shouldn't take up a significant
proportion of the available memory?

Thanks,
Mark.
Mark Salter Oct. 23, 2014, 4:19 p.m. UTC | #4 
On Thu, 2014-10-23 at 16:54 +0100, Mark Rutland wrote:
> On Wed, Oct 22, 2014 at 06:06:56PM +0100, Mark Salter wrote:
> > 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@linaro.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.
> 
> What memory are we wasting in that case? Surely the only items that we
> could choose to throw away if we failed uefi_init are
> EFI_ACPI_RECLAIM_MEMORY and EFI_MEMORY_RUNTIME?
> 
> We might want to keep those around so we can kexec into a kernel where
> we can make use of them. Surely they shouldn't take up a significant
> proportion of the available memory?

In addition, reserve_regions() also reserves BOOT_SERVICES (which gets
freed up later after set_virtual_address_map(). That won't happen if
uefi_init() fails. In any case, if uefi_init() fails, we won't be able
to find the ACPI tables kexec kernel won't be able to either. The
BOOT_SERVICES stuff is the big chunk. There was some discussion of
not reserving that but no one has gotten around to writing the patch
to clean out the code that does it.
Ard Biesheuvel Oct. 23, 2014, 6:41 p.m. UTC | #5 
On 23 October 2014 18:19, Mark Salter <msalter@redhat.com> wrote:
> On Thu, 2014-10-23 at 16:54 +0100, Mark Rutland wrote:
>> On Wed, Oct 22, 2014 at 06:06:56PM +0100, Mark Salter wrote:
>> > 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@linaro.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.
>>
>> What memory are we wasting in that case? Surely the only items that we
>> could choose to throw away if we failed uefi_init are
>> EFI_ACPI_RECLAIM_MEMORY and EFI_MEMORY_RUNTIME?
>>
>> We might want to keep those around so we can kexec into a kernel where
>> we can make use of them. Surely they shouldn't take up a significant
>> proportion of the available memory?
>
> In addition, reserve_regions() also reserves BOOT_SERVICES (which gets
> freed up later after set_virtual_address_map(). That won't happen if
> uefi_init() fails. In any case, if uefi_init() fails, we won't be able
> to find the ACPI tables kexec kernel won't be able to either. The
> BOOT_SERVICES stuff is the big chunk. There was some discussion of
> not reserving that but no one has gotten around to writing the patch
> to clean out the code that does it.
>

I am working on some patches to move the call to
SetVirtualAddressMap() to the stub, allowing us to handle the first
boot and subsequent kexec boots uniformly. This also means there will
be no need to reserve the BOOT_SERVICES regions anymore.

Perhaps we should drop this patch from this series, and I can revisit
it as part of the kexec series. That can wait for 3.20, I suppose, as
I don't see kexec being merged for 3.19
diff mbox

Patch

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("*");