On 04/11/2021 14:52, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH-for-4.16 1/2] configure: modify default of
> building rombios"):
>> The tools/configure script will default to build rombios if qemu
>> traditional is enabled. If rombios is being built, ipxe will be built
>> per default, too.
>>
>> This results in rombios and ipxe no longer being built per default when
>> disabling qemu traditional.
>>
>> Fix that be rearranging the dependencies:
>>
>> - build ipxe per default
>> - build rombios per default if either ipxe or qemu traditional are
>> being built
>>
>> This modification prepares not building qemu traditional per default
>> without affecting build of rombios and ipxe.
> Thanks. We had a conversation on irc about this. In summary:
>
> Reviewed-by: Ian Jackson <i...@xenproject.org>
>
>
> Discussion:
>
> Both before and after this patch --with-system-ipxe --disable-ipxe (or
> --with-system-ipxe on arm) would set IPXE_PATH but not cause ipxe to
> be actually built. I think that is correct.
>
> We discussed the linkage between rombios and ipxe. Our understanding
> is that thbe intent was to support two configurations:
> qemu-trad + {hvmloader, rombios, ipxe}
> qemu-upstraem + {hvmloader, seabios, pxe rom provided by qemu}
>
> Before this patch, --disable-rombios --enable-ipxe would be an error.
> After this patch, it is tolerated (and indeed, it might occur with
> only one of those options due to defaulting). But we think that there
> is no actual functional link between these pieces: ie, nothing will
> actually go wrong. You just might not have ipxe support since you
> wouldn't be using trad at all.
>
> We think that someone who explicitly manipulates the
> --en/disable-rombios and --en/disable-ipxe options so as to construct
> such a situation ought to know what they are doing and should get what
> the asked for.
>
> Most people will controlling this via --en/disable-qemu-trad, which
> will do a plausible. thing.
>
>
> I think *this* patch is a bugfix, although to rather obscure configure
> behaviour. I am inclined to release ack both this and the followup,
> for the reasons I gave previously.
>
> Comments (especially, anything to contradict what I wrote above) ASAP
> please.
I think the patch will fix the problem from CI. And from that point of
view it is an improvement.
However, I don't think the help text is correct any more. Specifically:
+ [Use system supplied IPXE PATH instead of building and installing
+ our own version, it takes precedence over --{en,dis}able-ipxe
and is
+ bound by the presence of rombios, --without-system-ipxe is an
error]),[
and the binding to RomBIOS is no longer true AFAICT. At a minimum, I
think the wording there needs tweaking.
If tweaking is going on, then "per default" in the commit message feels
a little awkward, and would more normally be "by default".
Otherwise, LGTM.
~Andrew
