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

[Daniel Kiper]

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?

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

Daniel