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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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