| Message ID | 1413997616.2985.74.camel@deneb.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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.
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 --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("*");