| Message ID | 20240903115334.3526368-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] x86/boot: Fix include paths for 32bit objects | expand |
On Tue, Sep 03, 2024 at 12:53:34PM +0100, Andrew Cooper wrote: > Most of Xen is build using -nostdinc and a fully specified include path. > However, the makefile line: > > $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > > discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. > > Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> I wouldn't mind if you also open-coded the config.h -include addition to CFLAGS_x86_32, regardless: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> I do wonder however whether the explicit assembler includes parameters (-Wa,-I) are actually required, seeing as we only provide include/ to the assembler, but not the arch-specific include paths. This is from XSA-254, which used the '.include' asm directive, but that was ultimately removed by: 762c3890c89f x86: fold indirect_thunk_asm.h into asm-defns.h So maybe the -Wa,-I is no longer needed? Thanks, Roger.
On 03/09/2024 1:43 pm, Roger Pau Monné wrote: > On Tue, Sep 03, 2024 at 12:53:34PM +0100, Andrew Cooper wrote: >> Most of Xen is build using -nostdinc and a fully specified include path. >> However, the makefile line: >> >> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic >> >> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. >> >> Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > I wouldn't mind if you also open-coded the config.h -include addition > to CFLAGS_x86_32, regardless: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. TBH, I'm going to put it in as is and unblock the fixes behind it. We can adjust the others in due course. Given the other shuffling of headers we've done recently, I'm starting to think that the -include isn't really as needed as it might once have been. > I do wonder however whether the explicit assembler includes parameters > (-Wa,-I) are actually required, seeing as we only provide include/ to > the assembler, but not the arch-specific include paths. > > This is from XSA-254, which used the '.include' asm directive, but > that was ultimately removed by: > > 762c3890c89f x86: fold indirect_thunk_asm.h into asm-defns.h > > So maybe the -Wa,-I is no longer needed? Perhaps, although I'm struggling to find where it's declared today. ~Andrew
On Tue, Sep 03, 2024 at 02:01:45PM +0100, Andrew Cooper wrote: > On 03/09/2024 1:43 pm, Roger Pau Monné wrote: > > On Tue, Sep 03, 2024 at 12:53:34PM +0100, Andrew Cooper wrote: > >> Most of Xen is build using -nostdinc and a fully specified include path. > >> However, the makefile line: > >> > >> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > >> > >> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. > >> > >> Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. > >> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > I wouldn't mind if you also open-coded the config.h -include addition > > to CFLAGS_x86_32, regardless: > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > TBH, I'm going to put it in as is and unblock the fixes behind it. > > We can adjust the others in due course. Sure, if that allows you to unblock the rest. > Given the other shuffling of headers we've done recently, I'm starting > to think that the -include isn't really as needed as it might once have > been. > > > I do wonder however whether the explicit assembler includes parameters > > (-Wa,-I) are actually required, seeing as we only provide include/ to > > the assembler, but not the arch-specific include paths. > > > > This is from XSA-254, which used the '.include' asm directive, but > > that was ultimately removed by: > > > > 762c3890c89f x86: fold indirect_thunk_asm.h into asm-defns.h > > > > So maybe the -Wa,-I is no longer needed? > > Perhaps, although I'm struggling to find where it's declared today. It's in xen/arch/x86/arch.mk. Thanks, Roger.
On 03.09.2024 13:53, Andrew Cooper wrote: > Most of Xen is build using -nostdinc and a fully specified include path. > However, the makefile line: > > $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > > discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. > > Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. And the files in boot/ indeed need the -Wa,-I? I'd favor us not propagating such without actually being required for a particular reason. Jan
On 03/09/2024 3:32 pm, Jan Beulich wrote: > On 03.09.2024 13:53, Andrew Cooper wrote: >> Most of Xen is build using -nostdinc and a fully specified include path. >> However, the makefile line: >> >> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic >> >> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. >> >> Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. > And the files in boot/ indeed need the -Wa,-I? I'd favor us not propagating > such without actually being required for a particular reason. Hmm ok fine, I'll strip that back out as both you and Roger have asked for it. I'm still waiting on Gitlab CI, so haven't pushed yet. ~Andrew
On 03/09/2024 3:33 pm, Andrew Cooper wrote: > On 03/09/2024 3:32 pm, Jan Beulich wrote: >> On 03.09.2024 13:53, Andrew Cooper wrote: >>> Most of Xen is build using -nostdinc and a fully specified include path. >>> However, the makefile line: >>> >>> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic >>> >>> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. >>> >>> Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. >> And the files in boot/ indeed need the -Wa,-I? I'd favor us not propagating >> such without actually being required for a particular reason. > Hmm ok fine, I'll strip that back out as both you and Roger have asked > for it. > > I'm still waiting on Gitlab CI, so haven't pushed yet. Eclair has just said no, because the -include is needed to pull in Kconfig, ahead of compiler.h using CONFIG_GCC_* symbols I'm retesting with: @@ -14,10 +14,8 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin) CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -ifneq ($(abs_objtree),$(abs_srctree)) -CFLAGS_x86_32 += -I$(objtree)/include -endif -CFLAGS_x86_32 += -I$(srctree)/include +CFLAGS_x86_32 += -nostdinc -include $(filter %/include/xen/config.h,$(XEN_CFLAGS)) +CFLAGS_x86_32 += $(filter -I%,$(XEN_CFLAGS)) # override for 32bit binaries $(head-bin-objs): CFLAGS_stack_boundary := which is the best I can think of to find the path of config.h in XEN_CFLAGS. This is the same pattern we use in filter-out elsewhere. ~Andrew
On 03.09.2024 13:53, Andrew Cooper wrote: > Most of Xen is build using -nostdinc and a fully specified include path. > However, the makefile line: > > $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > > discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. > > Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Despite the title saying "Fix" I take the absence of a Fixes: tag as meaning that this won't need backporting, and is rather only needed for what went on top. Jan
On 04/09/2024 7:54 am, Jan Beulich wrote: > On 03.09.2024 13:53, Andrew Cooper wrote: >> Most of Xen is build using -nostdinc and a fully specified include path. >> However, the makefile line: >> >> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic >> >> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. >> >> Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Despite the title saying "Fix" I take the absence of a Fixes: tag as meaning > that this won't need backporting, and is rather only needed for what went on > top. Oh sorry. v1 had a Fixes tag, and then Anthony objected. "x86/boot: Use <xen/types.h>" is where it breaks properly without this patch, so I don't think there's a specific need to backport. ~Andrew
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index 03d8ce3a9e48..1ec2b123305d 100644 --- a/xen/arch/x86/boot/Makefile +++ b/xen/arch/x86/boot/Makefile @@ -14,10 +14,7 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin) CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -ifneq ($(abs_objtree),$(abs_srctree)) -CFLAGS_x86_32 += -I$(objtree)/include -endif -CFLAGS_x86_32 += -I$(srctree)/include +CFLAGS_x86_32 += -nostdinc $(filter -I% -Wa$(comma)-I%,$(XEN_CFLAGS)) # override for 32bit binaries $(head-bin-objs): CFLAGS_stack_boundary :=
Most of Xen is build using -nostdinc and a fully specified include path. However, the makefile line: $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. Reinstate -nostdinc, and copy all -I (and -Wa,-I) arguments from XEN_CFLAGS. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Anthony PERARD <anthony.perard@vates.tech> v2: * Copy all -I from XEN_CFLAGS https://cirrus-ci.com/build/6740464739549184 Note that this misses the overall '-include ./include/xen/config.h' but it was missing before as well. --- xen/arch/x86/boot/Makefile | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)