[PATCH] firmware: don't build hvmloader if it is not needed

[PATCH] firmware: don't build hvmloader if it is not needed

[Stefano Stabellini]

If rombios, seabios and ovmf are all disabled, don't attempt to build
hvmloader.

This patches fixes the x86 alpine linux builds currently failing in
gitlab-ci.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 tools/firmware/Makefile | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 1f27117794..e68cd0d358 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -13,7 +13,16 @@ SUBDIRS-$(CONFIG_ROMBIOS) += rombios
 SUBDIRS-$(CONFIG_ROMBIOS) += vgabios
 SUBDIRS-$(CONFIG_IPXE) += etherboot
 SUBDIRS-$(CONFIG_PV_SHIM) += xen-dir
-SUBDIRS-y += hvmloader
+ifeq ($(CONFIG_ROMBIOS),y)
+CONFIG_HVMLOADER ?= y
+endif
+ifeq ($(CONFIG_SEABIOS),y)
+CONFIG_HVMLOADER ?= y
+endif
+ifeq ($(CONFIG_OVMF),y)
+CONFIG_HVMLOADER ?= y
+endif
+SUBDIRS-$(CONFIG_HVMLOADER) += hvmloader
 
 SEABIOSCC ?= $(CC)
 SEABIOSLD ?= $(LD)
-- 
2.17.1

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Marek Marczykowski-Górecki]

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

On Fri, Feb 12, 2021 at 06:05:40PM -0800, Stefano Stabellini wrote:
> If rombios, seabios and ovmf are all disabled, don't attempt to build
> hvmloader.

What if you choose to not build any of rombios, seabios, ovmf, but use
system one instead? Wouldn't that exclude hvmloader too?

This heuristic seems like a bit too much, maybe instead add an explicit
option to skip hvmloader?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Jan Beulich]

On 13.02.2021 14:50, Marek Marczykowski-Górecki wrote:
> On Fri, Feb 12, 2021 at 06:05:40PM -0800, Stefano Stabellini wrote:
>> If rombios, seabios and ovmf are all disabled, don't attempt to build
>> hvmloader.
> 
> What if you choose to not build any of rombios, seabios, ovmf, but use
> system one instead? Wouldn't that exclude hvmloader too?

Even further - one can disable all firmware and have every guest
config explicitly specify the firmware to use, afaict.

> This heuristic seems like a bit too much, maybe instead add an explicit
> option to skip hvmloader?

+1 (If making this configurable is needed at all - is having
hvmloader without needing it really a problem?)

Jan

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Stefano Stabellini]

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

On Mon, 15 Feb 2021, Jan Beulich wrote:
> On 13.02.2021 14:50, Marek Marczykowski-Górecki wrote:
> > On Fri, Feb 12, 2021 at 06:05:40PM -0800, Stefano Stabellini wrote:
> >> If rombios, seabios and ovmf are all disabled, don't attempt to build
> >> hvmloader.
> > 
> > What if you choose to not build any of rombios, seabios, ovmf, but use
> > system one instead? Wouldn't that exclude hvmloader too?
> 
> Even further - one can disable all firmware and have every guest
> config explicitly specify the firmware to use, afaict.

I didn't realize there was a valid reason for wanting to build hvmloader
without rombios, seabios, and ovfm.


> > This heuristic seems like a bit too much, maybe instead add an explicit
> > option to skip hvmloader?
> 
> +1 (If making this configurable is needed at all - is having
> hvmloader without needing it really a problem?)

The hvmloader build fails on Alpine Linux x86:

https://gitlab.com/xen-project/xen/-/jobs/1033722465



I admit I was just trying to find the fastest way to make those Alpine
Linux builds succeed to unblock patchew: although the Alpine Linux
builds are marked as "allow_failure: true" in gitlab-ci, patchew will
still report the whole battery of tests as "failure". As a consequence
the notification emails from patchew after a build of a contributed
patch series always says "job failed" today, making it kind of useless.
See attached.

I would love if somebody else took over this fix as I am doing this
after hours, but if you have a simple suggestion on how to fix the
Alpine Linux hvmloader builds, or skip the build when appropriate, I can
try to follow up.

Otherwise for now it might be best to just temporarily remove the Alpine
Linux x86 builds from gitlab-ci.

Any thoughts?

[-- Attachment #2: Type: text/plain, Size: 2786 bytes --]

From: no-reply@patchew.org
To: famzheng@amazon.com
Cc: sstabellini@kernel.org,, cardoe@cardoe.com,, wl@xen.org,, Bertrand.Marquis@arm.com,, julien@xen.org,, andrew.cooper3@citrix.com
Date: Thu, 11 Feb 2021 05:03:50 -0800 (PST)

Hi,

Patchew automatically ran gitlab-ci pipeline with this patch (series) applied, but the job failed. Maybe there's a bug in the patches?

You can find the link to the pipeline near the end of the report below:

Type: series
Message-id: 20210210092211.53359-1-roger.pau@citrix.com
Subject: [PATCH] x86/irq: simplify loop in unmap_domain_pirq

=== TEST SCRIPT BEGIN ===
#!/bin/bash
sleep 10
patchew gitlab-pipeline-check -p xen-project/patchew/xen
=== TEST SCRIPT END ===

warning: redirecting to https://gitlab.com/xen-project/patchew/xen.git/
warning: redirecting to https://gitlab.com/xen-project/patchew/xen.git/
From https://gitlab.com/xen-project/patchew/xen
 * [new tag]               patchew/20210210092211.53359-1-roger.pau@citrix.com -> patchew/20210210092211.53359-1-roger.pau@citrix.com
Switched to a new branch 'test'
58b36b77d5 x86/irq: simplify loop in unmap_domain_pirq

=== OUTPUT BEGIN ===
[2021-02-11 11:10:07] Looking up pipeline...
[2021-02-11 11:10:07] Found pipeline 254760433:

https://gitlab.com/xen-project/patchew/xen/-/pipelines/254760433

[2021-02-11 11:10:07] Waiting for pipeline to finish...
[2021-02-11 11:25:12] Still waiting...
[2021-02-11 11:40:17] Still waiting...
[2021-02-11 11:55:21] Still waiting...
[2021-02-11 12:10:29] Still waiting...
[2021-02-11 12:25:35] Still waiting...
[2021-02-11 12:40:40] Still waiting...
[2021-02-11 12:55:45] Still waiting...
[2021-02-11 13:03:48] Pipeline failed
[2021-02-11 13:03:48] Job 'alpine-3.12-gcc-debug-arm64' in stage 'build' is failed
[2021-02-11 13:03:48] Job 'alpine-3.12-gcc-arm64' in stage 'build' is failed
[2021-02-11 13:03:48] Job 'qemu-smoke-x86-64-clang-pvh' in stage 'test' is skipped
[2021-02-11 13:03:48] Job 'qemu-smoke-x86-64-gcc-pvh' in stage 'test' is skipped
[2021-02-11 13:03:48] Job 'qemu-smoke-x86-64-clang' in stage 'test' is skipped
[2021-02-11 13:03:48] Job 'qemu-smoke-x86-64-gcc' in stage 'test' is skipped
[2021-02-11 13:03:48] Job 'qemu-smoke-arm64-gcc' in stage 'test' is skipped
[2021-02-11 13:03:48] Job 'qemu-alpine-arm64-gcc' in stage 'test' is skipped
[2021-02-11 13:03:48] Job 'build-each-commit-gcc' in stage 'test' is skipped
[2021-02-11 13:03:48] Job 'alpine-3.12-clang-debug' in stage 'build' is failed
[2021-02-11 13:03:48] Job 'alpine-3.12-clang' in stage 'build' is failed
[2021-02-11 13:03:48] Job 'alpine-3.12-gcc-debug' in stage 'build' is failed
[2021-02-11 13:03:48] Job 'alpine-3.12-gcc' in stage 'build' is failed
=== OUTPUT END ===

Test command exited with code: 1

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Jan Beulich]

On 16.02.2021 19:31, Stefano Stabellini wrote:
> On Mon, 15 Feb 2021, Jan Beulich wrote:
>> On 13.02.2021 14:50, Marek Marczykowski-Górecki wrote:
>>> On Fri, Feb 12, 2021 at 06:05:40PM -0800, Stefano Stabellini wrote:
>>>> If rombios, seabios and ovmf are all disabled, don't attempt to build
>>>> hvmloader.
>>>
>>> What if you choose to not build any of rombios, seabios, ovmf, but use
>>> system one instead? Wouldn't that exclude hvmloader too?
>>
>> Even further - one can disable all firmware and have every guest
>> config explicitly specify the firmware to use, afaict.
> 
> I didn't realize there was a valid reason for wanting to build hvmloader
> without rombios, seabios, and ovfm.
> 
> 
>>> This heuristic seems like a bit too much, maybe instead add an explicit
>>> option to skip hvmloader?
>>
>> +1 (If making this configurable is needed at all - is having
>> hvmloader without needing it really a problem?)
> 
> The hvmloader build fails on Alpine Linux x86:
> 
> https://gitlab.com/xen-project/xen/-/jobs/1033722465
> 
> 
> 
> I admit I was just trying to find the fastest way to make those Alpine
> Linux builds succeed to unblock patchew: although the Alpine Linux
> builds are marked as "allow_failure: true" in gitlab-ci, patchew will
> still report the whole battery of tests as "failure". As a consequence
> the notification emails from patchew after a build of a contributed
> patch series always says "job failed" today, making it kind of useless.
> See attached.
> 
> I would love if somebody else took over this fix as I am doing this
> after hours, but if you have a simple suggestion on how to fix the
> Alpine Linux hvmloader builds, or skip the build when appropriate, I can
> try to follow up.

There is an issue with the definition of uint64_t there. Initial
errors like

hvmloader.c: In function 'init_vm86_tss':
hvmloader.c:202:39: error: left shift count >= width of type [-Werror=shift-count-overflow]
  202 |                   ((uint64_t)TSS_SIZE << 32) | virt_to_phys(tss));

already hint at this, but then

util.c: In function 'get_cpu_mhz':
util.c:824:15: error: conversion from 'long long unsigned int' to 'uint64_t' {aka 'long unsigned int'} changes value from '4294967296000000' to '0' [-Werror=overflow]
  824 |     cpu_khz = 1000000ull << 32;

is quite explicit: "aka 'long unsigned int'"? This is a 32-bit
environment, after all. I suspect the build picks up headers
(stdint.h here in particular) intended for 64-bit builds only.
Can you check whether "gcc -m32" properly sets include paths
_different_ from those plain "gcc" sets, if the headers found in
the latter case aren't suitable for the former? Or alternatively
is the Alpine build environment set up incorrectly, in that it
lacks 32-bit devel packages?

As an aside I don't think it's really a good idea to have
hvmloader depend on any external headers. Just like the
hypervisor it's a free-standing binary, and hence ought to be
free of any dependencies on the build/host environment.

Jan

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Stefano Stabellini]

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

On Wed, 17 Feb 2021, Jan Beulich wrote:
> On 16.02.2021 19:31, Stefano Stabellini wrote:
> > On Mon, 15 Feb 2021, Jan Beulich wrote:
> >> On 13.02.2021 14:50, Marek Marczykowski-Górecki wrote:
> >>> On Fri, Feb 12, 2021 at 06:05:40PM -0800, Stefano Stabellini wrote:
> >>>> If rombios, seabios and ovmf are all disabled, don't attempt to build
> >>>> hvmloader.
> >>>
> >>> What if you choose to not build any of rombios, seabios, ovmf, but use
> >>> system one instead? Wouldn't that exclude hvmloader too?
> >>
> >> Even further - one can disable all firmware and have every guest
> >> config explicitly specify the firmware to use, afaict.
> > 
> > I didn't realize there was a valid reason for wanting to build hvmloader
> > without rombios, seabios, and ovfm.
> > 
> > 
> >>> This heuristic seems like a bit too much, maybe instead add an explicit
> >>> option to skip hvmloader?
> >>
> >> +1 (If making this configurable is needed at all - is having
> >> hvmloader without needing it really a problem?)
> > 
> > The hvmloader build fails on Alpine Linux x86:
> > 
> > https://gitlab.com/xen-project/xen/-/jobs/1033722465
> > 
> > 
> > 
> > I admit I was just trying to find the fastest way to make those Alpine
> > Linux builds succeed to unblock patchew: although the Alpine Linux
> > builds are marked as "allow_failure: true" in gitlab-ci, patchew will
> > still report the whole battery of tests as "failure". As a consequence
> > the notification emails from patchew after a build of a contributed
> > patch series always says "job failed" today, making it kind of useless.
> > See attached.
> > 
> > I would love if somebody else took over this fix as I am doing this
> > after hours, but if you have a simple suggestion on how to fix the
> > Alpine Linux hvmloader builds, or skip the build when appropriate, I can
> > try to follow up.
> 
> There is an issue with the definition of uint64_t there. Initial
> errors like
> 
> hvmloader.c: In function 'init_vm86_tss':
> hvmloader.c:202:39: error: left shift count >= width of type [-Werror=shift-count-overflow]
>   202 |                   ((uint64_t)TSS_SIZE << 32) | virt_to_phys(tss));
> 
> already hint at this, but then
> 
> util.c: In function 'get_cpu_mhz':
> util.c:824:15: error: conversion from 'long long unsigned int' to 'uint64_t' {aka 'long unsigned int'} changes value from '4294967296000000' to '0' [-Werror=overflow]
>   824 |     cpu_khz = 1000000ull << 32;
> 
> is quite explicit: "aka 'long unsigned int'"? This is a 32-bit
> environment, after all. I suspect the build picks up headers
> (stdint.h here in particular) intended for 64-bit builds only.
> Can you check whether "gcc -m32" properly sets include paths
> _different_ from those plain "gcc" sets, if the headers found in
> the latter case aren't suitable for the former? Or alternatively
> is the Alpine build environment set up incorrectly, in that it
> lacks 32-bit devel packages?
> 
> As an aside I don't think it's really a good idea to have
> hvmloader depend on any external headers. Just like the
> hypervisor it's a free-standing binary, and hence ought to be
> free of any dependencies on the build/host environment.

All the automation containers are available for anybody to use, so FYI
you can repro the issue by doing inside your Xen repo:

docker run -it -v `pwd`:/build registry.gitlab.com/xen-project/xen/alpine:3.12
CC=gcc bash automation/scripts/build


I did just that and ran a simple test:

~ # gcc -m32 test.c -o test
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: skipping incompatible /usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../libssp_nonshared.a when searching for -lssp_nonshared
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: skipping incompatible /usr/lib/libssp_nonshared.a when searching for -lssp_nonshared
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: cannot find -lssp_nonshared
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: skipping incompatible /usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/libgcc.a when searching for -lgcc
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: cannot find -lgcc
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: skipping incompatible /usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../libgcc_s.so.1 when searching for libgcc_s.so.1
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: skipping incompatible /usr/lib/libgcc_s.so.1 when searching for libgcc_s.so.1
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: cannot find libgcc_s.so.1
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: skipping incompatible /usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/libgcc.a when searching for -lgcc
/usr/lib/gcc/x86_64-alpine-linux-musl/9.3.0/../../../../x86_64-alpine-linux-musl/bin/ld: cannot find -lgcc
collect2: error: ld returned 1 exit status
~ # cat test.c 
#include <stdio.h>

int main() {
    printf("DEBUG\n");
}


Given this, I take there is no 32bit build env? A bit of Googling tells
me that gcc on Alpine Linux is compiled without multilib support.


That said I was looking at the Alpine Linux APKBUILD script:
https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/xen/APKBUILD

And I noticed this patch that looks suspicious:
https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/xen/musl-hvmloader-fix-stdint.patch

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Jan Beulich]

On 18.02.2021 00:45, Stefano Stabellini wrote:
> Given this, I take there is no 32bit build env? A bit of Googling tells
> me that gcc on Alpine Linux is compiled without multilib support.
> 
> 
> That said I was looking at the Alpine Linux APKBUILD script:
> https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/xen/APKBUILD
> 
> And I noticed this patch that looks suspicious:
> https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/xen/musl-hvmloader-fix-stdint.patch

Indeed. I find it very odd that they have a bimodal gcc (allowing
-m32) but no suitable further infrastructure (headers). So perhaps
configure should probe for "gcc -m32" producing a uint64_t that is
actually 64 bits wide, and disable hvmloader building otherwise
(and - important - no matter whether it would actually be needed;
alternative being to fail configuring altogether)? Until - as said
before - we've made hvmloader properly freestanding.

Jan

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Stefano Stabellini]

On Thu, 18 Feb 2021, Jan Beulich wrote:
> On 18.02.2021 00:45, Stefano Stabellini wrote:
> > Given this, I take there is no 32bit build env? A bit of Googling tells
> > me that gcc on Alpine Linux is compiled without multilib support.
> > 
> > 
> > That said I was looking at the Alpine Linux APKBUILD script:
> > https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/xen/APKBUILD
> > 
> > And I noticed this patch that looks suspicious:
> > https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/xen/musl-hvmloader-fix-stdint.patch
> 
> Indeed. I find it very odd that they have a bimodal gcc (allowing
> -m32) but no suitable further infrastructure (headers). So perhaps
> configure should probe for "gcc -m32" producing a uint64_t that is
> actually 64 bits wide, and disable hvmloader building otherwise
> (and - important - no matter whether it would actually be needed;
> alternative being to fail configuring altogether)? Until - as said
> before - we've made hvmloader properly freestanding.

OK it took me a lot longer than expected (I have never had the dubious
pleasure of working with autoconf before) but the following seems to
work, tested on both Alpine Linux and Debian Unstable. Of course I had
to run autoreconf first.


diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 48bd9ab731..d5e4f1679f 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -50,6 +50,7 @@ CONFIG_OVMF         := @ovmf@
 CONFIG_ROMBIOS      := @rombios@
 CONFIG_SEABIOS      := @seabios@
 CONFIG_IPXE         := @ipxe@
+CONFIG_HVMLOADER    := @hvmloader@
 CONFIG_QEMU_TRAD    := @qemu_traditional@
 CONFIG_QEMU_XEN     := @qemu_xen@
 CONFIG_QEMUU_EXTRA_ARGS:= @EXTRA_QEMUU_CONFIGURE_ARGS@
diff --git a/tools/Makefile b/tools/Makefile
index 757a560be0..6cff5766f3 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -14,7 +14,7 @@ SUBDIRS-y += examples
 SUBDIRS-y += hotplug
 SUBDIRS-y += xentrace
 SUBDIRS-$(CONFIG_XCUTILS) += xcutils
-SUBDIRS-$(CONFIG_X86) += firmware
+SUBDIRS-$(CONFIG_HVMLOADER) += firmware
 SUBDIRS-y += console
 SUBDIRS-y += xenmon
 SUBDIRS-y += xentop
diff --git a/tools/configure.ac b/tools/configure.ac
index 6b611deb13..a3a52cec41 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -307,6 +307,10 @@ AC_ARG_VAR([AWK], [Path to awk tool])
 
 # Checks for programs.
 AC_PROG_CC
+AC_LANG(C)
+AC_LANG_CONFTEST([AC_LANG_SOURCE([[int main() { return 0;}]])])
+AS_IF([gcc -m32 conftest.c -o - 2>/dev/null], [hvmloader=y], [AC_MSG_WARN(hvmloader build disabled as the compiler cannot build 32bit binaries)])
+AC_SUBST(hvmloader)
 AC_PROG_MAKE_SET
 AC_PROG_INSTALL
 AC_PATH_PROG([FLEX], [flex])

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Jan Beulich]

On 19.02.2021 02:42, Stefano Stabellini wrote:
> OK it took me a lot longer than expected (I have never had the dubious
> pleasure of working with autoconf before) but the following seems to
> work, tested on both Alpine Linux and Debian Unstable. Of course I had
> to run autoreconf first.
> 
> 
> diff --git a/config/Tools.mk.in b/config/Tools.mk.in
> index 48bd9ab731..d5e4f1679f 100644
> --- a/config/Tools.mk.in
> +++ b/config/Tools.mk.in
> @@ -50,6 +50,7 @@ CONFIG_OVMF         := @ovmf@
>  CONFIG_ROMBIOS      := @rombios@
>  CONFIG_SEABIOS      := @seabios@
>  CONFIG_IPXE         := @ipxe@
> +CONFIG_HVMLOADER    := @hvmloader@
>  CONFIG_QEMU_TRAD    := @qemu_traditional@
>  CONFIG_QEMU_XEN     := @qemu_xen@
>  CONFIG_QEMUU_EXTRA_ARGS:= @EXTRA_QEMUU_CONFIGURE_ARGS@
> diff --git a/tools/Makefile b/tools/Makefile
> index 757a560be0..6cff5766f3 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -14,7 +14,7 @@ SUBDIRS-y += examples
>  SUBDIRS-y += hotplug
>  SUBDIRS-y += xentrace
>  SUBDIRS-$(CONFIG_XCUTILS) += xcutils
> -SUBDIRS-$(CONFIG_X86) += firmware
> +SUBDIRS-$(CONFIG_HVMLOADER) += firmware

But there are more subdirs under firmware/ than just hvmloader.
In particular you'd now also skip building the shim if hvmloader
was disabled.

> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -307,6 +307,10 @@ AC_ARG_VAR([AWK], [Path to awk tool])
>  
>  # Checks for programs.
>  AC_PROG_CC
> +AC_LANG(C)
> +AC_LANG_CONFTEST([AC_LANG_SOURCE([[int main() { return 0;}]])])
> +AS_IF([gcc -m32 conftest.c -o - 2>/dev/null], [hvmloader=y], [AC_MSG_WARN(hvmloader build disabled as the compiler cannot build 32bit binaries)])
> +AC_SUBST(hvmloader)

I'm puzzled: "gcc -m32" looked to work fine on its own. I suppose
the above fails at the linking stage, but that's not what we care
about (we don't link with any system libraries). Instead, as said,
you want to check "gcc -m32 -c" produces correct code, in
particular with sizeof(uint64_t) being 8. Of course all of this
would be easier if their headers at least caused some form of
error, instead of silently causing bad code to be generated.

The way you do it, someone simply not having 32-bit C libraries
installed would then also have hvmloader build disabled, even if
their compiler and headers are fine to use.

Also I don't think "-o -" does what you want - it'll produce a
binary named "-" (if compilation and linking succeed), while I
suppose what you want is to discard the output (i.e. probably
"-o /dev/null"). Albeit even that doesn't look to be the commonly
used approach - a binary named "conftest" would normally be
specified as the output, according to other configure.ac I've
looked at. Such tests then have a final "rm -f conftest*".

Jan

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Stefano Stabellini]

On Fri, 19 Feb 2021, Jan Beulich wrote:
> On 19.02.2021 02:42, Stefano Stabellini wrote:
> > OK it took me a lot longer than expected (I have never had the dubious
> > pleasure of working with autoconf before) but the following seems to
> > work, tested on both Alpine Linux and Debian Unstable. Of course I had
> > to run autoreconf first.
> > 
> > 
> > diff --git a/config/Tools.mk.in b/config/Tools.mk.in
> > index 48bd9ab731..d5e4f1679f 100644
> > --- a/config/Tools.mk.in
> > +++ b/config/Tools.mk.in
> > @@ -50,6 +50,7 @@ CONFIG_OVMF         := @ovmf@
> >  CONFIG_ROMBIOS      := @rombios@
> >  CONFIG_SEABIOS      := @seabios@
> >  CONFIG_IPXE         := @ipxe@
> > +CONFIG_HVMLOADER    := @hvmloader@
> >  CONFIG_QEMU_TRAD    := @qemu_traditional@
> >  CONFIG_QEMU_XEN     := @qemu_xen@
> >  CONFIG_QEMUU_EXTRA_ARGS:= @EXTRA_QEMUU_CONFIGURE_ARGS@
> > diff --git a/tools/Makefile b/tools/Makefile
> > index 757a560be0..6cff5766f3 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -14,7 +14,7 @@ SUBDIRS-y += examples
> >  SUBDIRS-y += hotplug
> >  SUBDIRS-y += xentrace
> >  SUBDIRS-$(CONFIG_XCUTILS) += xcutils
> > -SUBDIRS-$(CONFIG_X86) += firmware
> > +SUBDIRS-$(CONFIG_HVMLOADER) += firmware
> 
> But there are more subdirs under firmware/ than just hvmloader.
> In particular you'd now also skip building the shim if hvmloader
> was disabled.
> 
> > --- a/tools/configure.ac
> > +++ b/tools/configure.ac
> > @@ -307,6 +307,10 @@ AC_ARG_VAR([AWK], [Path to awk tool])
> >  
> >  # Checks for programs.
> >  AC_PROG_CC
> > +AC_LANG(C)
> > +AC_LANG_CONFTEST([AC_LANG_SOURCE([[int main() { return 0;}]])])
> > +AS_IF([gcc -m32 conftest.c -o - 2>/dev/null], [hvmloader=y], [AC_MSG_WARN(hvmloader build disabled as the compiler cannot build 32bit binaries)])
> > +AC_SUBST(hvmloader)
> 
> I'm puzzled: "gcc -m32" looked to work fine on its own. I suppose
> the above fails at the linking stage, but that's not what we care
> about (we don't link with any system libraries). Instead, as said,
> you want to check "gcc -m32 -c" produces correct code, in
> particular with sizeof(uint64_t) being 8. Of course all of this
> would be easier if their headers at least caused some form of
> error, instead of silently causing bad code to be generated.
> 
> The way you do it, someone simply not having 32-bit C libraries
> installed would then also have hvmloader build disabled, even if
> their compiler and headers are fine to use.

I realize that technically this test is probing for something different:
the ability to build and link a trivial 32-bit userspace program, rather
than a specific check about sizeof(uint64_t). However, I thought that if
this test failed we didn't want to continue anyway.

If you say that hvmloader doesn't link against any system libraries,
then in theory the hvmloader build could succeed even if this test
failed. Hence, we need to change strategy.

What do you think of something like this?

AC_LANG_CONFTEST([AC_LANG_SOURCE([[#include <assert.h>
#include <stdint.h>
int main() { assert(sizeof(uint64_t) == 8); return 0;}]])])
AS_IF([gcc -m32 conftest.c -o conftest 2>/dev/null], [hvmloader=y], [AC_MSG_WARN(XXX)])


Do you have any better ideas?


> Also I don't think "-o -" does what you want - it'll produce a
> binary named "-" (if compilation and linking succeed), while I
> suppose what you want is to discard the output (i.e. probably
> "-o /dev/null"). Albeit even that doesn't look to be the commonly
> used approach - a binary named "conftest" would normally be
> specified as the output, according to other configure.ac I've
> looked at. Such tests then have a final "rm -f conftest*".

OK

Re: [PATCH] firmware: don't build hvmloader if it is not needed

[Jan Beulich]

On 23.02.2021 00:05, Stefano Stabellini wrote:
> On Fri, 19 Feb 2021, Jan Beulich wrote:
>> On 19.02.2021 02:42, Stefano Stabellini wrote:
>>> --- a/tools/configure.ac
>>> +++ b/tools/configure.ac
>>> @@ -307,6 +307,10 @@ AC_ARG_VAR([AWK], [Path to awk tool])
>>>  
>>>  # Checks for programs.
>>>  AC_PROG_CC
>>> +AC_LANG(C)
>>> +AC_LANG_CONFTEST([AC_LANG_SOURCE([[int main() { return 0;}]])])
>>> +AS_IF([gcc -m32 conftest.c -o - 2>/dev/null], [hvmloader=y], [AC_MSG_WARN(hvmloader build disabled as the compiler cannot build 32bit binaries)])
>>> +AC_SUBST(hvmloader)
>>
>> I'm puzzled: "gcc -m32" looked to work fine on its own. I suppose
>> the above fails at the linking stage, but that's not what we care
>> about (we don't link with any system libraries). Instead, as said,
>> you want to check "gcc -m32 -c" produces correct code, in
>> particular with sizeof(uint64_t) being 8. Of course all of this
>> would be easier if their headers at least caused some form of
>> error, instead of silently causing bad code to be generated.
>>
>> The way you do it, someone simply not having 32-bit C libraries
>> installed would then also have hvmloader build disabled, even if
>> their compiler and headers are fine to use.
> 
> I realize that technically this test is probing for something different:
> the ability to build and link a trivial 32-bit userspace program, rather
> than a specific check about sizeof(uint64_t). However, I thought that if
> this test failed we didn't want to continue anyway.
> 
> If you say that hvmloader doesn't link against any system libraries,
> then in theory the hvmloader build could succeed even if this test
> failed. Hence, we need to change strategy.
> 
> What do you think of something like this?
> 
> AC_LANG_CONFTEST([AC_LANG_SOURCE([[#include <assert.h>
> #include <stdint.h>
> int main() { assert(sizeof(uint64_t) == 8); return 0;}]])])
> AS_IF([gcc -m32 conftest.c -o conftest 2>/dev/null], [hvmloader=y], [AC_MSG_WARN(XXX)])

The assert() would trigger at runtime only, so you'd also need to
invoke the program, wouldn't you?

> Do you have any better ideas?

An open-coded BUILD_BUG_ON()-like test would allow noticing the issue
already at compile time.

Jan