[PATCH for-4.15 0/3] firmware: fix build on Alpine

[PATCH for-4.15 0/3] firmware: fix build on Alpine

[Roger Pau Monne]

Hello,

While the series started as a build fix for Alpine I think they are
interesting on their own for other OSes/distros, since it allows to
remove the i386 libc as a build dependency.

THis is done by proving a suitable set of standalone headers that are
suitable for the needs of the firmware build. Patch 2 contains the full
description on why it's done this way.

The main risk for patches 1 and 2 is breaking the build in some obscure
distro/OS and toolchain combination. We aim to have this mostly covered
by gitlab CI. Patch 3 main risk is breaking the Alpine containers in
gitlab CI, but they are already failing.

Wanted to send this yesterday but was waiting on gitlab CI output, it's now
all green:

https://gitlab.com/xen-project/people/royger/xen/-/pipelines/261928726

Thanks, Roger.

Roger Pau Monne (3):
  hvmloader: do not include inttypes.h
  firmware: provide a stand alone set of headers
  automation: enable rombios build on Alpine

 README                                        |  3 --
 automation/scripts/build                      |  5 +--
 tools/firmware/Rules.mk                       | 11 ++++++
 tools/firmware/hvmloader/32bitbios_support.c  |  2 +-
 tools/firmware/include/stdarg.h               | 10 +++++
 tools/firmware/include/stdbool.h              |  9 +++++
 tools/firmware/include/stddef.h               | 10 +++++
 tools/firmware/include/stdint.h               | 39 +++++++++++++++++++
 tools/firmware/rombios/32bit/rombios_compat.h |  4 +-
 tools/firmware/rombios/rombios.c              |  5 +--
 10 files changed, 85 insertions(+), 13 deletions(-)
 create mode 100644 tools/firmware/include/stdarg.h
 create mode 100644 tools/firmware/include/stdbool.h
 create mode 100644 tools/firmware/include/stddef.h
 create mode 100644 tools/firmware/include/stdint.h

-- 
2.30.1

[PATCH for-4.15 1/3] hvmloader: do not include inttypes.h

[Roger Pau Monne]

elfstructs.h doesn't require anything from inttypes.h: it's more
appropriate to include stdint.h instead which contains the type
declarations required for the ELF types.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/firmware/hvmloader/32bitbios_support.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/32bitbios_support.c b/tools/firmware/hvmloader/32bitbios_support.c
index e726946a7b..d1ead1ec11 100644
--- a/tools/firmware/hvmloader/32bitbios_support.c
+++ b/tools/firmware/hvmloader/32bitbios_support.c
@@ -20,7 +20,7 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <inttypes.h>
+#include <stdint.h>
 #include <xen/libelf/elfstructs.h>
 #ifdef __sun__
 #include <sys/machelf.h>
-- 
2.30.1

[PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Roger Pau Monne]

The current build of the firmware relies on having 32bit compatible
headers installed in order to build some of the 32bit firmware, but
that usually requires multilib support and installing a i386 libc when
building from an amd64 environment which is cumbersome just to get
some headers.

Usually this could be solved by using the -ffreestanding compiler
option which drops the usage of the system headers in favor of a
private set of freestanding headers provided by the compiler itself
that are not tied to libc. However such option is broken at least
in the gcc compiler provided in Alpine Linux, as the system include
path (ie: /usr/include) takes precedence over the gcc private include
path:

#include <...> search starts here:
 /usr/include
 /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include

Since -ffreestanding is currently broken on at least that distro, and
for resilience against future compilers also having the option broken
provide a set of stand alone 32bit headers required for the firmware
build.

This allows to drop the build time dependency on having a i386
compatible set of libc headers on amd64.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
There's the argument of fixing gcc in Alpine and instead just use
-ffreestanding. I think that's more fragile than providing our own set
of stand alone headers for the firmware bits. Having the include paths
wrongly sorted can easily make the system headers being picked up
instead of the gcc ones, and then building can randomly fail because
the system headers could be amd64 only (like the musl ones).

I've also seen clang-9 on Debian with the following include paths:

#include "..." search starts here:
#include <...> search starts here:
 /usr/local/include
 /usr/lib/llvm-9/lib/clang/9.0.1/include
 /usr/include/x86_64-linux-gnu
 /usr/include

Which also seems slightly dangerous as local comes before the compiler
private path.

IMO using our own set of stand alone headers is more resilient.

Regarding the release risks, the main one would be breaking the build
(as it's currently broken on Alpine). I think there's a very low risk
of this change successfully producing a binary image that's broken,
and hence with enough build testing it should be safe to merge.
---
 README                                        |  3 --
 tools/firmware/Rules.mk                       | 11 ++++++
 tools/firmware/include/stdarg.h               | 10 +++++
 tools/firmware/include/stdbool.h              |  9 +++++
 tools/firmware/include/stddef.h               | 10 +++++
 tools/firmware/include/stdint.h               | 39 +++++++++++++++++++
 tools/firmware/rombios/32bit/rombios_compat.h |  4 +-
 7 files changed, 80 insertions(+), 6 deletions(-)
 create mode 100644 tools/firmware/include/stdarg.h
 create mode 100644 tools/firmware/include/stdbool.h
 create mode 100644 tools/firmware/include/stddef.h
 create mode 100644 tools/firmware/include/stdint.h

diff --git a/README b/README
index 33cdf6b826..5167bb1708 100644
--- a/README
+++ b/README
@@ -62,9 +62,6 @@ provided by your OS distributor:
     * GNU bison and GNU flex
     * GNU gettext
     * ACPI ASL compiler (iasl)
-    * Libc multiarch package (e.g. libc6-dev-i386 / glibc-devel.i686).
-      Required when building on a 64-bit platform to build
-      32-bit components which are enabled on a default build.
 
 In addition to the above there are a number of optional build
 prerequisites. Omitting these will cause the related features to be
diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
index 26bbddccd4..5d09ab06df 100644
--- a/tools/firmware/Rules.mk
+++ b/tools/firmware/Rules.mk
@@ -17,3 +17,14 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 
 # Extra CFLAGS suitable for an embedded type of environment.
 CFLAGS += -fno-builtin -msoft-float
+
+# Use our own set of library headers to build firmware.
+#
+# Ideally we would instead use -ffreestanding, but that relies on the compiler
+# having the right order for include paths (ie: compiler private headers before
+# system ones). This is not the case in Alpine at least which searches system
+# headers before compiler ones, and has been reported upstream:
+# https://gitlab.alpinelinux.org/alpine/aports/-/issues/12477
+# In the meantime (and for resilience against broken compilers) use our own set
+# of headers that provide what's needed for the firmware build.
+CFLAGS += -nostdinc -I$(XEN_ROOT)/tools/firmware/include
diff --git a/tools/firmware/include/stdarg.h b/tools/firmware/include/stdarg.h
new file mode 100644
index 0000000000..c5e3761cd2
--- /dev/null
+++ b/tools/firmware/include/stdarg.h
@@ -0,0 +1,10 @@
+#ifndef _STDARG_H_
+#define _STDARG_H_
+
+typedef __builtin_va_list va_list;
+#define va_copy(dest, src) __builtin_va_copy(dest, src)
+#define va_start(ap, last) __builtin_va_start(ap, last)
+#define va_end(ap) __builtin_va_end(ap)
+#define va_arg __builtin_va_arg
+
+#endif
diff --git a/tools/firmware/include/stdbool.h b/tools/firmware/include/stdbool.h
new file mode 100644
index 0000000000..0cf76b106c
--- /dev/null
+++ b/tools/firmware/include/stdbool.h
@@ -0,0 +1,9 @@
+#ifndef _STDBOOL_H_
+#define _STDBOOL_H_
+
+#define bool _Bool
+#define true 1
+#define false 0
+#define __bool_true_false_are_defined 1
+
+#endif
diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
new file mode 100644
index 0000000000..c7f974608a
--- /dev/null
+++ b/tools/firmware/include/stddef.h
@@ -0,0 +1,10 @@
+#ifndef _STDDEF_H_
+#define _STDDEF_H_
+
+typedef __SIZE_TYPE__ size_t;
+
+#define NULL ((void*)0)
+
+#define offsetof(t, m) __builtin_offsetof(t, m)
+
+#endif
diff --git a/tools/firmware/include/stdint.h b/tools/firmware/include/stdint.h
new file mode 100644
index 0000000000..7514096846
--- /dev/null
+++ b/tools/firmware/include/stdint.h
@@ -0,0 +1,39 @@
+#ifndef _STDINT_H_
+#define _STDINT_H_
+
+#ifdef __LP64__
+#error "32bit only header"
+#endif
+
+typedef unsigned char uint8_t;
+typedef signed char int8_t;
+
+typedef unsigned short uint16_t;
+typedef signed short int16_t;
+
+typedef unsigned int uint32_t;
+typedef signed int int32_t;
+
+typedef unsigned long long uint64_t;
+typedef signed long long int64_t;
+
+#define INT8_MIN        (-0x7f-1)
+#define INT16_MIN       (-0x7fff-1)
+#define INT32_MIN       (-0x7fffffff-1)
+#define INT64_MIN       (-0x7fffffffffffffffll-1)
+
+#define INT8_MAX        0x7f
+#define INT16_MAX       0x7fff
+#define INT32_MAX       0x7fffffff
+#define INT64_MAX       0x7fffffffffffffffll
+
+#define UINT8_MAX       0xff
+#define UINT16_MAX      0xffff
+#define UINT32_MAX      0xffffffffu
+#define UINT64_MAX      0xffffffffffffffffull
+
+typedef uint32_t uintptr_t;
+
+#define UINTPTR_MAX     UINT32_MAX
+
+#endif
diff --git a/tools/firmware/rombios/32bit/rombios_compat.h b/tools/firmware/rombios/32bit/rombios_compat.h
index 3fe7d67721..8ba4c17ffd 100644
--- a/tools/firmware/rombios/32bit/rombios_compat.h
+++ b/tools/firmware/rombios/32bit/rombios_compat.h
@@ -8,9 +8,7 @@
 
 #define ADDR_FROM_SEG_OFF(seg, off)  (void *)((((uint32_t)(seg)) << 4) + (off))
 
-typedef unsigned char uint8_t;
-typedef unsigned short int uint16_t;
-typedef unsigned int uint32_t;
+#include <stdint.h>
 
 typedef uint8_t  Bit8u;
 typedef uint16_t Bit16u;
-- 
2.30.1

[PATCH for-4.15 3/3] automation: enable rombios build on Alpine

[Roger Pau Monne]

It's now safe to enable the build of rombios on Alpine systems, as
hvmloader already builds fine there.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 automation/scripts/build | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/automation/scripts/build b/automation/scripts/build
index d8990c3bf4..87e44bb940 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -31,9 +31,8 @@ fi
 if ! test -z "$(ldd /bin/ls|grep musl|head -1)"; then
     # disable --disable-werror for QEMUU when building with MUSL
     cfgargs+=("--with-extra-qemuu-configure-args=\"--disable-werror\"")
-    # hvmloader doesn't build on MUSL systems
-    cfgargs+=("--disable-seabios")
-    cfgargs+=("--disable-rombios")
+    # SeaBIOS doesn't build on MUSL systems
+    cfgargs+=("--with-system-seabios=/bin/false")
 fi
 
 # Qemu requires Python 3.5 or later
-- 
2.30.1

Re: [PATCH for-4.15 1/3] hvmloader: do not include inttypes.h

[Jan Beulich]

On 26.02.2021 09:59, Roger Pau Monne wrote:
> elfstructs.h doesn't require anything from inttypes.h: it's more
> appropriate to include stdint.h instead which contains the type
> declarations required for the ELF types.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Let's go with Andrew's slightly larger change here.

Jan

Re: [PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Jan Beulich]

On 26.02.2021 09:59, Roger Pau Monne wrote:
> The current build of the firmware relies on having 32bit compatible
> headers installed in order to build some of the 32bit firmware, but
> that usually requires multilib support and installing a i386 libc when
> building from an amd64 environment which is cumbersome just to get
> some headers.
> 
> Usually this could be solved by using the -ffreestanding compiler
> option which drops the usage of the system headers in favor of a
> private set of freestanding headers provided by the compiler itself
> that are not tied to libc. However such option is broken at least
> in the gcc compiler provided in Alpine Linux, as the system include
> path (ie: /usr/include) takes precedence over the gcc private include
> path:
> 
> #include <...> search starts here:
>  /usr/include
>  /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
> 
> Since -ffreestanding is currently broken on at least that distro, and
> for resilience against future compilers also having the option broken
> provide a set of stand alone 32bit headers required for the firmware
> build.
> 
> This allows to drop the build time dependency on having a i386
> compatible set of libc headers on amd64.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with possibly small adjustments:

> ---
> There's the argument of fixing gcc in Alpine and instead just use
> -ffreestanding. I think that's more fragile than providing our own set
> of stand alone headers for the firmware bits. Having the include paths
> wrongly sorted can easily make the system headers being picked up
> instead of the gcc ones, and then building can randomly fail because
> the system headers could be amd64 only (like the musl ones).
> 
> I've also seen clang-9 on Debian with the following include paths:
> 
> #include "..." search starts here:
> #include <...> search starts here:
>  /usr/local/include
>  /usr/lib/llvm-9/lib/clang/9.0.1/include
>  /usr/include/x86_64-linux-gnu
>  /usr/include
> 
> Which also seems slightly dangerous as local comes before the compiler
> private path.
> 
> IMO using our own set of stand alone headers is more resilient.

I agree (in particular given the observations), but I don't view
this as an argument against use of -ffreestanding. In fact I'd
rather see this change re-based on top of Andrew's changes. Then ...

> --- a/tools/firmware/Rules.mk
> +++ b/tools/firmware/Rules.mk
> @@ -17,3 +17,14 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  
>  # Extra CFLAGS suitable for an embedded type of environment.
>  CFLAGS += -fno-builtin -msoft-float
> +
> +# Use our own set of library headers to build firmware.
> +#
> +# Ideally we would instead use -ffreestanding, but that relies on the compiler
> +# having the right order for include paths (ie: compiler private headers before
> +# system ones). This is not the case in Alpine at least which searches system
> +# headers before compiler ones, and has been reported upstream:
> +# https://gitlab.alpinelinux.org/alpine/aports/-/issues/12477
> +# In the meantime (and for resilience against broken compilers) use our own set
> +# of headers that provide what's needed for the firmware build.
> +CFLAGS += -nostdinc -I$(XEN_ROOT)/tools/firmware/include

... the initial part of the comment here would want re-wording.

> --- /dev/null
> +++ b/tools/firmware/include/stdint.h
> @@ -0,0 +1,39 @@
> +#ifndef _STDINT_H_
> +#define _STDINT_H_
> +
> +#ifdef __LP64__
> +#error "32bit only header"
> +#endif

Could I talk you into extending this to also cover __P64__? (The
alternative I see would be to omit this altogether.)

Jan

Re: [PATCH for-4.15 0/3] firmware: fix build on Alpine

[Stefano Stabellini]

On Fri, 26 Feb 2021, Roger Pau Monne wrote:
> Hello,
> 
> While the series started as a build fix for Alpine I think they are
> interesting on their own for other OSes/distros, since it allows to
> remove the i386 libc as a build dependency.
> 
> THis is done by proving a suitable set of standalone headers that are
> suitable for the needs of the firmware build. Patch 2 contains the full
> description on why it's done this way.
> 
> The main risk for patches 1 and 2 is breaking the build in some obscure
> distro/OS and toolchain combination. We aim to have this mostly covered
> by gitlab CI. Patch 3 main risk is breaking the Alpine containers in
> gitlab CI, but they are already failing.
> 
> Wanted to send this yesterday but was waiting on gitlab CI output, it's now
> all green:
> 
> https://gitlab.com/xen-project/people/royger/xen/-/pipelines/261928726

That's fantastic! Spreaking with Andrew I thought the Alpine Linux
hvmloader build issue was still unresolved, but obviously you found a
way to fix it. Great!



> Thanks, Roger.
> 
> Roger Pau Monne (3):
>   hvmloader: do not include inttypes.h
>   firmware: provide a stand alone set of headers
>   automation: enable rombios build on Alpine
> 
>  README                                        |  3 --
>  automation/scripts/build                      |  5 +--
>  tools/firmware/Rules.mk                       | 11 ++++++
>  tools/firmware/hvmloader/32bitbios_support.c  |  2 +-
>  tools/firmware/include/stdarg.h               | 10 +++++
>  tools/firmware/include/stdbool.h              |  9 +++++
>  tools/firmware/include/stddef.h               | 10 +++++
>  tools/firmware/include/stdint.h               | 39 +++++++++++++++++++
>  tools/firmware/rombios/32bit/rombios_compat.h |  4 +-
>  tools/firmware/rombios/rombios.c              |  5 +--
>  10 files changed, 85 insertions(+), 13 deletions(-)
>  create mode 100644 tools/firmware/include/stdarg.h
>  create mode 100644 tools/firmware/include/stdbool.h
>  create mode 100644 tools/firmware/include/stddef.h
>  create mode 100644 tools/firmware/include/stdint.h
> 
> -- 
> 2.30.1
> 

Re: [PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 8057 bytes --]

On Fri, 26 Feb 2021, Roger Pau Monne wrote:
> The current build of the firmware relies on having 32bit compatible
> headers installed in order to build some of the 32bit firmware, but
> that usually requires multilib support and installing a i386 libc when
> building from an amd64 environment which is cumbersome just to get
> some headers.
> 
> Usually this could be solved by using the -ffreestanding compiler
> option which drops the usage of the system headers in favor of a
> private set of freestanding headers provided by the compiler itself
> that are not tied to libc. However such option is broken at least
> in the gcc compiler provided in Alpine Linux, as the system include
> path (ie: /usr/include) takes precedence over the gcc private include
> path:
> 
> #include <...> search starts here:
>  /usr/include
>  /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
> 
> Since -ffreestanding is currently broken on at least that distro, and
> for resilience against future compilers also having the option broken
> provide a set of stand alone 32bit headers required for the firmware
> build.
> 
> This allows to drop the build time dependency on having a i386
> compatible set of libc headers on amd64.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> There's the argument of fixing gcc in Alpine and instead just use
> -ffreestanding. I think that's more fragile than providing our own set
> of stand alone headers for the firmware bits. Having the include paths
> wrongly sorted can easily make the system headers being picked up
> instead of the gcc ones, and then building can randomly fail because
> the system headers could be amd64 only (like the musl ones).
> 
> I've also seen clang-9 on Debian with the following include paths:
> 
> #include "..." search starts here:
> #include <...> search starts here:
>  /usr/local/include
>  /usr/lib/llvm-9/lib/clang/9.0.1/include
>  /usr/include/x86_64-linux-gnu
>  /usr/include
> 
> Which also seems slightly dangerous as local comes before the compiler
> private path.
> 
> IMO using our own set of stand alone headers is more resilient.
> 
> Regarding the release risks, the main one would be breaking the build
> (as it's currently broken on Alpine). I think there's a very low risk
> of this change successfully producing a binary image that's broken,
> and hence with enough build testing it should be safe to merge.

This patch is a lot nicer and smaller than I thought it would be. It
looks like the best approach.

In terms of testing, gitlab-ci has a pretty wide build test coverage, so
if we can pass those (and you have already provided a link with all
tests green in patch #0) then I am in favor of getting this in for 4.15.


> ---
>  README                                        |  3 --
>  tools/firmware/Rules.mk                       | 11 ++++++
>  tools/firmware/include/stdarg.h               | 10 +++++
>  tools/firmware/include/stdbool.h              |  9 +++++
>  tools/firmware/include/stddef.h               | 10 +++++
>  tools/firmware/include/stdint.h               | 39 +++++++++++++++++++
>  tools/firmware/rombios/32bit/rombios_compat.h |  4 +-
>  7 files changed, 80 insertions(+), 6 deletions(-)
>  create mode 100644 tools/firmware/include/stdarg.h
>  create mode 100644 tools/firmware/include/stdbool.h
>  create mode 100644 tools/firmware/include/stddef.h
>  create mode 100644 tools/firmware/include/stdint.h
> 
> diff --git a/README b/README
> index 33cdf6b826..5167bb1708 100644
> --- a/README
> +++ b/README
> @@ -62,9 +62,6 @@ provided by your OS distributor:
>      * GNU bison and GNU flex
>      * GNU gettext
>      * ACPI ASL compiler (iasl)
> -    * Libc multiarch package (e.g. libc6-dev-i386 / glibc-devel.i686).
> -      Required when building on a 64-bit platform to build
> -      32-bit components which are enabled on a default build.
>  
>  In addition to the above there are a number of optional build
>  prerequisites. Omitting these will cause the related features to be
> diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
> index 26bbddccd4..5d09ab06df 100644
> --- a/tools/firmware/Rules.mk
> +++ b/tools/firmware/Rules.mk
> @@ -17,3 +17,14 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  
>  # Extra CFLAGS suitable for an embedded type of environment.
>  CFLAGS += -fno-builtin -msoft-float
> +
> +# Use our own set of library headers to build firmware.
> +#
> +# Ideally we would instead use -ffreestanding, but that relies on the compiler
> +# having the right order for include paths (ie: compiler private headers before
> +# system ones). This is not the case in Alpine at least which searches system
> +# headers before compiler ones, and has been reported upstream:
> +# https://gitlab.alpinelinux.org/alpine/aports/-/issues/12477
> +# In the meantime (and for resilience against broken compilers) use our own set
> +# of headers that provide what's needed for the firmware build.
> +CFLAGS += -nostdinc -I$(XEN_ROOT)/tools/firmware/include
> diff --git a/tools/firmware/include/stdarg.h b/tools/firmware/include/stdarg.h
> new file mode 100644
> index 0000000000..c5e3761cd2
> --- /dev/null
> +++ b/tools/firmware/include/stdarg.h
> @@ -0,0 +1,10 @@
> +#ifndef _STDARG_H_
> +#define _STDARG_H_
> +
> +typedef __builtin_va_list va_list;
> +#define va_copy(dest, src) __builtin_va_copy(dest, src)
> +#define va_start(ap, last) __builtin_va_start(ap, last)
> +#define va_end(ap) __builtin_va_end(ap)
> +#define va_arg __builtin_va_arg
> +
> +#endif
> diff --git a/tools/firmware/include/stdbool.h b/tools/firmware/include/stdbool.h
> new file mode 100644
> index 0000000000..0cf76b106c
> --- /dev/null
> +++ b/tools/firmware/include/stdbool.h
> @@ -0,0 +1,9 @@
> +#ifndef _STDBOOL_H_
> +#define _STDBOOL_H_
> +
> +#define bool _Bool
> +#define true 1
> +#define false 0
> +#define __bool_true_false_are_defined 1
> +
> +#endif
> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
> new file mode 100644
> index 0000000000..c7f974608a
> --- /dev/null
> +++ b/tools/firmware/include/stddef.h
> @@ -0,0 +1,10 @@
> +#ifndef _STDDEF_H_
> +#define _STDDEF_H_
> +
> +typedef __SIZE_TYPE__ size_t;
> +
> +#define NULL ((void*)0)
> +
> +#define offsetof(t, m) __builtin_offsetof(t, m)
> +
> +#endif
> diff --git a/tools/firmware/include/stdint.h b/tools/firmware/include/stdint.h
> new file mode 100644
> index 0000000000..7514096846
> --- /dev/null
> +++ b/tools/firmware/include/stdint.h
> @@ -0,0 +1,39 @@
> +#ifndef _STDINT_H_
> +#define _STDINT_H_
> +
> +#ifdef __LP64__
> +#error "32bit only header"
> +#endif
> +
> +typedef unsigned char uint8_t;
> +typedef signed char int8_t;
> +
> +typedef unsigned short uint16_t;
> +typedef signed short int16_t;
> +
> +typedef unsigned int uint32_t;
> +typedef signed int int32_t;
> +
> +typedef unsigned long long uint64_t;
> +typedef signed long long int64_t;
> +
> +#define INT8_MIN        (-0x7f-1)
> +#define INT16_MIN       (-0x7fff-1)
> +#define INT32_MIN       (-0x7fffffff-1)
> +#define INT64_MIN       (-0x7fffffffffffffffll-1)
> +
> +#define INT8_MAX        0x7f
> +#define INT16_MAX       0x7fff
> +#define INT32_MAX       0x7fffffff
> +#define INT64_MAX       0x7fffffffffffffffll
> +
> +#define UINT8_MAX       0xff
> +#define UINT16_MAX      0xffff
> +#define UINT32_MAX      0xffffffffu
> +#define UINT64_MAX      0xffffffffffffffffull
> +
> +typedef uint32_t uintptr_t;
> +
> +#define UINTPTR_MAX     UINT32_MAX
> +
> +#endif
> diff --git a/tools/firmware/rombios/32bit/rombios_compat.h b/tools/firmware/rombios/32bit/rombios_compat.h
> index 3fe7d67721..8ba4c17ffd 100644
> --- a/tools/firmware/rombios/32bit/rombios_compat.h
> +++ b/tools/firmware/rombios/32bit/rombios_compat.h
> @@ -8,9 +8,7 @@
>  
>  #define ADDR_FROM_SEG_OFF(seg, off)  (void *)((((uint32_t)(seg)) << 4) + (off))
>  
> -typedef unsigned char uint8_t;
> -typedef unsigned short int uint16_t;
> -typedef unsigned int uint32_t;
> +#include <stdint.h>
>  
>  typedef uint8_t  Bit8u;
>  typedef uint16_t Bit16u;
> -- 
> 2.30.1
> 

Re: [PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Roger Pau Monné]

On Fri, Feb 26, 2021 at 02:24:43PM +0100, Jan Beulich wrote:
> On 26.02.2021 09:59, Roger Pau Monne wrote:
> > The current build of the firmware relies on having 32bit compatible
> > headers installed in order to build some of the 32bit firmware, but
> > that usually requires multilib support and installing a i386 libc when
> > building from an amd64 environment which is cumbersome just to get
> > some headers.
> > 
> > Usually this could be solved by using the -ffreestanding compiler
> > option which drops the usage of the system headers in favor of a
> > private set of freestanding headers provided by the compiler itself
> > that are not tied to libc. However such option is broken at least
> > in the gcc compiler provided in Alpine Linux, as the system include
> > path (ie: /usr/include) takes precedence over the gcc private include
> > path:
> > 
> > #include <...> search starts here:
> >  /usr/include
> >  /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
> > 
> > Since -ffreestanding is currently broken on at least that distro, and
> > for resilience against future compilers also having the option broken
> > provide a set of stand alone 32bit headers required for the firmware
> > build.
> > 
> > This allows to drop the build time dependency on having a i386
> > compatible set of libc headers on amd64.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with possibly small adjustments:
> 
> > ---
> > There's the argument of fixing gcc in Alpine and instead just use
> > -ffreestanding. I think that's more fragile than providing our own set
> > of stand alone headers for the firmware bits. Having the include paths
> > wrongly sorted can easily make the system headers being picked up
> > instead of the gcc ones, and then building can randomly fail because
> > the system headers could be amd64 only (like the musl ones).
> > 
> > I've also seen clang-9 on Debian with the following include paths:
> > 
> > #include "..." search starts here:
> > #include <...> search starts here:
> >  /usr/local/include
> >  /usr/lib/llvm-9/lib/clang/9.0.1/include
> >  /usr/include/x86_64-linux-gnu
> >  /usr/include
> > 
> > Which also seems slightly dangerous as local comes before the compiler
> > private path.
> > 
> > IMO using our own set of stand alone headers is more resilient.
> 
> I agree (in particular given the observations), but I don't view
> this as an argument against use of -ffreestanding. In fact I'd
> rather see this change re-based on top of Andrew's changes. Then ...

But doesn't using -nostdinc kind of defeats the purpose of
-ffreestanding, as it would remove all default paths from the include
search, and thus prevent using the gcc private headers?

> > --- /dev/null
> > +++ b/tools/firmware/include/stdint.h
> > @@ -0,0 +1,39 @@
> > +#ifndef _STDINT_H_
> > +#define _STDINT_H_
> > +
> > +#ifdef __LP64__
> > +#error "32bit only header"
> > +#endif
> 
> Could I talk you into extending this to also cover __P64__? (The
> alternative I see would be to omit this altogether.)

Sure. I'm having a hard time finding any documentation for __P64__
however. Does it stand for pointers are 64 bits, while longs are
32bits?

Thanks, Roger.

Re: [PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Jan Beulich]

On 01.03.2021 10:07, Roger Pau Monné wrote:
> On Fri, Feb 26, 2021 at 02:24:43PM +0100, Jan Beulich wrote:
>> On 26.02.2021 09:59, Roger Pau Monne wrote:
>>> The current build of the firmware relies on having 32bit compatible
>>> headers installed in order to build some of the 32bit firmware, but
>>> that usually requires multilib support and installing a i386 libc when
>>> building from an amd64 environment which is cumbersome just to get
>>> some headers.
>>>
>>> Usually this could be solved by using the -ffreestanding compiler
>>> option which drops the usage of the system headers in favor of a
>>> private set of freestanding headers provided by the compiler itself
>>> that are not tied to libc. However such option is broken at least
>>> in the gcc compiler provided in Alpine Linux, as the system include
>>> path (ie: /usr/include) takes precedence over the gcc private include
>>> path:
>>>
>>> #include <...> search starts here:
>>>  /usr/include
>>>  /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
>>>
>>> Since -ffreestanding is currently broken on at least that distro, and
>>> for resilience against future compilers also having the option broken
>>> provide a set of stand alone 32bit headers required for the firmware
>>> build.
>>>
>>> This allows to drop the build time dependency on having a i386
>>> compatible set of libc headers on amd64.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with possibly small adjustments:
>>
>>> ---
>>> There's the argument of fixing gcc in Alpine and instead just use
>>> -ffreestanding. I think that's more fragile than providing our own set
>>> of stand alone headers for the firmware bits. Having the include paths
>>> wrongly sorted can easily make the system headers being picked up
>>> instead of the gcc ones, and then building can randomly fail because
>>> the system headers could be amd64 only (like the musl ones).
>>>
>>> I've also seen clang-9 on Debian with the following include paths:
>>>
>>> #include "..." search starts here:
>>> #include <...> search starts here:
>>>  /usr/local/include
>>>  /usr/lib/llvm-9/lib/clang/9.0.1/include
>>>  /usr/include/x86_64-linux-gnu
>>>  /usr/include
>>>
>>> Which also seems slightly dangerous as local comes before the compiler
>>> private path.
>>>
>>> IMO using our own set of stand alone headers is more resilient.
>>
>> I agree (in particular given the observations), but I don't view
>> this as an argument against use of -ffreestanding. In fact I'd
>> rather see this change re-based on top of Andrew's changes. Then ...
> 
> But doesn't using -nostdinc kind of defeats the purpose of
> -ffreestanding, as it would remove all default paths from the include
> search, and thus prevent using the gcc private headers?

I guess I don't understand: It is the purpose of this change here to
not use compiler provided headers (nor libc provided ones), so why
would it matter to retain any kind of built-in include paths?

>>> --- /dev/null
>>> +++ b/tools/firmware/include/stdint.h
>>> @@ -0,0 +1,39 @@
>>> +#ifndef _STDINT_H_
>>> +#define _STDINT_H_
>>> +
>>> +#ifdef __LP64__
>>> +#error "32bit only header"
>>> +#endif
>>
>> Could I talk you into extending this to also cover __P64__? (The
>> alternative I see would be to omit this altogether.)
> 
> Sure. I'm having a hard time finding any documentation for __P64__
> however. Does it stand for pointers are 64 bits, while longs are
> 32bits?

Yeah, it's uncommon in Linux/Unix, but it's the model Windows uses
for 64-bit environments.

Jan

Re: [PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Roger Pau Monné]

On Mon, Mar 01, 2021 at 10:17:32AM +0100, Jan Beulich wrote:
> On 01.03.2021 10:07, Roger Pau Monné wrote:
> > On Fri, Feb 26, 2021 at 02:24:43PM +0100, Jan Beulich wrote:
> >> On 26.02.2021 09:59, Roger Pau Monne wrote:
> >>> The current build of the firmware relies on having 32bit compatible
> >>> headers installed in order to build some of the 32bit firmware, but
> >>> that usually requires multilib support and installing a i386 libc when
> >>> building from an amd64 environment which is cumbersome just to get
> >>> some headers.
> >>>
> >>> Usually this could be solved by using the -ffreestanding compiler
> >>> option which drops the usage of the system headers in favor of a
> >>> private set of freestanding headers provided by the compiler itself
> >>> that are not tied to libc. However such option is broken at least
> >>> in the gcc compiler provided in Alpine Linux, as the system include
> >>> path (ie: /usr/include) takes precedence over the gcc private include
> >>> path:
> >>>
> >>> #include <...> search starts here:
> >>>  /usr/include
> >>>  /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
> >>>
> >>> Since -ffreestanding is currently broken on at least that distro, and
> >>> for resilience against future compilers also having the option broken
> >>> provide a set of stand alone 32bit headers required for the firmware
> >>> build.
> >>>
> >>> This allows to drop the build time dependency on having a i386
> >>> compatible set of libc headers on amd64.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >> with possibly small adjustments:
> >>
> >>> ---
> >>> There's the argument of fixing gcc in Alpine and instead just use
> >>> -ffreestanding. I think that's more fragile than providing our own set
> >>> of stand alone headers for the firmware bits. Having the include paths
> >>> wrongly sorted can easily make the system headers being picked up
> >>> instead of the gcc ones, and then building can randomly fail because
> >>> the system headers could be amd64 only (like the musl ones).
> >>>
> >>> I've also seen clang-9 on Debian with the following include paths:
> >>>
> >>> #include "..." search starts here:
> >>> #include <...> search starts here:
> >>>  /usr/local/include
> >>>  /usr/lib/llvm-9/lib/clang/9.0.1/include
> >>>  /usr/include/x86_64-linux-gnu
> >>>  /usr/include
> >>>
> >>> Which also seems slightly dangerous as local comes before the compiler
> >>> private path.
> >>>
> >>> IMO using our own set of stand alone headers is more resilient.
> >>
> >> I agree (in particular given the observations), but I don't view
> >> this as an argument against use of -ffreestanding. In fact I'd
> >> rather see this change re-based on top of Andrew's changes. Then ...
> > 
> > But doesn't using -nostdinc kind of defeats the purpose of
> > -ffreestanding, as it would remove all default paths from the include
> > search, and thus prevent using the gcc private headers?
> 
> I guess I don't understand: It is the purpose of this change here to
> not use compiler provided headers (nor libc provided ones), so why
> would it matter to retain any kind of built-in include paths?

Sorry, I'm also confused.

It's my understanding that the point of using -ffreestanding is that
the compiler will set __STDC_HOSTED__ == 0, and then the built in
compiler headers will be used to provide a freestanding environment
instead of the libc ones.

However if -nostdinc is used the header search path becomes:

#include <...> search starts here:
End of search list.

At which point setting __STDC_HOSTED__ == 0 is pointless as the built
in compiler headers are not used, and hence the compiler will always
resort to the stand alone environment provided in this patch.

-ffreestanding also allows the program to have a non-standard main,
but I don't think we care much about that since we already use a custom
linker script.

Thanks, Roger.

Re: [PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Jan Beulich]

On 01.03.2021 10:50, Roger Pau Monné wrote:
> On Mon, Mar 01, 2021 at 10:17:32AM +0100, Jan Beulich wrote:
>> On 01.03.2021 10:07, Roger Pau Monné wrote:
>>> On Fri, Feb 26, 2021 at 02:24:43PM +0100, Jan Beulich wrote:
>>>> On 26.02.2021 09:59, Roger Pau Monne wrote:
>>>>> The current build of the firmware relies on having 32bit compatible
>>>>> headers installed in order to build some of the 32bit firmware, but
>>>>> that usually requires multilib support and installing a i386 libc when
>>>>> building from an amd64 environment which is cumbersome just to get
>>>>> some headers.
>>>>>
>>>>> Usually this could be solved by using the -ffreestanding compiler
>>>>> option which drops the usage of the system headers in favor of a
>>>>> private set of freestanding headers provided by the compiler itself
>>>>> that are not tied to libc. However such option is broken at least
>>>>> in the gcc compiler provided in Alpine Linux, as the system include
>>>>> path (ie: /usr/include) takes precedence over the gcc private include
>>>>> path:
>>>>>
>>>>> #include <...> search starts here:
>>>>>  /usr/include
>>>>>  /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
>>>>>
>>>>> Since -ffreestanding is currently broken on at least that distro, and
>>>>> for resilience against future compilers also having the option broken
>>>>> provide a set of stand alone 32bit headers required for the firmware
>>>>> build.
>>>>>
>>>>> This allows to drop the build time dependency on having a i386
>>>>> compatible set of libc headers on amd64.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> with possibly small adjustments:
>>>>
>>>>> ---
>>>>> There's the argument of fixing gcc in Alpine and instead just use
>>>>> -ffreestanding. I think that's more fragile than providing our own set
>>>>> of stand alone headers for the firmware bits. Having the include paths
>>>>> wrongly sorted can easily make the system headers being picked up
>>>>> instead of the gcc ones, and then building can randomly fail because
>>>>> the system headers could be amd64 only (like the musl ones).
>>>>>
>>>>> I've also seen clang-9 on Debian with the following include paths:
>>>>>
>>>>> #include "..." search starts here:
>>>>> #include <...> search starts here:
>>>>>  /usr/local/include
>>>>>  /usr/lib/llvm-9/lib/clang/9.0.1/include
>>>>>  /usr/include/x86_64-linux-gnu
>>>>>  /usr/include
>>>>>
>>>>> Which also seems slightly dangerous as local comes before the compiler
>>>>> private path.
>>>>>
>>>>> IMO using our own set of stand alone headers is more resilient.
>>>>
>>>> I agree (in particular given the observations), but I don't view
>>>> this as an argument against use of -ffreestanding. In fact I'd
>>>> rather see this change re-based on top of Andrew's changes. Then ...
>>>
>>> But doesn't using -nostdinc kind of defeats the purpose of
>>> -ffreestanding, as it would remove all default paths from the include
>>> search, and thus prevent using the gcc private headers?
>>
>> I guess I don't understand: It is the purpose of this change here to
>> not use compiler provided headers (nor libc provided ones), so why
>> would it matter to retain any kind of built-in include paths?
> 
> Sorry, I'm also confused.
> 
> It's my understanding that the point of using -ffreestanding is that
> the compiler will set __STDC_HOSTED__ == 0, and then the built in
> compiler headers will be used to provide a freestanding environment
> instead of the libc ones.
> 
> However if -nostdinc is used the header search path becomes:
> 
> #include <...> search starts here:
> End of search list.
> 
> At which point setting __STDC_HOSTED__ == 0 is pointless as the built
> in compiler headers are not used, and hence the compiler will always
> resort to the stand alone environment provided in this patch.
> 
> -ffreestanding also allows the program to have a non-standard main,
> but I don't think we care much about that since we already use a custom
> linker script.

While indeed we don't care about this specific last aspect, we
do e.g. care about the implied -fno-builtin (which currently we
specify explicitly, yes). So while with -nostdinc added we
_might_ indeed be fine already, I think we're better off going
the full step and specify what we mean, even if - right now -
we're unaware of further effects which are relevant to us. (For
example, I don't see why in principle we couldn't ourselves
grow a use of __STDC_HOSTED__ somewhere.)

Jan

Re: [PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Roger Pau Monné]

On Mon, Mar 01, 2021 at 11:05:17AM +0100, Jan Beulich wrote:
> On 01.03.2021 10:50, Roger Pau Monné wrote:
> > On Mon, Mar 01, 2021 at 10:17:32AM +0100, Jan Beulich wrote:
> >> On 01.03.2021 10:07, Roger Pau Monné wrote:
> >>> On Fri, Feb 26, 2021 at 02:24:43PM +0100, Jan Beulich wrote:
> >>>> On 26.02.2021 09:59, Roger Pau Monne wrote:
> >>>>> The current build of the firmware relies on having 32bit compatible
> >>>>> headers installed in order to build some of the 32bit firmware, but
> >>>>> that usually requires multilib support and installing a i386 libc when
> >>>>> building from an amd64 environment which is cumbersome just to get
> >>>>> some headers.
> >>>>>
> >>>>> Usually this could be solved by using the -ffreestanding compiler
> >>>>> option which drops the usage of the system headers in favor of a
> >>>>> private set of freestanding headers provided by the compiler itself
> >>>>> that are not tied to libc. However such option is broken at least
> >>>>> in the gcc compiler provided in Alpine Linux, as the system include
> >>>>> path (ie: /usr/include) takes precedence over the gcc private include
> >>>>> path:
> >>>>>
> >>>>> #include <...> search starts here:
> >>>>>  /usr/include
> >>>>>  /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
> >>>>>
> >>>>> Since -ffreestanding is currently broken on at least that distro, and
> >>>>> for resilience against future compilers also having the option broken
> >>>>> provide a set of stand alone 32bit headers required for the firmware
> >>>>> build.
> >>>>>
> >>>>> This allows to drop the build time dependency on having a i386
> >>>>> compatible set of libc headers on amd64.
> >>>>>
> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>>>
> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>>> with possibly small adjustments:
> >>>>
> >>>>> ---
> >>>>> There's the argument of fixing gcc in Alpine and instead just use
> >>>>> -ffreestanding. I think that's more fragile than providing our own set
> >>>>> of stand alone headers for the firmware bits. Having the include paths
> >>>>> wrongly sorted can easily make the system headers being picked up
> >>>>> instead of the gcc ones, and then building can randomly fail because
> >>>>> the system headers could be amd64 only (like the musl ones).
> >>>>>
> >>>>> I've also seen clang-9 on Debian with the following include paths:
> >>>>>
> >>>>> #include "..." search starts here:
> >>>>> #include <...> search starts here:
> >>>>>  /usr/local/include
> >>>>>  /usr/lib/llvm-9/lib/clang/9.0.1/include
> >>>>>  /usr/include/x86_64-linux-gnu
> >>>>>  /usr/include
> >>>>>
> >>>>> Which also seems slightly dangerous as local comes before the compiler
> >>>>> private path.
> >>>>>
> >>>>> IMO using our own set of stand alone headers is more resilient.
> >>>>
> >>>> I agree (in particular given the observations), but I don't view
> >>>> this as an argument against use of -ffreestanding. In fact I'd
> >>>> rather see this change re-based on top of Andrew's changes. Then ...
> >>>
> >>> But doesn't using -nostdinc kind of defeats the purpose of
> >>> -ffreestanding, as it would remove all default paths from the include
> >>> search, and thus prevent using the gcc private headers?
> >>
> >> I guess I don't understand: It is the purpose of this change here to
> >> not use compiler provided headers (nor libc provided ones), so why
> >> would it matter to retain any kind of built-in include paths?
> > 
> > Sorry, I'm also confused.
> > 
> > It's my understanding that the point of using -ffreestanding is that
> > the compiler will set __STDC_HOSTED__ == 0, and then the built in
> > compiler headers will be used to provide a freestanding environment
> > instead of the libc ones.
> > 
> > However if -nostdinc is used the header search path becomes:
> > 
> > #include <...> search starts here:
> > End of search list.
> > 
> > At which point setting __STDC_HOSTED__ == 0 is pointless as the built
> > in compiler headers are not used, and hence the compiler will always
> > resort to the stand alone environment provided in this patch.
> > 
> > -ffreestanding also allows the program to have a non-standard main,
> > but I don't think we care much about that since we already use a custom
> > linker script.
> 
> While indeed we don't care about this specific last aspect, we
> do e.g. care about the implied -fno-builtin (which currently we
> specify explicitly, yes). So while with -nostdinc added we
> _might_ indeed be fine already, I think we're better off going
> the full step and specify what we mean, even if - right now -
> we're unaware of further effects which are relevant to us. (For
> example, I don't see why in principle we couldn't ourselves
> grow a use of __STDC_HOSTED__ somewhere.)

OK I will rebase once Andrew posts an updated version and adjust the
commit message and comment to notice what we have discussed above.

Thanks, Roger.

[PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Ian Jackson]

Roger Pau Monne writes ("[PATCH for-4.15 2/3] firmware: provide a stand alone set of headers"):
> The current build of the firmware relies on having 32bit compatible
> headers installed in order to build some of the 32bit firmware, but
> that usually requires multilib support and installing a i386 libc when
> building from an amd64 environment which is cumbersome just to get
> some headers.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>