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

From: "Jan Beulich" <JBeulich@suse.com>
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Juergen Gross <JGross@suse.com>,
	stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	qiaowei.ren@intel.com, richard.l.maliszewski@intel.com,
	gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v3 11/16] efi: build xen.gz with EFI code
Date: Wed, 25 May 2016 01:53:31 -0600	[thread overview]
Message-ID: <5745761B02000078000EE8C8@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1460723596-13261-12-git-send-email-daniel.kiper@oracle.com>

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

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