Hi, Matt
Thanks for the feedback.
On 02/04/16 at 10:03am, Matt Fleming wrote:
> 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.
I agree with you the less special case the better.
>
> 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.
Consider the original code path, maybe change it to efi_kexec_setup will
be better to remind people? Or something else like a wraper function with
similar name..
>
> 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".
For building ACPI tables we need do it in kernel instead of kexec-tools
because of kexec_file_load for secure boot case so we still need a conditional
code path for kexec..
Also I'm not sure how to rebuild ACPI tables, it is easy or hard. Let me
checking the detail and think more about it.
Thanks a lot
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
