Re: [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Daniel Kiper <daniel.kiper@oracle.com>]

On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote:
> > 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.
>
> I do not view this as a valid reason for the change.

Why?

> > Additionally, following patch adds efi struct flags member which
> > is used during runtime to differentiate between legacy BIOS and
> > EFI platforms and multiboot2 and EFI native loader. So, efi symbol
> > have to proper representation in ELF and PE Xen image. Hence,
> > define efi struct in xen/arch/x86/efi/stub.c and remove efi
> > symbol from ld script.
>
> Only this one is, afaic. The only request here would be to replace
> "following" by e.g. "a subsequent", to make the description
> independent of whether the two patches get committed together.

OK.

> > --- a/xen/arch/x86/efi/stub.c
> > +++ b/xen/arch/x86/efi/stub.c
> > @@ -8,6 +8,14 @@
> >  const bool_t efi_enabled = 0;
> >  #endif
> >
> > +struct efi __read_mostly efi = {
> > +	.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
> > +};
>
> I don't view duplicating this here as a good approach - you'd better
> move the existing instance elsewhere. If this was a temporary thing
> (until a later patch), it might be acceptable, but since building without
> EFI support will need to remain an option (for people using older tool
> chains), I don't expect a later patch to remove this.

Do you think about separate C file which should contain efi struct
and should be included in stub.c and runtime.c? Or anything else?

Daniel

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