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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote:
> >>> On 25.05.16 at 18:45, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
> >> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> 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?
>
> Because there are no accesses to the structure in non-EFI builds?
> Even if it's just a small table, I'm generally opposed to adding dead
> code or data. I simply do not like the attitude of "memory is cheap"
> these days. Following that model leads to quite a bit of useless
> bloat. Plus no matter whether memory is cheap, cache and TLB
> bandwidth are precious, and both may get pressure added by such
> dead elements.
>
> >> > --- 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?
>
> A separate file seems to be overkill. Just move it to some other
> existing file; I'm sure some sensible place can be found.

This solution is not perfect, however, I cannot find better place for
efi struct. If you have one then drop me a line.

Daniel

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