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
confusion 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" doesn'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 can hide the
compat aliases that x86 uses behind CONFIG_COMPAT so a Arm build will
not have them.

Also revert the change to the rule that creates symbolic links it's
better to just recreate the link in cases where an existing file exist
or the link goes to the wrong file.

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@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v3:
    - back to using common-stub.c
    - include the x86 compat function alias in common-stub.c but guard them
      with CONFIG_COMPAT
    
    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: Wei Chen <wei.chen@arm.com>

 xen/arch/arm/efi/Makefile                |  6 +-----
 xen/arch/x86/efi/Makefile                |  2 +-
 xen/common/efi/efi-common.mk             |  4 ++--
 xen/arch/x86/efi/stub.c                  |  7 -------
 xen/common/efi/{stub.c => common-stub.c} | 10 ++++++++++
 .gitignore                               |  1 +
 6 files changed, 15 insertions(+), 15 deletions(-)
 rename xen/common/efi/{stub.c => common-stub.c} (64%)

diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile
index bd954a3b2d..2459cbae3a 100644
--- a/xen/arch/arm/efi/Makefile
+++ b/xen/arch/arm/efi/Makefile
@@ -4,11 +4,7 @@ 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-y += common-stub.o
 
 $(obj)/stub.o: CFLAGS-y += -fno-short-wchar
 
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 034ec87895..24dfecfad1 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -11,7 +11,7 @@ $(obj)/boot.init.o: $(obj)/buildid.o
 $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
 $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS_stack_boundary := $(cflags-stack-boundary)
 
-obj-y := stub.o
+obj-y := common-stub.o stub.o
 obj-$(XEN_BUILD_EFI) := $(filter-out %.init.o,$(EFIOBJ-y))
 obj-bin-$(XEN_BUILD_EFI) := $(filter %.init.o,$(EFIOBJ-y))
 extra-$(XEN_BUILD_EFI) += buildid.o relocs-dummy.o
diff --git a/xen/common/efi/efi-common.mk b/xen/common/efi/efi-common.mk
index ec2c34f198..53fdb81583 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 += common-stub.c
 
 .PRECIOUS: $(obj)/%.c
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index f2365bc041..2cd5c8d4dc 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -8,7 +8,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
@@ -55,9 +54,3 @@ bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
 }
 
 void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
-
-int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *)
-    __attribute__((__alias__("efi_get_info")));
-
-int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *)
-    __attribute__((__alias__("efi_runtime_call")));
diff --git a/xen/common/efi/stub.c b/xen/common/efi/common-stub.c
similarity index 64%
rename from xen/common/efi/stub.c
rename to xen/common/efi/common-stub.c
index 15694632c2..5a91fe28cc 100644
--- a/xen/common/efi/stub.c
+++ b/xen/common/efi/common-stub.c
@@ -30,3 +30,13 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
 {
     return -ENOSYS;
 }
+
+#ifdef CONFIG_COMPAT
+
+int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *)
+    __attribute__((__alias__("efi_get_info")));
+
+int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *)
+    __attribute__((__alias__("efi_runtime_call")));
+
+#endif
diff --git a/.gitignore b/.gitignore
index af9bf749c4..98adcf02ad 100644
--- a/.gitignore
+++ b/.gitignore
@@ -280,6 +280,7 @@ xen/arch/*/efi/ebmalloc.c
 xen/arch/*/efi/efi.h
 xen/arch/*/efi/pe.c
 xen/arch/*/efi/runtime.c
+xen/arch/*/efi/common-stub.c
 xen/arch/*/include/asm/asm-offsets.h
 xen/common/config_data.S
 xen/common/config.gz
-- 
Anthony PERARD

On 19.09.2022 12:28, Anthony PERARD 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
> confusion 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" doesn'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 can hide the
> compat aliases that x86 uses behind CONFIG_COMPAT so a Arm build will
> not have them.
> 
> Also revert the change to the rule that creates symbolic links it's
> better to just recreate the link in cases where an existing file exist
> or the link goes to the wrong file.
> 
> 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@citrix.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
with a remark and one more adjustment:

> ---
> 
> Notes:
>     v3:
>     - back to using common-stub.c

As said in person, I'm a little puzzled by this, as the v2 discussion
had no hint in that direction.

> --- a/.gitignore
> +++ b/.gitignore
> @@ -280,6 +280,7 @@ xen/arch/*/efi/ebmalloc.c
>  xen/arch/*/efi/efi.h
>  xen/arch/*/efi/pe.c
>  xen/arch/*/efi/runtime.c
> +xen/arch/*/efi/common-stub.c
>  xen/arch/*/include/asm/asm-offsets.h
>  xen/common/config_data.S
>  xen/common/config.gz

The new line wants inserting a few lines further up, to retain sorting.
Can perhaps be done while committing.

Jan

On Mon, Sep 19, 2022 at 01:02:52PM +0200, Jan Beulich wrote:
> On 19.09.2022 12:28, Anthony PERARD wrote:
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> with a remark and one more adjustment:
> 
> > ---
> > 
> > Notes:
> >     v3:
> >     - back to using common-stub.c
> 
> As said in person, I'm a little puzzled by this, as the v2 discussion
> had no hint in that direction.

I think it's because the discussion continued on V1, and I mostly follow
that:
    https://lore.kernel.org/xen-devel/5d926715-b6af-234f-9798-622cdb53f181@xen.org/

> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -280,6 +280,7 @@ xen/arch/*/efi/ebmalloc.c
> >  xen/arch/*/efi/efi.h
> >  xen/arch/*/efi/pe.c
> >  xen/arch/*/efi/runtime.c
> > +xen/arch/*/efi/common-stub.c
> >  xen/arch/*/include/asm/asm-offsets.h
> >  xen/common/config_data.S
> >  xen/common/config.gz
> 
> The new line wants inserting a few lines further up, to retain sorting.

Oops, forgot to re-sort after renaming from "stub" to "common-stub".

> Can perhaps be done while committing.

Thanks,

-- 
Anthony PERARD