| Message ID | 1470438282-4226-11-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> 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@oracle.com> > --- > 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
On Wed, Aug 17, 2016 at 09:56:39AM -0600, Jan Beulich wrote: > >>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> 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@oracle.com> > > --- > > 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 Here it is (I list just first email from thread in a given month): https://lists.xen.org/archives/html/xen-devel/2016-04/msg02186.html https://lists.xen.org/archives/html/xen-devel/2016-05/msg02659.html https://lists.xen.org/archives/html/xen-devel/2016-06/msg00124.html https://lists.xen.org/archives/html/xen-devel/2016-07/msg00530.html > 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. I see two solutions for this issue: - define efi struct members conditionally; this requires also some #ifs sprinkled over Xen code (not very nice) or other substantial changes, - replace efi.flags with efi_flags and leave existing code as is. What is your choice? Personally I prefer existing patch (maybe with minimal changes suggested by you). Daniel
>>> On 18.08.16 at 12:17, <daniel.kiper@oracle.com> wrote: > On Wed, Aug 17, 2016 at 09:56:39AM -0600, Jan Beulich wrote: >> >>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> 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@oracle.com> >> > --- >> > 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 > > Here it is (I list just first email from thread in a given month): > https://lists.xen.org/archives/html/xen-devel/2016-04/msg02186.html > https://lists.xen.org/archives/html/xen-devel/2016-05/msg02659.html > https://lists.xen.org/archives/html/xen-devel/2016-06/msg00124.html > https://lists.xen.org/archives/html/xen-devel/2016-07/msg00530.html > >> 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. > > I see two solutions for this issue: > - define efi struct members conditionally; this requires also > some #ifs sprinkled over Xen code (not very nice) or other > substantial changes, That won't work, afaict, for the current model of building xen.efi and xen.gz from mostly the same object files. > - replace efi.flags with efi_flags and leave existing code as is. > > What is your choice? Hence the latter would be my choice, unless you can give good arguments in favor of another functioning solution. Jan
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 0970299..b1b15b7 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -265,8 +265,6 @@ SECTIONS .pad : { . = ALIGN(MB(16)); } :text -#else - efi = .; #endif /* Sections to be discarded */ diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c index c256814..82c45bc 100644 --- a/xen/common/efi/runtime.c +++ b/xen/common/efi/runtime.c @@ -43,14 +43,6 @@ UINT64 __read_mostly efi_boot_max_var_store_size; UINT64 __read_mostly efi_boot_remain_var_store_size; UINT64 __read_mostly efi_boot_max_var_size; -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, -}; - const struct efi_pci_rom *__read_mostly efi_pci_roms; #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */ diff --git a/xen/common/lib.c b/xen/common/lib.c index ae0bbb3..32f21e2 100644 --- 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> @@ -32,6 +32,14 @@ const unsigned char _ctype[] = { _L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L, /* 224-239 */ _L,_L,_L,_L,_L,_L,_L,_P,_L,_L,_L,_L,_L,_L,_L,_L}; /* 240-255 */ +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, +}; + /* * A couple of 64 bit operations ported from FreeBSD. * The code within the '#if BITS_PER_LONG == 32' block below, and no other
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@oracle.com> --- 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), - improve commit message (suggested by Jan Beulich). --- xen/arch/x86/xen.lds.S | 2 -- xen/common/efi/runtime.c | 8 -------- xen/common/lib.c | 10 +++++++++- 3 files changed, 9 insertions(+), 11 deletions(-)