Mailing List Archive

[XEN PATCH] build: Fix x86 build without EFI
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
Re: [XEN PATCH] build: Fix x86 out-of-tree build without EFI [ In reply to ]
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
Re: [XEN PATCH] build: Fix x86 build without EFI [ In reply to ]
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
Re: [XEN PATCH] build: Fix x86 build without EFI [ In reply to ]
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
Re: [XEN PATCH] build: Fix x86 build without EFI [ In reply to ]
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
Re: [XEN PATCH] build: Fix x86 build without EFI [ In reply to ]
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
Re: [XEN PATCH] build: Fix x86 build without EFI [ In reply to ]
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>
Re: [XEN PATCH] build: Fix x86 build without EFI [ In reply to ]
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
Re: [XEN PATCH] build: Fix x86 build without EFI [ In reply to ]
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
Re: [XEN PATCH] build: Fix x86 build without EFI [ In reply to ]
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