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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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