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. And build both *stub.c as two different objects.
This mean we have to move some efi_compat_* aliases which are probably
useless for Arm.

Avoid using $(EFIOBJ-y) as an alias for $(clean-files), add
common_stub.c directly to $(clean-files).

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

For the cflag addition in non-ARM_EFI, I was tempted to apply it to
the whole directory instead of just stub.o. (Even if there's only a
single object). I think that would be enough to overwrite the
-fshort-wchar from efi-common.mk, but I forgot what cflags come after
that. But adding it to just one object mean that it's added at the
last possible moment.
---
 xen/arch/arm/efi/Makefile                | 8 ++------
 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} | 6 ++++++
 5 files changed, 11 insertions(+), 16 deletions(-)
 rename xen/common/efi/{stub.c => common_stub.c} (67%)

diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile
index bd954a3b2d..8e463d156a 100644
--- a/xen/arch/arm/efi/Makefile
+++ b/xen/arch/arm/efi/Makefile
@@ -4,12 +4,8 @@ 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
+$(obj)/common_stub.o: CFLAGS-y += -fno-short-wchar
 
 endif
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 034ec87895..bbabfc3795 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..5d5c427e8b 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 67%
rename from xen/common/efi/stub.c
rename to xen/common/efi/common_stub.c
index 15694632c2..4dc724b2ac 100644
--- a/xen/common/efi/stub.c
+++ b/xen/common/efi/common_stub.c
@@ -30,3 +30,9 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
 {
     return -ENOSYS;
 }
+
+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")));
-- 
Anthony PERARD

On 16/08/2022 11:30, 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
> 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. And build both *stub.c as two different objects.
> This mean we have to move some efi_compat_* aliases which are probably
> useless for Arm.
>
> Avoid using $(EFIOBJ-y) as an alias for $(clean-files), add
> common_stub.c directly to $(clean-files).
>
> 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>

Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

On 16.08.2022 12:30, 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
> 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. And build both *stub.c as two different objects.
> This mean we have to move some efi_compat_* aliases which are probably
> useless for Arm.

These useless aliases want avoiding there imo. Already when the original
series was discussed, I requested to avoid introduction of a file named
common-stub.c or alike. If names need to be different, can't we follow
boot.c's model and introduce a per-arch efi/stub.h which stub.c would
include at a suitable position (and which right now would be empty for
Arm)?

> Avoid using $(EFIOBJ-y) as an alias for $(clean-files), add
> common_stub.c directly to $(clean-files).
> 
> 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>
> ---
> 
> For the cflag addition in non-ARM_EFI, I was tempted to apply it to
> the whole directory instead of just stub.o. (Even if there's only a
> single object). I think that would be enough to overwrite the
> -fshort-wchar from efi-common.mk, but I forgot what cflags come after
> that. But adding it to just one object mean that it's added at the
> last possible moment.
> ---
>  xen/arch/arm/efi/Makefile                | 8 ++------
>  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} | 6 ++++++

At the very least I'd like to request to avoid the underscore in the
file name.

Jan

On Tue, Aug 16, 2022 at 03:02:10PM +0200, Jan Beulich wrote:
> On 16.08.2022 12:30, 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
> > 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. And build both *stub.c as two different objects.
> > This mean we have to move some efi_compat_* aliases which are probably
> > useless for Arm.
> 
> These useless aliases want avoiding there imo. Already when the original
> series was discussed, I requested to avoid introduction of a file named
> common-stub.c or alike.

Yeah, I've notice that. This is why the build is broken under
specific condition.

> If names need to be different, can't we follow
> boot.c's model and introduce a per-arch efi/stub.h which stub.c would
> include at a suitable position (and which right now would be empty for
> Arm)?

That seems to be possible. But how is it better than having two
different source file? The only thing is to avoid exporting the
efi_compat_* symbol aliases. The downside is we would have another weird
looking not really header which is actually just part of a source file.
At least, "stub.c" and "stub.h" would be two different names, we just
change the extension rather than the basename.

Cheers,

-- 
Anthony PERARD

On 16.08.2022 17:43, Anthony PERARD wrote:
> On Tue, Aug 16, 2022 at 03:02:10PM +0200, Jan Beulich wrote:
>> On 16.08.2022 12:30, 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
>>> 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. And build both *stub.c as two different objects.
>>> This mean we have to move some efi_compat_* aliases which are probably
>>> useless for Arm.
>>
>> These useless aliases want avoiding there imo. Already when the original
>> series was discussed, I requested to avoid introduction of a file named
>> common-stub.c or alike.
> 
> Yeah, I've notice that. This is why the build is broken under
> specific condition.
> 
>> If names need to be different, can't we follow
>> boot.c's model and introduce a per-arch efi/stub.h which stub.c would
>> include at a suitable position (and which right now would be empty for
>> Arm)?
> 
> That seems to be possible. But how is it better than having two
> different source file? The only thing is to avoid exporting the
> efi_compat_* symbol aliases.

As said - I think they're wrong to have in Arm. But if Arm maintainers
don't care about them being there, so be it. As long as they don't
voice a view, I guess as the EFI maintainer I can sensibly ask for
them to be avoided in a reasonably clean way.

> The downside is we would have another weird
> looking not really header which is actually just part of a source file.
> At least, "stub.c" and "stub.h" would be two different names, we just
> change the extension rather than the basename.

Whether that's "weird" is certainly a matter of taste ... To me,
common-stub.c also comes close  to "weird", fwiw. But as I've tried
to express, if I'm the only one disliking common-stub.c, then please
ignore my view and I'll nevertheless ack the resulting patch. (That
said, I view the vpath issue causing the problem as really the one
that would want tackling. There shouldn't be a requirement for
files to have different names as long as they live in different
directories.)

Jan

Hi Jan,

On 16/08/2022 17:29, Jan Beulich wrote:
> On 16.08.2022 17:43, Anthony PERARD wrote:
>> On Tue, Aug 16, 2022 at 03:02:10PM +0200, Jan Beulich wrote:
>>> On 16.08.2022 12:30, 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
>>>> 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. And build both *stub.c as two different objects.
>>>> This mean we have to move some efi_compat_* aliases which are probably
>>>> useless for Arm.
>>>
>>> These useless aliases want avoiding there imo. Already when the original
>>> series was discussed, I requested to avoid introduction of a file named
>>> common-stub.c or alike.
>>
>> Yeah, I've notice that. This is why the build is broken under
>> specific condition.
>>
>>> If names need to be different, can't we follow
>>> boot.c's model and introduce a per-arch efi/stub.h which stub.c would
>>> include at a suitable position (and which right now would be empty for
>>> Arm)?
>>
>> That seems to be possible. But how is it better than having two
>> different source file? The only thing is to avoid exporting the
>> efi_compat_* symbol aliases.
> 
> As said - I think they're wrong to have in Arm. But if Arm maintainers
> don't care about them being there, so be it. As long as they don't
> voice a view, I guess as the EFI maintainer I can sensibly ask for
> them to be avoided in a reasonably clean way.

AFAIU, the two aliases are using by the compat code. So how about 
protecting it with CONFIG_COMPAT (like we do for other compat code in 
common code)?

> 
>> The downside is we would have another weird
>> looking not really header which is actually just part of a source file.
>> At least, "stub.c" and "stub.h" would be two different names, we just
>> change the extension rather than the basename.
> 
> Whether that's "weird" is certainly a matter of taste ... To me,
> common-stub.c also comes close  to "weird", fwiw. But as I've tried
> to express, if I'm the only one disliking common-stub.c, then please
> ignore my view and I'll nevertheless ack the resulting patch. (That
> said, I view the vpath issue causing the problem as really the one
> that would want tackling. There shouldn't be a requirement for
> files to have different names as long as they live in different
> directories.)

So I agree with Anthony here. I think the approach we use for 
boot.c/efi-boot.h should not be promoted.

I also agree that "common-stub.c" sounds weird. But it would require 
some change in the build system (I always find a bit string we are using 
linking) which is likely too late for 4.17.

So I would be fine with stub-common.c and then protect the alias with 
#ifdef CONFIG_COMPAT.

Cheers,

-- 
Julien Grall

On 18.08.2022 19:42, Julien Grall wrote:
> On 16/08/2022 17:29, Jan Beulich wrote:
>> On 16.08.2022 17:43, Anthony PERARD wrote:
>>> On Tue, Aug 16, 2022 at 03:02:10PM +0200, Jan Beulich wrote:
>>>> On 16.08.2022 12:30, 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
>>>>> 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. And build both *stub.c as two different objects.
>>>>> This mean we have to move some efi_compat_* aliases which are probably
>>>>> useless for Arm.
>>>>
>>>> These useless aliases want avoiding there imo. Already when the original
>>>> series was discussed, I requested to avoid introduction of a file named
>>>> common-stub.c or alike.
>>>
>>> Yeah, I've notice that. This is why the build is broken under
>>> specific condition.
>>>
>>>> If names need to be different, can't we follow
>>>> boot.c's model and introduce a per-arch efi/stub.h which stub.c would
>>>> include at a suitable position (and which right now would be empty for
>>>> Arm)?
>>>
>>> That seems to be possible. But how is it better than having two
>>> different source file? The only thing is to avoid exporting the
>>> efi_compat_* symbol aliases.
>>
>> As said - I think they're wrong to have in Arm. But if Arm maintainers
>> don't care about them being there, so be it. As long as they don't
>> voice a view, I guess as the EFI maintainer I can sensibly ask for
>> them to be avoided in a reasonably clean way.
> 
> AFAIU, the two aliases are using by the compat code. So how about 
> protecting it with CONFIG_COMPAT (like we do for other compat code in 
> common code)?

Hmm, yes, that ought to work.

Jan

Hi Anthony,

On 16/08/2022 11:30, 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
> comfusion in make and it can pick up the wrong source file. This issue

Typo: s/comfusion/confusion/

> 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. And build both *stub.c as two different objects.
> This mean we have to move some efi_compat_* aliases which are probably
> useless for Arm.
> 
> Avoid using $(EFIOBJ-y) as an alias for $(clean-files), add
> common_stub.c directly to $(clean-files).
> 
> 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>
> ---
> 
> For the cflag addition in non-ARM_EFI, I was tempted to apply it to
> the whole directory instead of just stub.o. (Even if there's only a
> single object). I think that would be enough to overwrite the
> -fshort-wchar from efi-common.mk, but I forgot what cflags come after
> that. But adding it to just one object mean that it's added at the
> last possible moment.
> ---
>   xen/arch/arm/efi/Makefile                | 8 ++------
>   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} | 6 ++++++

I haven't looked at the rest of the patch. However, I think you also 
want to update .gitignore to excluse arch/*/efi/common_stub.c.

Also, I am thinking to drop my patch [1] which update .gitignore as this 
will become moot with this change. Let me know what you think.

Cheers,

[1] 20220812191930.34494-1-julien@xen.org

-- 
Julien Grall

On Tue, Aug 16, 2022 at 12:01:40PM +0100, Julien Grall wrote:
> >   xen/common/efi/{stub.c => common_stub.c} | 6 ++++++
> 
> I haven't looked at the rest of the patch. However, I think you also want to
> update .gitignore to excluse arch/*/efi/common_stub.c.
> 
> Also, I am thinking to drop my patch [1] which update .gitignore as this
> will become moot with this change. Let me know what you think.

Sound good,

Thanks,

-- 
Anthony PERARD

This patch probably need a slight better subject, as the issue is only
with out-of-tree build. So new subject:
    build: Fix x86 out-of-tree build without EFI

-- 
Anthony PERARD

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

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@suse.com>
    CC: Wei Chen <wei.chen@arm.com>

 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

Hi Anthony,

> On 17 Aug 2022, at 10:15, Anthony PERARD <anthony.perard@citrix.com> 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@citrix.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

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

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@suse.com>
>    CC: Wei Chen <wei.chen@arm.com>
> 
> 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
> 

On 17/08/2022 10:15, 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
> 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@citrix.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

This version is broken I'm afraid.

/builddir/build/BUILD/xen-4.17.0/xen/build-shim-release/../arch/x86/setup.c:2045:(.init.text+0x3bc14):
relocation truncated to fit: R_X86_64_PLT32 against undefined symbol
`efi_boot_mem_unused'
ld: ./.xen-syms.0: hidden symbol `efi_boot_mem_unused' isn't defined
ld: final link failed: bad value

~Andrew

On 17/08/2022 12:02, Andrew Cooper wrote:
> On 17/08/2022 10:15, 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
>> 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@citrix.com>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> This version is broken I'm afraid.

No it's not.  User error on my behalf.  Sorry.

Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

On 17.08.2022 11:15, Anthony PERARD wrote:
> --- 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) $@

I'm afraid the commit message hasn't made clear to me why this part of
the change is (still) needed (or perhaps just wanted). The rest of this
lgtm now, thanks.

Jan

On Wed, Aug 17, 2022 at 12:38:36PM +0200, Jan Beulich wrote:
> On 17.08.2022 11:15, Anthony PERARD wrote:
> > --- 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) $@
> 
> I'm afraid the commit message hasn't made clear to me why this part of
> the change is (still) needed (or perhaps just wanted). The rest of this
> lgtm now, thanks.

There's an explanation in the commit message, quoted here:
> >  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".

The problem with `test -f $@` it doesn't test what we think it does. It
doesn't test for the presence of a regular file in the source tree as
stated in the original tree. First, `test -f` happily follow symlinks.
Second, $@ is always going to point to the build dir, as GNU Make will
try to not make changes to the source tree, if I understand the logic
correctly.

Instead of `test -f`, we could probably remove the "FORCE" from the
prerequisite, but there's still going to be an issue if there's a file
with the same name in both common and per-arch directory, when the common
file is newer.

So `test -f` needs to go.

Cheers,

-- 
Anthony PERARD

On 17.08.2022 16:12, Anthony PERARD wrote:
> On Wed, Aug 17, 2022 at 12:38:36PM +0200, Jan Beulich wrote:
>> On 17.08.2022 11:15, Anthony PERARD wrote:
>>> --- 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) $@
>>
>> I'm afraid the commit message hasn't made clear to me why this part of
>> the change is (still) needed (or perhaps just wanted). The rest of this
>> lgtm now, thanks.
> 
> There's an explanation in the commit message, quoted here:
>>>  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".

Hmm, yes, I had guessed that this might be it, but I wasn't able to make
the connection, sorry.

> The problem with `test -f $@` it doesn't test what we think it does. It
> doesn't test for the presence of a regular file in the source tree as
> stated in the original tree.

I didn't think it would to that. $@ is the target of the rule, and the
(pattern) target explicitly points into the build tree, by way of using
$(obj).

> First, `test -f` happily follow symlinks.

Which is of no relevance here, afaict.

> Second, $@ is always going to point to the build dir, as GNU Make will
> try to not make changes to the source tree, if I understand the logic
> correctly.
> 
> Instead of `test -f`, we could probably remove the "FORCE" from the
> prerequisite, but there's still going to be an issue if there's a file
> with the same name in both common and per-arch directory, when the common
> file is newer.

This would be a mistake now, wouldn't it? I did add "(still)" in my earlier
reply for the very reason that it looks to me as if this change might have
been an attempt to address the issue without any renaming.

> So `test -f` needs to go.

I'm sorry to conclude that for now I continue to only see that its removal
does no harm (hence the "(or perhaps just wanted)" in my original reply),
but I still don't see that it's strictly needed. Therefore I'm okay with
the change as is, but I don't view the description as quite clear enough
in this one regard.

Jan