Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325987 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/efi: skip bgrt init for kexec reboot Dave Young <dyoung@redhat.com> - 2016-02-03 22:50 +0100
    Re: [PATCH] x86/efi: skip bgrt init for kexec reboot Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-04 00:00 +0100
      Re: [PATCH] x86/efi: skip bgrt init for kexec reboot Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-04 11:10 +0100
        Re: [PATCH] x86/efi: skip bgrt init for kexec reboot Dave Young <dyoung@redhat.com> - 2016-02-04 12:10 +0100
          Re: [PATCH] x86/efi: skip bgrt init for kexec reboot Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-04 13:00 +0100
            Re: [PATCH] x86/efi: skip bgrt init for kexec reboot Dave Young <dyoung@redhat.com> - 2016-02-05 01:50 +0100

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

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@redhat.com>
> ---
>  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?

Thanks
Dave

[toc] | [next] | [standalone]

#1326096

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@redhat.com>
> > ---
> >  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.

[toc] | [prev] | [next] | [standalone]

#1326551

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@redhat.com>
> > > ---
> > >  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".

[toc] | [prev] | [next] | [standalone]

#1326672

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@redhat.com>
> > > > ---
> > > >  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

[toc] | [prev] | [next] | [standalone]

#1326708

On Thu, 04 Feb, at 07:09:03PM, Dave Young wrote:
> 
> 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..
 
Possibly. I had considered adding a new efi_enabled() bit for
KEXEC_BOOT, but I'm worried that'll just encourage more uses.

The best approach is going to be to see whether we can reduce the uses
of efi_setup and the associated special code. Once we've completed
that exercise, we can think about the best name for this variable.

> 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..

Note that it may not be necessary to build any ACPI tables at all,
provided that things like acpi_get_table() fail gracefully for kexec.
I'm assuming that's the problem that you discovered when writing this
patch.

And yes, I don't expect you can build the ACPI table from userspace,
but it should at least be possible to do it in setup_boot_parameters()
or so when you setup the EFI table pointers (efi.config_tables), etc.
I think that would be a natural home for this feature.

> 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.

[toc] | [prev] | [next] | [standalone]

#1327381

On 02/04/16 at 11:56am, Matt Fleming wrote:
> On Thu, 04 Feb, at 07:09:03PM, Dave Young wrote:
> > 
> > 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..
>  
> Possibly. I had considered adding a new efi_enabled() bit for
> KEXEC_BOOT, but I'm worried that'll just encourage more uses.
> 
> The best approach is going to be to see whether we can reduce the uses
> of efi_setup and the associated special code. Once we've completed
> that exercise, we can think about the best name for this variable.

Ok, thanks.

> 
> > 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..
> 
> Note that it may not be necessary to build any ACPI tables at all,
> provided that things like acpi_get_table() fail gracefully for kexec.
> I'm assuming that's the problem that you discovered when writing this
> patch.
> 
> And yes, I don't expect you can build the ACPI table from userspace,
> but it should at least be possible to do it in setup_boot_parameters()
> or so when you setup the EFI table pointers (efi.config_tables), etc.
> I think that would be a natural home for this feature.

Thing is we support both kexec_load and kexec_file_load, if we do something
in kernel loader we will need do same in userspace kexec-tools as well.

Another way is we probably can retain the boot service areas for kexec
boot, but yes it is another special handling for kexec :(. Is this way
better to you?

Thanks
Dave

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web