Re: [Xen-devel] [PATCH v4 10/19] efi: move efi struct initialization to xen/common/lib.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.08.16 at 01:04, <daniel.kiper@xxxxxxxxxx> wrote:
> A subsequent 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,
> move efi struct initialization to xen/common/lib.c and remove
> efi symbol from ld script.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> ---
> v4 - suggestions/fixes:
>    - move efi struct initialization to xen/common/lib.c
>      and drop one from xen/arch/x86/efi/stub.c
>      (suggested by Jan Beulich),

I recall I didn't like where you placed it last time round. I've just tried
to locate the old thread, but going back a whole year in the list archives
I was not able to find a mail with the title containing "move efi". Hence I
can only say what I think now, without reference to earlier remarks:
The struct currently isn't overly large, but I still don't see why non-EFI
builds need to include it instead of just the flags variable you mean to
introduce subsequently. And it's even less obvious what use it is on
platforms not even supporting EFI, i.e. ARM32.

> --- a/xen/common/lib.c
> +++ b/xen/common/lib.c
> @@ -1,4 +1,4 @@
> -
> +#include <xen/efi.h>
>  #include <xen/ctype.h>
>  #include <xen/lib.h>
>  #include <xen/types.h>

At least the visible section here is nicely sorted alphabetically, and I
don't think xen/efi.h absolutely needs to go first.

Jan


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