Re: [XEN PATCH v2] build: Fix x86 out-of-tree build without EFI

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Anthony,

> On 17 Aug 2022, at 10:15, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
> 
> We can't have a source file with the same name that exist in both the
> common code and in the arch specific code for efi/. This can lead to
> comfusion in make and it can pick up the wrong source file. This issue
> lead to a failure to build a pv-shim for x86 out-of-tree, as this is
> one example of an x86 build using the efi/stub.c.
> 
> The issue is that in out-of-tree, make might find x86/efi/stub.c via
> VPATH, but as the target needs to be rebuilt due to FORCE, make
> actually avoid changing the source tree and rebuilt the target with
> VPATH ignored, so $@ lead to the build tree where "stub.c" dosen't
> exist yet so a link is made to "common/stub.c".
> 
> Rework the new common/stub.c file to have a different name than the
> already existing one, by renaming the existing one. We will take
> example of efi/boot.c and have the common stub.c include a per-arch
> stub.h. This at least avoid the need to expose to Arm both alias
> efi_compat_get_info and efi_compat_runtime_call.
> 
> Avoid using $(EFIOBJ-y) as an alias for $(clean-files), add
> "stub.c" directly to $(clean-files).
> 
> Also update .gitignore as this was also missing from the original
> patch.
> 
> Fixes: 7f96859b0d00 ("xen: reuse x86 EFI stub functions for Arm")
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

I do not really like the empty header but I have no better solution so:
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Also I did some compilation runs and it works.

Cheers
Bertrand

> ---
> 
> Notes:
>    v2:
>    - instead of renaming common/efi/stub.c to common_stub.c; we rename
>      arch/*/efi/stub.c to stub.h and include it from common/stub.c
>    - update .gitignore
> 
>    CC: Jan Beulich <jbeulich@xxxxxxxx>
>    CC: Wei Chen <wei.chen@xxxxxxx>
> 
> xen/arch/arm/efi/Makefile           | 4 ----
> xen/common/efi/efi-common.mk        | 4 ++--
> xen/arch/arm/efi/stub.h             | 4 ++++
> xen/arch/x86/efi/{stub.c => stub.h} | 5 ++++-
> xen/common/efi/stub.c               | 5 +++++
> .gitignore                          | 1 +
> 6 files changed, 16 insertions(+), 7 deletions(-)
> create mode 100644 xen/arch/arm/efi/stub.h
> rename xen/arch/x86/efi/{stub.c => stub.h} (93%)
> 
> diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile
> index bd954a3b2d..ff1bcd6c50 100644
> --- a/xen/arch/arm/efi/Makefile
> +++ b/xen/arch/arm/efi/Makefile
> @@ -4,10 +4,6 @@ ifeq ($(CONFIG_ARM_EFI),y)
> obj-y += $(EFIOBJ-y)
> obj-$(CONFIG_ACPI) +=  efi-dom0.init.o
> else
> -# Add stub.o to EFIOBJ-y to re-use the clean-files in
> -# efi-common.mk. Otherwise the link of stub.c in arm/efi
> -# will not be cleaned in "make clean".
> -EFIOBJ-y += stub.o
> obj-y += stub.o
> 
> $(obj)/stub.o: CFLAGS-y += -fno-short-wchar
> diff --git a/xen/common/efi/efi-common.mk b/xen/common/efi/efi-common.mk
> index ec2c34f198..950f564575 100644
> --- a/xen/common/efi/efi-common.mk
> +++ b/xen/common/efi/efi-common.mk
> @@ -9,9 +9,9 @@ CFLAGS-y += -iquote $(srcdir)
> # e.g.: It transforms "dir/foo/bar" into successively
> #       "dir foo bar", ".. .. ..", "../../.."
> $(obj)/%.c: $(srctree)/common/efi/%.c FORCE
> -     $(Q)test -f $@ || \
> -         ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, 
> ,$(obj))))/source/common/efi/$(<F) $@
> +     $(Q)ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, 
> ,$(obj))))/source/common/efi/$(<F) $@
> 
> clean-files += $(patsubst %.o, %.c, $(EFIOBJ-y:.init.o=.o) $(EFIOBJ-))
> +clean-files += stub.c
> 
> .PRECIOUS: $(obj)/%.c
> diff --git a/xen/arch/arm/efi/stub.h b/xen/arch/arm/efi/stub.h
> new file mode 100644
> index 0000000000..b0a9b03e59
> --- /dev/null
> +++ b/xen/arch/arm/efi/stub.h
> @@ -0,0 +1,4 @@
> +/*
> + * Architecture specific implementation for EFI stub code.  This file
> + * is intended to be included by common/efi/stub.c _only_.
> + */
> diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.h
> similarity index 93%
> rename from xen/arch/x86/efi/stub.c
> rename to xen/arch/x86/efi/stub.h
> index f2365bc041..9d2845b833 100644
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.h
> @@ -1,3 +1,7 @@
> +/*
> + * Architecture specific implementation for EFI stub code.  This file
> + * is intended to be included by common/efi/stub.c _only_.
> + */
> #include <xen/efi.h>
> #include <xen/init.h>
> #include <asm/asm_defns.h>
> @@ -8,7 +12,6 @@
> #include <efi/eficon.h>
> #include <efi/efidevp.h>
> #include <efi/efiapi.h>
> -#include "../../../common/efi/stub.c"
> 
> /*
>  * Here we are in EFI stub. EFI calls are not supported due to lack
> diff --git a/xen/common/efi/stub.c b/xen/common/efi/stub.c
> index 15694632c2..854efd9c99 100644
> --- a/xen/common/efi/stub.c
> +++ b/xen/common/efi/stub.c
> @@ -30,3 +30,8 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
> {
>     return -ENOSYS;
> }
> +
> +/*
> + * Include architecture specific implementation here.
> + */
> +#include "stub.h"
> diff --git a/.gitignore b/.gitignore
> index ed7bd8bdc7..3a91e79672 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -311,6 +311,7 @@ xen/arch/*/efi/ebmalloc.c
> xen/arch/*/efi/efi.h
> xen/arch/*/efi/pe.c
> xen/arch/*/efi/runtime.c
> +xen/arch/*/efi/stub.c
> xen/arch/*/include/asm/asm-offsets.h
> xen/common/config_data.S
> xen/common/config.gz
> -- 
> Anthony PERARD
>