1. Patchew
  2. Xen
  3. View series

[PATCH v2] x86/boot: Fix include paths for 32bit objects

Andrew Cooper posted 1 patch 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240903115334.3526368-1-andrew.cooper3@citrix.com
xen/arch/x86/boot/Makefile | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
[PATCH v2] x86/boot: Fix include paths for 32bit objects
Posted by Andrew Cooper 3 months ago 
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(-)

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 :=
-- 
2.39.2
Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects
Posted by Jan Beulich 3 months ago 
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
Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects
Posted by Andrew Cooper 3 months ago 
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
Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects
Posted by Jan Beulich 3 months ago 
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
Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects
Posted by Andrew Cooper 3 months ago 
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
Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects
Posted by Andrew Cooper 3 months ago 
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
Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects
Posted by Roger Pau Monné 3 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.