Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

[Matt Fleming]

On Wed, 03 Feb, at 10:53:33PM, Matt Fleming wrote:
> On Thu, 04 Feb, at 05:42:00AM, Dave Young wrote:
> > 
> > On 01/27/16 at 07:20pm, Dave Young wrote:
> > > For kexec reboot the bgrt image address could contains random data because
> > > we have freed boot service areas in 1st kernel boot phase. One possible
> > > result is kmalloc fail in efi_bgrt_init due to large random image size.
> > > 
> > > So change efi_late_init to avoid efi_bgrt_init in case kexec boot.
> > > 
> > > Signed-off-by: Dave Young <dyoung@xxxxxxxxxx>
> > > ---
> > >  arch/x86/platform/efi/efi.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > --- linux-x86.orig/arch/x86/platform/efi/efi.c
> > > +++ linux-x86/arch/x86/platform/efi/efi.c
> > > @@ -531,7 +531,8 @@ void __init efi_init(void)
> > >  
> > >  void __init efi_late_init(void)
> > >  {
> > > -	efi_bgrt_init();
> > > +	if (!efi_setup)
> > > +		efi_bgrt_init();
> > >  }
> > >  
> > >  void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
> > 
> > Matt, opinions about this patch?
> 
> Yeah, I'm not happy seeing efi_setup escaping into even more places,
> nor am I happy to see more code paths introduced where kexec boot is
> special-cased.
> 
> I'll reply with more details tomorrow.

OK, let me expand upon that rather terse feedback. 

This patch highlights a general problem I see in the EFI code which is
that we're continuously increasing the number of execution paths
through the boot code. This makes it increasingly difficult to modify
the code without introducing bugs and regressions.

I was bitten by this recently with the EFI separate page table rework,
which led to commit 753b11ef8e92 ("x86/efi: Setup separate EFI page
tables in kexec paths"), i.e I forgot to update the special kexec
virtual mapping function.

We should be reducing the use of 'efi_setup', not adding more uses.

As an aside, I've always had a problem with using 'efi_setup' to
indicate when we've been booted via kexec. If a developer with no
prior knowledge reads those if conditions they are going to have zero
clue what the code means. 

Now, specifically for the issue you've raised, would it not make more
sense for kexec to build its own ACPI tables and omit those entries
that are not valid, e.g. BGRT? I can imagine that the BGRT driver
won't be the only driver with this problem. Let's re-use the existing
error paths that handle missing/invalid tables.

Fundamentally I don't think there should be a discernible difference
between "Booted via kexec" and "That ACPI table does not exist".