* [PATCH] firmware: don't build hvmloader if it is not needed @ 2021-02-13 2:05 Stefano Stabellini 2021-02-13 13:50 ` Marek Marczykowski-Górecki 0 siblings, 1 reply; 11+ messages in thread From: Stefano Stabellini @ 2021-02-13 2:05 UTC (permalink / raw) To: xen-devel Cc: sstabellini, cardoe, andrew.cooper3, wl, iwj, anthony.perard, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-13 2:05 [PATCH] firmware: don't build hvmloader if it is not needed Stefano Stabellini @ 2021-02-13 13:50 ` Marek Marczykowski-Górecki 2021-02-15 8:29 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Marek Marczykowski-Górecki @ 2021-02-13 13:50 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, Stefano Stabellini [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-13 13:50 ` Marek Marczykowski-Górecki @ 2021-02-15 8:29 ` Jan Beulich 2021-02-16 18:31 ` Stefano Stabellini 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2021-02-15 8:29 UTC (permalink / raw) To: Marek Marczykowski-Górecki, Stefano Stabellini Cc: xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-15 8:29 ` Jan Beulich @ 2021-02-16 18:31 ` Stefano Stabellini 2021-02-17 9:47 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Stefano Stabellini @ 2021-02-16 18:31 UTC (permalink / raw) To: Jan Beulich Cc: Marek Marczykowski-Górecki, Stefano Stabellini, xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-16 18:31 ` Stefano Stabellini @ 2021-02-17 9:47 ` Jan Beulich 2021-02-17 23:45 ` Stefano Stabellini 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2021-02-17 9:47 UTC (permalink / raw) To: Stefano Stabellini Cc: Marek Marczykowski-Górecki, xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-17 9:47 ` Jan Beulich @ 2021-02-17 23:45 ` Stefano Stabellini 2021-02-18 11:40 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Stefano Stabellini @ 2021-02-17 23:45 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Marek Marczykowski-Górecki, xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-17 23:45 ` Stefano Stabellini @ 2021-02-18 11:40 ` Jan Beulich 2021-02-19 1:42 ` Stefano Stabellini 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2021-02-18 11:40 UTC (permalink / raw) To: Stefano Stabellini Cc: Marek Marczykowski-Górecki, xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-18 11:40 ` Jan Beulich @ 2021-02-19 1:42 ` Stefano Stabellini 2021-02-19 8:28 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Stefano Stabellini @ 2021-02-19 1:42 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Marek Marczykowski-Górecki, xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, 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]) ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-19 1:42 ` Stefano Stabellini @ 2021-02-19 8:28 ` Jan Beulich 2021-02-22 23:05 ` Stefano Stabellini 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2021-02-19 8:28 UTC (permalink / raw) To: Stefano Stabellini Cc: Marek Marczykowski-Górecki, xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-19 8:28 ` Jan Beulich @ 2021-02-22 23:05 ` Stefano Stabellini 2021-02-23 7:23 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Stefano Stabellini @ 2021-02-22 23:05 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Marek Marczykowski-Górecki, xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: don't build hvmloader if it is not needed 2021-02-22 23:05 ` Stefano Stabellini @ 2021-02-23 7:23 ` Jan Beulich 0 siblings, 0 replies; 11+ messages in thread From: Jan Beulich @ 2021-02-23 7:23 UTC (permalink / raw) To: Stefano Stabellini Cc: Marek Marczykowski-Górecki, xen-devel, cardoe, andrew.cooper3, wl, iwj, anthony.perard, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-23 7:23 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-13 2:05 [PATCH] firmware: don't build hvmloader if it is not needed Stefano Stabellini 2021-02-13 13:50 ` Marek Marczykowski-Górecki 2021-02-15 8:29 ` Jan Beulich 2021-02-16 18:31 ` Stefano Stabellini 2021-02-17 9:47 ` Jan Beulich 2021-02-17 23:45 ` Stefano Stabellini 2021-02-18 11:40 ` Jan Beulich 2021-02-19 1:42 ` Stefano Stabellini 2021-02-19 8:28 ` Jan Beulich 2021-02-22 23:05 ` Stefano Stabellini 2021-02-23 7:23 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).