On 12/16/22 10:24, Laszlo Ersek wrote: > On 12/15/22 18:34, Richard W.M. Jones wrote: >> On Thu, Dec 15, 2022 at 06:45:46PM +0200, Andrey Drobyshev wrote: >>> According to [1], there're different ways to specify which firmware is >>> to be used by a libvirt-driven VM. Namely, there's an automatic >>> firmware selection, e.g.: >>> >>> ... >>> <os firmware='(bios|efi)'> >>> ... >>> >>> and a manual one, e.g.: >>> >>> ... >>> <os> >>> <loader readonly='yes' >>> type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> >>> ... >>> </os> >>> ... >>> >>> with the latter being a way to specify UEFI firmware. So let's add this >>> search path as well when parsing source VM's libvirt xml. >>> >>> [1] https://libvirt.org/formatdomain.html#bios-bootloader >>> >>> Signed-off-by: Andrey Drobyshev <andrey.drobys...@virtuozzo.com> >>> Originally-by: Denis Plotnikov <dplotni...@virtuozzo.com> >> >> Laszlo, any comments on this? > > Aren't you supposed to be enjoying your PTO? :) > >> >>> input/parse_libvirt_xml.ml | 19 +++++++++++++++++-- >>> 1 file changed, 17 insertions(+), 2 deletions(-) >>> >>> diff --git a/input/parse_libvirt_xml.ml b/input/parse_libvirt_xml.ml >>> index 1e98ce1a..afe3fbc2 100644 >>> --- a/input/parse_libvirt_xml.ml >>> +++ b/input/parse_libvirt_xml.ml >>> @@ -446,12 +446,27 @@ let parse_libvirt_xml ?conn xml = >>> done; >>> List.rev !nics in >>> >>> - (* Firmware. *) >>> + (* Firmware. >>> + * If "/domain/os" node doesn't contain "firmware" attribute (automatic >>> + * firmware), we look for the presence of "OVMF_CODE" in >>> "/domain/os/loader" >>> + * node (manual firmware). >>> + * See https://libvirt.org/formatdomain.html#bios-bootloader >>> + *) >>> let firmware = >>> match xpath_string "/domain/os/@firmware" with >>> | Some "bios" -> BIOS >>> | Some "efi" -> UEFI >>> - | None | Some _ -> UnknownFirmware in >>> + | None | Some _ -> ( >>> + let loader = xpath_string "/domain/os/loader" in >>> + match loader with >>> + | None -> UnknownFirmware >>> + | _ -> ( >>> + let re = Str.regexp_string "OVMF_CODE" in >>> + let loader = Option.default "" loader in >>> + try ignore (Str.search_forward re loader 0); UEFI >>> + with Not_found -> UnknownFirmware >> >> It's a bit nicer to use PCRE regexps here, and that allows you to make >> them more precise, and also to hoist the compilation to the top of the >> file. You could write something like: >> >> (near the top of the file) >> >> let re_loader_contains_ovmf = PCRE.compile "/OVMF_CODE" >> >> (here) >> >> let loader = Option.default "" loader in >> if PCRE.matches re_loader_contains_ovmf loader then UEFI >> else UnknownFirmware >> >>> + ) >>> + ) in >>> >>> (* Check for hostdev devices. (RHBZ#1472719) *) >>> let () = > > It's better to equate UEFI with the XPath expression > > /domain/os/loader/@type = 'pflash' [1] > > (CC Michal) > > Historically, the progress was: > > - SeaBIOS binary: mapped as ROM > > - unified OVMF.fd file: mapped as flash, single pflash chip, writeable. > This is what the @type='pflash' attribute was introduced for (with > @type='rom' introduced for the preexistent mapping type) > > - UEFI with split files (OVMF_CODE.fd as firmware executable, and a copy > of the OVMF_VARS.fd template as varstore): assuming @type='pflash', > introduce @readonly='no' for the preexistent mapping type above, and > introduce @readonly='yes' for signaling that the firmware exacutable is > separate from the varstore. OVMF_CODE is mapped r/o, while the varstore > is mapped r/w. When @readonly='yes', the varstore is described in the > separate element /domain/os/nvram (which I won't get into, here). > > - SMM: assuming @type='pflash' *and* @readonly='yes' (i.e., UEFI with > split varstore), introduce @secure='no' for the previous case above, and > introduce @secure='yes' for expressing the following: only such guest > code will be permitted to issue writes for the (otherwise writeable) > varstore pflash chip that executes in SMM. In other words, restrict > pflash writes (which are only permitted for the varstore anyway, as the > fw executable pflash is r/o whole-sale anyway) to guest code that runs > in SMM. This setting is important for protecting the UEFI varstore from > unauthenticated code running *within the guest* (for example, so that > the guest kernel / root user cannot overwrite the Secure Boot related > authenticated UEFI variables with direct in-guest hardware-level pflash > accesses, circumventing the SB verifications performed by the firmware). > > For this case (i.e., @type='pflash' @readonly='yes' @secure='yes') the > board must be Q35 (I forget the exact minimum machine type version, > something around 2.5) and the separate > > /domain/features/smm/@state = 'on' > > attribute is also needed for enabling SMM emulation. The @secure > attribute is a restriction, but then the board must actually be able to > emulate SMM, and that's what this latter attribute does. We usually > spell it out, although Q35 does default to enabling SMM emualtion, at > least at the QEMU level. > > ... Long story short: looking for "OVMF_CODE" in the text child node of > "/domain/os/loader" is less robust than [1]. While [1] is not > bulletproof either (in theory you could perhaps map non-EFI firmware via > pflash...), equating @type='pflash' with UEFI is very safe in practice. > The filenames of firmware binaries are much more liable to change. > > Laszlo >
Hi Laszlo, Thanks you for such a detailed historical overview. Based on what you said, shouldn't we consider checking for @type='rom' as well? AFAICT this value is still acceptable (at least according to libvirt docs https://libvirt.org/formatdomain.html#bios-bootloader), but the current v2v implementation will fail to detect BIOS firmware in that case. _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
