Re: [PATCH v3 11/16] efi: build xen.gz with EFI code

[Daniel Kiper <daniel.kiper@oracle.com>]

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

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