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.drobyshev(a)virtuozzo.com&gt; > Originally-by: Denis Plotnikov <dplotnikov(a)virtuozzo.com&gt; Laszlo, any comments on this? > 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 () = Rich.

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.drobyshev(a)virtuozzo.com&gt; >> Originally-by: Denis Plotnikov <dplotnikov(a)virtuozzo.com&gt; > > 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

On 12/16/22 18:09, Andrey Drobyshev wrote: > 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.drobyshev(a)virtuozzo.com&gt; >>>> Originally-by: Denis Plotnikov <dplotnikov(a)virtuozzo.com&gt; >>> >>> 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. Hmmm, not sure if this buys us much. The various input modules seem to handle "unknown" differently. Some equate "unknown" with BIOS (e.g., "input/parse_domain_from_vmx.ml"), some are only capable of reporting "unknown" ("input/input_disk.ml"); either way, if "convert/convert.ml" sees UnknownFirmware, it will use the inspection info for determining the target firmware. Right now the libvirt input (IIUC) doesn't specifically understand @type=rom, so it goes with UnknownFirmware, and then we just use the inspection info (partitions etc) in "convert/convert.ml". So it's not really a "failure to detect". By recognizing @type='rom', we could perhaps short-circuit the partition-based detection, but I'm not convinced we should do it. Doesn't buy us much and any code changes can result in regressions.

Plus, I don't recall a single domain XML where @type='rom' was actually spelled out! So the pattern wouldn't match in practice anyway, I believe.

Laszlo