| Message ID | 20220817091540.18949-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v2] build: Fix x86 out-of-tree build without EFI | expand |
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 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 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,
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 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
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
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%)