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

[Jan Beulich <jbeulich@xxxxxxxx>]

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