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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> 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
result in a crash during reboot. Right now its stub is intentionally
doing nothing (instead of BUG()ing).

Jan


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