| Message ID | 20220816103043.32662-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] build: Fix x86 build without EFI | expand |
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
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
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 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,
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,
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 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,
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
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")));
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%)