| Message ID | 20210213020540.27894-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: don't build hvmloader if it is not needed | expand |
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?
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
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? 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
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
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
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
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])
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
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
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
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)
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(-)