| Message ID | 1460723596-13261-12-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > --- a/xen/arch/x86/efi/Makefile > +++ b/xen/arch/x86/efi/Makefile > @@ -1,14 +1,9 @@ > CFLAGS += -fshort-wchar > > -obj-y += stub.o > - > -create = test -e $(1) || touch -t 199901010000 $(1) > - > efi := y$(shell rm -f disabled) > efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c check.c 2>disabled && echo y)) > efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y)) > -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o))) > +efi := $(if $(efi),$(shell rm disabled)y) > > -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o > - > -stub.o: $(extra-y) > +obj-y := stub.o > +obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o I assume/hope all these adjustments work for all intended cases, but they quite clearly leave stale bits in xen/arch/x86/Rules.mk: Its references to efi/*.o should all go away now afaict. > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void) > } *extra, *extra_head = NULL; > #endif > > + if ( !efi_enabled(EFI_PLATFORM) ) > + return; Arguably such checks would then better be put at the call site, allowing the respective stubs to just BUG(). Also - what's your rule for where to put such efi_enabled() checks? I would have expected them to get added to everything that has a counterpart in stubs.c, but things like efi_get_time() or efi_{halt,reset}_system() don't get any added. If those are unreachable, I'd at least expect respective ASSERT()s to get added there. > --- a/xen/common/efi/runtime.c > +++ b/xen/common/efi/runtime.c > @@ -167,6 +167,9 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) > { > unsigned int i, n; > > + if ( !efi_enabled(EFI_PLATFORM) ) > + return -EOPNOTSUPP; Please do not introduce behavioral differences to the current stub implementations: This and ... > @@ -301,6 +304,9 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op) > EFI_STATUS status = EFI_NOT_STARTED; > int rc = 0; > > + if ( !efi_enabled(EFI_PLATFORM) ) > + return -EOPNOTSUPP; ... this return -ENOSYS there. Jan
On Wed, May 25, 2016 at 01:53:31AM -0600, Jan Beulich wrote: > >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > > --- a/xen/arch/x86/efi/Makefile > > +++ b/xen/arch/x86/efi/Makefile > > @@ -1,14 +1,9 @@ > > CFLAGS += -fshort-wchar > > > > -obj-y += stub.o > > - > > -create = test -e $(1) || touch -t 199901010000 $(1) > > - > > efi := y$(shell rm -f disabled) > > efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c check.c 2>disabled && echo y)) > > efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y)) > > -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o))) > > +efi := $(if $(efi),$(shell rm disabled)y) > > > > -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o > > - > > -stub.o: $(extra-y) > > +obj-y := stub.o > > +obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o > > I assume/hope all these adjustments work for all intended cases, but I have done some tests and it looks that everything works. > they quite clearly leave stale bits in xen/arch/x86/Rules.mk: Its I suppose that you were thinking about xen/arch/x86/Makefile. > references to efi/*.o should all go away now afaict. OK. > > --- a/xen/common/efi/boot.c > > +++ b/xen/common/efi/boot.c > > @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void) > > } *extra, *extra_head = NULL; > > #endif > > > > + if ( !efi_enabled(EFI_PLATFORM) ) > > + return; > > Arguably such checks would then better be put at the call site, > allowing the respective stubs to just BUG(). Ugh... I am confused. Here http://lists.xen.org/archives/html/xen-devel/2015-08/msg01790.html you asked for what is done above. So, what is your final decision? > Also - what's your rule for where to put such efi_enabled() checks? > I would have expected them to get added to everything that has > a counterpart in stubs.c, but things like efi_get_time() or > efi_{halt,reset}_system() don't get any added. If those are > unreachable, I'd at least expect respective ASSERT()s to get added > there. I have added checks to functions which are called from common EFI/BIOS code. > > --- a/xen/common/efi/runtime.c > > +++ b/xen/common/efi/runtime.c > > @@ -167,6 +167,9 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) > > { > > unsigned int i, n; > > > > + if ( !efi_enabled(EFI_PLATFORM) ) > > + return -EOPNOTSUPP; > > Please do not introduce behavioral differences to the current stub > implementations: This and ... > > > @@ -301,6 +304,9 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op) > > EFI_STATUS status = EFI_NOT_STARTED; > > int rc = 0; > > > > + if ( !efi_enabled(EFI_PLATFORM) ) > > + return -EOPNOTSUPP; > > ... this return -ENOSYS there. OK. Daniel
>>> On 25.05.16 at 21:07, <daniel.kiper@oracle.com> wrote: > On Wed, May 25, 2016 at 01:53:31AM -0600, Jan Beulich wrote: >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: >> > --- a/xen/arch/x86/efi/Makefile >> > +++ b/xen/arch/x86/efi/Makefile >> > @@ -1,14 +1,9 @@ >> > CFLAGS += -fshort-wchar >> > >> > -obj-y += stub.o >> > - >> > -create = test -e $(1) || touch -t 199901010000 $(1) >> > - >> > efi := y$(shell rm -f disabled) >> > efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c > check.c 2>disabled && echo y)) >> > efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o >2>disabled && echo y)) >> > -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call > create,boot.init.o); $(call create,runtime.o))) >> > +efi := $(if $(efi),$(shell rm disabled)y) >> > >> > -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o >> > - >> > -stub.o: $(extra-y) >> > +obj-y := stub.o >> > +obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o >> >> I assume/hope all these adjustments work for all intended cases, but > > I have done some tests and it looks that everything works. > >> they quite clearly leave stale bits in xen/arch/x86/Rules.mk: Its > > I suppose that you were thinking about xen/arch/x86/Makefile. Oh, yes, of course. >> references to efi/*.o should all go away now afaict. > > OK. > >> > --- a/xen/common/efi/boot.c >> > +++ b/xen/common/efi/boot.c >> > @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void) >> > } *extra, *extra_head = NULL; >> > #endif >> > >> > + if ( !efi_enabled(EFI_PLATFORM) ) >> > + return; >> >> Arguably such checks would then better be put at the call site, >> allowing the respective stubs to just BUG(). > > Ugh... I am confused. Here > http://lists.xen.org/archives/html/xen-devel/2015-08/msg01790.html > you asked for what is done above. So, what is your final decision? Well, in v2 you didn't alter stubs.c at all. It's that connection which makes me think using that earlier approach might be better. The more that, from a purely abstract pov, it could even allow to remove some or all of stubs.c in a truly non-EFI build, provided we never build with -O0. But in the end, starting the sens with "arguably" I mean to express that this isn't all that important. >> Also - what's your rule for where to put such efi_enabled() checks? >> I would have expected them to get added to everything that has >> a counterpart in stubs.c, but things like efi_get_time() or >> efi_{halt,reset}_system() don't get any added. If those are >> unreachable, I'd at least expect respective ASSERT()s to get added >> there. > > I have added checks to functions which are called from common EFI/BIOS code. And how are the ones I named not called from "common" code? Jan
On Fri, May 27, 2016 at 02:31:52AM -0600, Jan Beulich wrote: > >>> On 25.05.16 at 21:07, <daniel.kiper@oracle.com> wrote: > > On Wed, May 25, 2016 at 01:53:31AM -0600, Jan Beulich wrote: > >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: [...] > >> > --- a/xen/common/efi/boot.c > >> > +++ b/xen/common/efi/boot.c > >> > @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void) > >> > } *extra, *extra_head = NULL; > >> > #endif > >> > > >> > + if ( !efi_enabled(EFI_PLATFORM) ) > >> > + return; > >> > >> Arguably such checks would then better be put at the call site, > >> allowing the respective stubs to just BUG(). > > > > Ugh... I am confused. Here > > http://lists.xen.org/archives/html/xen-devel/2015-08/msg01790.html > > you asked for what is done above. So, what is your final decision? > > Well, in v2 you didn't alter stubs.c at all. It's that connection > which makes me think using that earlier approach might be better. > The more that, from a purely abstract pov, it could even allow to > remove some or all of stubs.c in a truly non-EFI build, provided we > never build with -O0. I am not sure why "provided we never build with -O0". > But in the end, starting the sens with "arguably" I mean to express > that this isn't all that important. OK. > >> Also - what's your rule for where to put such efi_enabled() checks? > >> I would have expected them to get added to everything that has > >> a counterpart in stubs.c, but things like efi_get_time() or > >> efi_{halt,reset}_system() don't get any added. If those are > >> unreachable, I'd at least expect respective ASSERT()s to get added > >> there. > > > > I have added checks to functions which are called from common EFI/BIOS code. > > And how are the ones I named not called from "common" code? efi_get_time() call is protected by "if ( efi_enabled(EFI_PLATFORM) )" in xen/arch/x86/time.c. efi_halt_system() is called from nowhere, so, it can be removed. I will do that. efi_reset_system() call is protected by different means but EFI related. So, all of them are not called from "common" code during runtime on BIOS platforms. Daniel
>>> On 01.06.16 at 17:48, <daniel.kiper@oracle.com> wrote: > On Fri, May 27, 2016 at 02:31:52AM -0600, Jan Beulich wrote: >> >>> On 25.05.16 at 21:07, <daniel.kiper@oracle.com> wrote: >> > On Wed, May 25, 2016 at 01:53:31AM -0600, Jan Beulich wrote: >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: >> >> > --- a/xen/common/efi/boot.c >> >> > +++ b/xen/common/efi/boot.c >> >> > @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void) >> >> > } *extra, *extra_head = NULL; >> >> > #endif >> >> > >> >> > + if ( !efi_enabled(EFI_PLATFORM) ) >> >> > + return; >> >> >> >> Arguably such checks would then better be put at the call site, >> >> allowing the respective stubs to just BUG(). >> > >> > Ugh... I am confused. Here >> > http://lists.xen.org/archives/html/xen-devel/2015-08/msg01790.html >> > you asked for what is done above. So, what is your final decision? >> >> Well, in v2 you didn't alter stubs.c at all. It's that connection >> which makes me think using that earlier approach might be better. >> The more that, from a purely abstract pov, it could even allow to >> remove some or all of stubs.c in a truly non-EFI build, provided we >> never build with -O0. > > I am not sure why "provided we never build with -O0". Because a minimal amount of optimization is necessary for dead calls to actually get eliminated. >> >> Also - what's your rule for where to put such efi_enabled() checks? >> >> I would have expected them to get added to everything that has >> >> a counterpart in stubs.c, but things like efi_get_time() or >> >> efi_{halt,reset}_system() don't get any added. If those are >> >> unreachable, I'd at least expect respective ASSERT()s to get added >> >> there. >> > >> > I have added checks to functions which are called from common EFI/BIOS > code. >> >> And how are the ones I named not called from "common" code? > > efi_get_time() call is protected by "if ( efi_enabled(EFI_PLATFORM) )" > in xen/arch/x86/time.c. efi_halt_system() is called from nowhere, so, > it can be removed. I will do that. Please don't. Instead it should get wired up properly (in machine_halt()). > efi_reset_system() call is protected > by different means but EFI related. Where is that being protected? Nothing prevents anyone to boot with "reboot=efi" on a non-EFI system. That's silly, but shouldn't result in a crash during reboot. Right now its stub is intentionally doing nothing (instead of BUG()ing). Jan
On Wed, Jun 01, 2016 at 09:58:25AM -0600, Jan Beulich wrote: > >>> On 01.06.16 at 17:48, <daniel.kiper@oracle.com> wrote: > > On Fri, May 27, 2016 at 02:31:52AM -0600, Jan Beulich wrote: > >> >>> On 25.05.16 at 21:07, <daniel.kiper@oracle.com> wrote: > >> > On Wed, May 25, 2016 at 01:53:31AM -0600, Jan Beulich wrote: > >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > >> >> > --- a/xen/common/efi/boot.c > >> >> > +++ b/xen/common/efi/boot.c > >> >> > @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void) > >> >> > } *extra, *extra_head = NULL; > >> >> > #endif > >> >> > > >> >> > + if ( !efi_enabled(EFI_PLATFORM) ) > >> >> > + return; > >> >> > >> >> Arguably such checks would then better be put at the call site, > >> >> allowing the respective stubs to just BUG(). > >> > > >> > Ugh... I am confused. Here > >> > http://lists.xen.org/archives/html/xen-devel/2015-08/msg01790.html > >> > you asked for what is done above. So, what is your final decision? > >> > >> Well, in v2 you didn't alter stubs.c at all. It's that connection > >> which makes me think using that earlier approach might be better. > >> The more that, from a purely abstract pov, it could even allow to > >> remove some or all of stubs.c in a truly non-EFI build, provided we > >> never build with -O0. > > > > I am not sure why "provided we never build with -O0". > > Because a minimal amount of optimization is necessary for dead > calls to actually get eliminated. > > >> >> Also - what's your rule for where to put such efi_enabled() checks? > >> >> I would have expected them to get added to everything that has > >> >> a counterpart in stubs.c, but things like efi_get_time() or > >> >> efi_{halt,reset}_system() don't get any added. If those are > >> >> unreachable, I'd at least expect respective ASSERT()s to get added > >> >> there. > >> > > >> > I have added checks to functions which are called from common EFI/BIOS > > code. > >> > >> And how are the ones I named not called from "common" code? > > > > efi_get_time() call is protected by "if ( efi_enabled(EFI_PLATFORM) )" > > in xen/arch/x86/time.c. efi_halt_system() is called from nowhere, so, > > it can be removed. I will do that. > > Please don't. Instead it should get wired up properly (in > machine_halt()). OK, I will try to fix it. Hmmm... Probably efi_halt_system() call was somewhere but it was removed once. It is interesting why? > > efi_reset_system() call is protected > > by different means but EFI related. > > Where is that being protected? Nothing prevents anyone to boot > with "reboot=efi" on a non-EFI system. That's silly, but shouldn't Then it means that on non-EFI platforms we should not accept that, print relevant warning and automatically choose reboot method which make sense. > result in a crash during reboot. Right now its stub is intentionally > doing nothing (instead of BUG()ing). Above should solve that problem. Daniel
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile index 5099430..2a7d3e5 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -1,14 +1,9 @@ CFLAGS += -fshort-wchar -obj-y += stub.o - -create = test -e $(1) || touch -t 199901010000 $(1) - efi := y$(shell rm -f disabled) efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c check.c 2>disabled && echo y)) efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y)) -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o))) +efi := $(if $(efi),$(shell rm disabled)y) -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o - -stub.o: $(extra-y) +obj-y := stub.o +obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index f006575..d10c0ab 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void) } *extra, *extra_head = NULL; #endif + if ( !efi_enabled(EFI_PLATFORM) ) + return; + printk(XENLOG_INFO "EFI memory map:%s\n", map_bs ? " (mapping BootServices)" : ""); for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c index aa064e7..3eb21c1 100644 --- a/xen/common/efi/runtime.c +++ b/xen/common/efi/runtime.c @@ -167,6 +167,9 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) { unsigned int i, n; + if ( !efi_enabled(EFI_PLATFORM) ) + return -EOPNOTSUPP; + switch ( idx ) { case XEN_FW_EFI_VERSION: @@ -301,6 +304,9 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op) EFI_STATUS status = EFI_NOT_STARTED; int rc = 0; + if ( !efi_enabled(EFI_PLATFORM) ) + return -EOPNOTSUPP; + switch ( op->function ) { case XEN_EFI_get_time:
Build xen.gz with EFI code. We need this to support multiboot2 protocol on EFI platforms. If we wish to load non-ELF file using multiboot (v1) or multiboot2 then it must contain "linear" (or "flat") representation of code and data. This is requirement of both boot protocols. Currently, PE file contains many sections which are not "linear" (one after another without any holes) or even do not have representation in a file (e.g. BSS). From EFI point of view everything is OK and works. However, this file layout cannot be properly interpreted by multiboot protocols family. In theory there is a chance that we could build proper PE file (from multiboot protocols POV) using current build system. However, it means that xen.efi further diverge from Xen ELF file (in terms of contents and build method). On the other hand ELF has all needed properties. So, it means that this is good starting point for further development. Additionally, I think that this is also good starting point for further xen.efi code and build optimizations. It looks that there is a chance that finally we can generate xen.efi directly from Xen ELF using just simple objcopy or other tool. This way we will have one Xen binary which can be loaded by three boot protocols: EFI native loader, multiboot (v1) and multiboot2. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v3 - suggestions/fixes: - check for EFI platform in EFI code (suggested by Jan Beulich), - fix Makefiles (suggested by Jan Beulich), - improve commit message (suggested by Jan Beulich). v2 - suggestions/fixes: - build EFI code only if it is supported in a given build environment (suggested by Jan Beulich). --- xen/arch/x86/efi/Makefile | 11 +++-------- xen/common/efi/boot.c | 3 +++ xen/common/efi/runtime.c | 6 ++++++ 3 files changed, 12 insertions(+), 8 deletions(-)