Re: [PATCH v2 09/23] efi: create efi_enabled()

[Jan Beulich]

>>> On 22.08.15 at 14:33, <address@hidden> wrote:
> On Thu, Aug 20, 2015 at 09:18:17AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, <address@hidden> wrote:
>> > --- a/xen/arch/x86/efi/stub.c
>> > +++ b/xen/arch/x86/efi/stub.c
>> > @@ -4,9 +4,14 @@
>> >  #include <xen/lib.h>
>> >  #include <asm/page.h>
>> >
>> > -#ifndef efi_enabled
>> > -const bool_t efi_enabled = 0;
>> > -#endif
>> > +struct efi __read_mostly efi = {
>> > +  .flags   = 0, /* Initialized later. */
>> > +  .acpi    = EFI_INVALID_TABLE_ADDR,
>> > +  .acpi20  = EFI_INVALID_TABLE_ADDR,
>> > +  .mps     = EFI_INVALID_TABLE_ADDR,
>> > +  .smbios  = EFI_INVALID_TABLE_ADDR,
>> > +  .smbios3 = EFI_INVALID_TABLE_ADDR
>> > +};
>>
>> How is this change related to the subject of the patch?
> 
> I need to add this struct because...
> 
>> > --- a/xen/arch/x86/xen.lds.S
>> > +++ b/xen/arch/x86/xen.lds.S
>> > @@ -191,8 +191,6 @@ SECTIONS
>> >    .pad : {
>> >      . = ALIGN(MB(16));
>> >    } :text
>> > -#else
>> > -  efi = .;
>> >  #endif
>>
>> Same here.
> 
> ...this creates efi symbol to just satisfy linker and I am removing it.
> However, existing solution does not allocate space for this symbol and
> any references to acpi20, etc. does not make sense. As I saw any efi.*
> references are protected by relevant ifs but we should not do that because
> it makes code very fragile. If somebody does not know how efi symbol is
> created he/she may assume that it always represent valid structure and
> do invalid references somewhere. So, I still think that stub.c should
> define efi struct properly even if we assume that flags should not be
> there. However, I agree that this could be separate patch.
> 
> By the way why did you choose so strange way to satisfy liker needs?

To me there's nothing strange about it: I want a symbol that
occupies no space in memory.

>> > --- a/xen/common/efi/boot.c
>> > +++ b/xen/common/efi/boot.c
>> > @@ -717,6 +717,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
>> > *SystemTable)
>> >      char *option_str;
>> >      bool_t use_cfg_file;
>> >
>> > +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
>> > +    set_bit(EFI_PLATFORM, &efi.flags);
>> > +#endif
>>
>> Just for this to work? I don't see the need for all the pointers in
>> the stub case - why can't this be a separate variable? We don't
> 
> Could be but if we create struct with so generic name like just simple
> efi it suggest that this is good place to put flags there. If it is not
> how to call it? efi_flags? Or maybe we should rename efi to efi_tables
> too. Then everything will be clear.

I agree that this may be matter of taste, but to me the current
naming looks quite fine. And yes, efi_flags of efi_state would be
a fine name. In general I wouldn't even mind it to be a field in
the structure, if only that resulted in the _full_ structure to be
allocated even in the no-EFI build case. I admit though that with
the goal of always building EFI code (unless the tool chain
doesn't support doing so) this becomes less of an issue; otoh us
probably wanting some Kconfig-like mechanism sooner or later to
{en,dis}able certain features would call for this to remain separable.
And yes, at that point it could be done by #ifdef-ing out everything
by the flags member.

So based on the above I'm withdrawing my implied objection, but
please make sure you write better patch descriptions explaining
what is done and _why_.

Jan