Re: [Xen-devel] [PATCH v3 11/16] efi: build xen.gz with EFI code

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Jun 01, 2016 at 09:58:25AM -0600, Jan Beulich wrote:
> >>> On 01.06.16 at 17:48, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Fri, May 27, 2016 at 02:31:52AM -0600, Jan Beulich wrote:
> >> >>> On 25.05.16 at 21:07, <daniel.kiper@xxxxxxxxxx> wrote:
> >> > On Wed, May 25, 2016 at 01:53:31AM -0600, Jan Beulich wrote:
> >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:
> >> >> > --- a/xen/common/efi/boot.c
> >> >> > +++ b/xen/common/efi/boot.c
> >> >> > @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void)
> >> >> >      } *extra, *extra_head = NULL;
> >> >> >  #endif
> >> >> >
> >> >> > +    if ( !efi_enabled(EFI_PLATFORM) )
> >> >> > +        return;
> >> >>
> >> >> Arguably such checks would then better be put at the call site,
> >> >> allowing the respective stubs to just BUG().
> >> >
> >> > Ugh... I am confused. Here
> >> > http://lists.xen.org/archives/html/xen-devel/2015-08/msg01790.html
> >> > you asked for what is done above. So, what is your final decision?
> >>
> >> Well, in v2 you didn't alter stubs.c at all. It's that connection
> >> which makes me think using that earlier approach might be better.
> >> The more that, from a purely abstract pov, it could even allow to
> >> remove some or all of stubs.c in a truly non-EFI build, provided we
> >> never build with -O0.
> >
> > I am not sure why "provided we never build with -O0".
>
> Because a minimal amount of optimization is necessary for dead
> calls to actually get eliminated.
>
> >> >> Also - what's your rule for where to put such efi_enabled() checks?
> >> >> I would have expected them to get added to everything that has
> >> >> a counterpart in stubs.c, but things like efi_get_time() or
> >> >> efi_{halt,reset}_system() don't get any added. If those are
> >> >> unreachable, I'd at least expect respective ASSERT()s to get added
> >> >> there.
> >> >
> >> > I have added checks to functions which are called from common EFI/BIOS
> > code.
> >>
> >> And how are the ones I named not called from "common" code?
> >
> > efi_get_time() call is protected by "if ( efi_enabled(EFI_PLATFORM) )"
> > in xen/arch/x86/time.c. efi_halt_system() is called from nowhere, so,
> > it can be removed. I will do that.
>
> Please don't. Instead it should get wired up properly (in
> machine_halt()).

OK, I will try to fix it. Hmmm... Probably efi_halt_system() call was
somewhere but it was removed once. It is interesting why?

> > efi_reset_system() call is protected
> > by different means but EFI related.
>
> Where is that being protected? Nothing prevents anyone to boot
> with "reboot=efi" on a non-EFI system. That's silly, but shouldn't

Then it means that on non-EFI platforms we should not accept that, print
relevant warning and automatically choose reboot method which make sense.

> result in a crash during reboot. Right now its stub is intentionally
> doing nothing (instead of BUG()ing).

Above should solve that problem.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel