[PATCH v2 0/2] firmware: build fixes with gcc-11

[PATCH v2 0/2] firmware: build fixes with gcc-11

[Roger Pau Monne]

Hello,

The following fixes some firmware build issues with gcc-11. Note that
dropping of .note.gnu.property section could likely be done in the
linker script in the hvmloader case, but rombios has no linker script
and such note is causing a non-working image. Other options could be
using objcopy to drop the section, but those seems more complicated than
just using the assembler command line option.

Thanks, Roger.

Roger Pau Monne (2):
  tools/firmware: fix setting of fcf-protection=none
  tools/firmware: do not add a .note.gnu.property section

 Config.mk               | 2 +-
 tools/firmware/Makefile | 2 --
 tools/firmware/Rules.mk | 6 ++++++
 3 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.35.1

[PATCH v2 1/2] tools/firmware: fix setting of fcf-protection=none

[Roger Pau Monne]

Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
Makefile doesn't get it propagated to the subdirectories, so instead
set the flag in firmware/Rules.mk, like it's done for other compiler
flags.

Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Add the option directly to CFLAGS using cc-option-add.
---
 tools/firmware/Makefile | 2 --
 tools/firmware/Rules.mk | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 53ed4f161e..345037b93b 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -6,8 +6,6 @@ TARGET      := hvmloader/hvmloader
 INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR)
 DEBG_DIR := $(DESTDIR)$(DEBUG_DIR)$(XENFIRMWAREDIR)
 
-EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
-
 SUBDIRS-y :=
 SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
 SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
index 9f78a7dec9..c227fe2524 100644
--- a/tools/firmware/Rules.mk
+++ b/tools/firmware/Rules.mk
@@ -15,6 +15,8 @@ CFLAGS += -Werror
 
 $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 
+$(call cc-option-add,CFLAGS,CC,-fcf-protection=none)
+
 # Extra CFLAGS suitable for an embedded type of environment.
 CFLAGS += -ffreestanding -msoft-float
 
-- 
2.35.1

[PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section

[Roger Pau Monne]

Prevent the assembler from creating a .note.gnu.property section on
the output objects, as it's not useful for firmware related binaries,
and breaks the resulting rombios image.

This requires modifying the cc-option Makefile macro so it can test
assembler options (by replacing the usage of the -S flag with -c) and
also stripping the -Wa, prefix if present when checking for the test
output.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Add the option to CFLAGS.
---
 Config.mk               | 2 +-
 tools/firmware/Rules.mk | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Config.mk b/Config.mk
index f56f7dc334..82832945e5 100644
--- a/Config.mk
+++ b/Config.mk
@@ -91,7 +91,7 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix)"
 #
 # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
 cc-option = $(shell if test -z "`echo 'void*p=1;' | \
-              $(1) $(2) -S -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \
+              $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2:-Wa$(comma)%=%) -`"; \
               then echo "$(2)"; else echo "$(3)"; fi ;)
 
 # cc-option-add: Add an option to compilation flags, but only if supported.
diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
index c227fe2524..278cca01e4 100644
--- a/tools/firmware/Rules.mk
+++ b/tools/firmware/Rules.mk
@@ -17,6 +17,10 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 
 $(call cc-option-add,CFLAGS,CC,-fcf-protection=none)
 
+# Do not add the .note.gnu.property section to any of the firmware objects: it
+# breaks the rombios binary and is not useful for firmware anyway.
+$(call cc-option-add,CFLAGS,CC,-Wa$$(comma)-mx86-used-note=no)
+
 # Extra CFLAGS suitable for an embedded type of environment.
 CFLAGS += -ffreestanding -msoft-float
 
-- 
2.35.1

Re: [PATCH v2 1/2] tools/firmware: fix setting of fcf-protection=none

[Anthony PERARD]

On Mon, Apr 04, 2022 at 12:40:43PM +0200, Roger Pau Monne wrote:
> Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
> Makefile doesn't get it propagated to the subdirectories, so instead
> set the flag in firmware/Rules.mk, like it's done for other compiler
> flags.
> 
> Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section

[Anthony PERARD]

On Mon, Apr 04, 2022 at 12:40:44PM +0200, Roger Pau Monne wrote:
> Prevent the assembler from creating a .note.gnu.property section on
> the output objects, as it's not useful for firmware related binaries,
> and breaks the resulting rombios image.
> 
> This requires modifying the cc-option Makefile macro so it can test
> assembler options (by replacing the usage of the -S flag with -c) and
> also stripping the -Wa, prefix if present when checking for the test
> output.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Add the option to CFLAGS.
> ---
>  Config.mk               | 2 +-
>  tools/firmware/Rules.mk | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Config.mk b/Config.mk
> index f56f7dc334..82832945e5 100644
> --- a/Config.mk
> +++ b/Config.mk
> @@ -91,7 +91,7 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix)"
>  #
>  # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
>  cc-option = $(shell if test -z "`echo 'void*p=1;' | \
> -              $(1) $(2) -S -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \
> +              $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2:-Wa$(comma)%=%) -`"; \
>                then echo "$(2)"; else echo "$(3)"; fi ;)

Hopefully, changing "-S" to "-c" in this macro will not break anything.
I would be of the opinion to create a new macro which deal with
assembler options. But if that works and doesn't changes CFLAGS in the
testing we do in GitLab, I guess that would be OK.

Whether you introduce a macro or keep this one:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section

[Anthony PERARD]

On Mon, Apr 04, 2022 at 12:12:54PM +0100, Anthony PERARD wrote:
> On Mon, Apr 04, 2022 at 12:40:44PM +0200, Roger Pau Monne wrote:
> > Prevent the assembler from creating a .note.gnu.property section on
> > the output objects, as it's not useful for firmware related binaries,
> > and breaks the resulting rombios image.
> > 
> > This requires modifying the cc-option Makefile macro so it can test
> > assembler options (by replacing the usage of the -S flag with -c) and
> > also stripping the -Wa, prefix if present when checking for the test
> > output.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v1:
> >  - Add the option to CFLAGS.
> > ---
> >  Config.mk               | 2 +-
> >  tools/firmware/Rules.mk | 4 ++++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Config.mk b/Config.mk
> > index f56f7dc334..82832945e5 100644
> > --- a/Config.mk
> > +++ b/Config.mk
> > @@ -91,7 +91,7 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix)"
> >  #
> >  # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586)
> >  cc-option = $(shell if test -z "`echo 'void*p=1;' | \
> > -              $(1) $(2) -S -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \
> > +              $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2:-Wa$(comma)%=%) -`"; \
> >                then echo "$(2)"; else echo "$(3)"; fi ;)
> 
> Hopefully, changing "-S" to "-c" in this macro will not break anything.
> I would be of the opinion to create a new macro which deal with
> assembler options. But if that works and doesn't changes CFLAGS in the
> testing we do in GitLab, I guess that would be OK.

It looks like Linux already use "-c" for this macro, and with "-Wa,"
options. They just don't use grep. So asking CC to do more work here is
probably fine (adding compile stage).

Cheers,

-- 
Anthony PERARD

Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section

[Jan Beulich]

On 04.04.2022 12:40, Roger Pau Monne wrote:
> Prevent the assembler from creating a .note.gnu.property section on
> the output objects, as it's not useful for firmware related binaries,
> and breaks the resulting rombios image.
> 
> This requires modifying the cc-option Makefile macro so it can test
> assembler options (by replacing the usage of the -S flag with -c) and
> also stripping the -Wa, prefix if present when checking for the test
> output.

I notice you've ack-ed and committed this patch, which I'm happy to
see. However, I don't understand why you gave your ack here, when you
did refused to ack (and to explain yourself!) for "x86emul: fix test
harness build for gas 2.36". Why is this note section useful there
but not similarly useful here (or, the other way around, useless)?

(This, as an aside, also makes pretty clear that - unlike the title
of the series suggests - this has nothing to do with gcc 11.)

Jan

Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section

[Roger Pau Monné]

On Mon, Apr 04, 2022 at 02:45:05PM +0200, Jan Beulich wrote:
> (This, as an aside, also makes pretty clear that - unlike the title
> of the series suggests - this has nothing to do with gcc 11.)

I've started debugging this as reported as an issue associated with
building with gcc 11, until I realized it was the assembler that was
adding such section. I guess in my mind I still had the idea of fixing
the build with gcc 11, and hence the misleading subject of the cover
letter. The commit messages should be fine however.

Thanks, Roger.

Re: [PATCH v2 2/2] tools/firmware: do not add a .note.gnu.property section

[Andrew Cooper]

On 04/04/2022 13:45, Jan Beulich wrote:
> On 04.04.2022 12:40, Roger Pau Monne wrote:
>> Prevent the assembler from creating a .note.gnu.property section on
>> the output objects, as it's not useful for firmware related binaries,
>> and breaks the resulting rombios image.
>>
>> This requires modifying the cc-option Makefile macro so it can test
>> assembler options (by replacing the usage of the -S flag with -c) and
>> also stripping the -Wa, prefix if present when checking for the test
>> output.
> I notice you've ack-ed and committed this patch, which I'm happy to
> see. However, I don't understand why you gave your ack here, when you
> did refused to ack (and to explain yourself!) for "x86emul: fix test
> harness build for gas 2.36". Why is this note section useful there
> but not similarly useful here (or, the other way around, useless)?

TBH, I'd forgotten that patch.

This work of Roger's came from a real XenServer regression which causes
RomBIOS to explode.  It needs backporting.

In the longterm, I would like to see us use real linker scripts for the
artefacts which have custom link requirements, because this is still a
bodge.

~Andrew