[libvirt PATCH] qemu_firmware: don't error out for unknown firmware features

[Daniel P. Berrangé]

On Mon, Jan 31, 2022 at 03:19:46PM +0100, Michal Prívozník wrote:
> On 1/31/22 14:18, Daniel P. Berrangé wrote:
> > On Mon, May 10, 2021 at 03:16:11PM +0200, Pavel Hrdina wrote:
> >> When QEMU introduces new firmware features libvirt will fail until we
> >> list that feature in our code as well which doesn't sound right.
> >>
> >> We should simply ignore the new feature until we add a proper support
> >> for it.
> >>
> >> Reported-by: Laszlo Ersek <lersek at redhat.com>
> >> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> >> ---
> >>  src/qemu/qemu_firmware.c | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > Re-visiting this patch....
> > 
> > During guest startup, libvirt parses all firmware descriptors and
> > treats any errors during parsing as fatal.
> > 
> > The problem we were fixing here was that on the QEMU / EDK2 side a
> > new firmware feature called 'amd-sev-es' was added which libvirt
> > didn't know about, so all libvirt guests using <os firmware='efi'/>
> > broke when a distro shipped a descriptor.
> > 
> > We semi-future proofed ourselves by ignoring unknown features
> > hereafter.
> > 
> > I'm now coming to introduce a new feature in the firmware descriptors
> > to allow for OVMF builds which have a read-only CODE and *NOT* VARs
> > file at all.
> > 
> > Currently the firmware descriptor treats nvram path as mandatory
> > and libvirt validates that.
> > 
> > IOW, as soon as I make the nvram path optional and some distro
> > ships a firmware descriptor taking advantage of that, libvirt again
> > breaks for all guests using <os firmware='efi'/>.  I've thought about
> > various different ways to handle this but can't figure out any way
> > to support an VARs-free firmware descriptor in a way that's backwards
> > compatible with libvirt's current parsing code.
> > 
> > This is my current qemu patch:
> > 
> >   https://listman.redhat.com/archives/libvir-list/2022-January/msg01316.html
> > 
> > We weren't future proofed enough.
> > 
> > It is nice reporting errors when parsing firmware descriptors because
> > it highlights real world mistakes.
> > 
> > It is bad reporting errors when parsing firmware descriptors because
> > it introduces potential for bogus failure scenarios any time the spec
> > is extended.
> > 
> > 
> > If we quit reporting errors and simply log a warning and ignore the
> > firmware descriptor we couldn't parse entirely, then we are more
> > robust if a completely new firmware descriptor is introduced - we
> > can still parse existing firmware descriptor files and probably
> > do something sensible.
> > 
> > On the flipside, if an existing firmware descriptor is modified and
> > we ignore it due to some unexpected data being present, we cause
> > a regression.
> > 
> > I feel like we're in a bit of a no-win situation here wrt error
> > reporting and future proofing.
> > 
> > My gut feeling is that we have little choice but to rely on the OS
> > distro not to introduce firmware descriptors with fields that
> > libvirt is too old to support, except in limited scenarios like
> > the features enum case below where we can reasonably ignore a
> > very specific error.
> > 
> > Anyone have other ideas ?
> 
> I think it's sane requirement for distros to ship FW descriptors that
> libvirt understands. Or backport patches for libvirt. One way or
> another, if a mgmt app would want to use SEV it would need bleeding edge
> libvirt anyway. And we can use those number prefixes to make new
> firmware files de-prioritized so that users who don't care are not
> affected.

Oh, I hadn't checked how number prioritization affects it. So you're
saying that we stop parsing the firmware file as soon as we find a
valid match. That does indeed limit the impact of the change.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|