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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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