[libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering

[Daniel P. Berrangé]

On Thu, Mar 18, 2021 at 01:26:45PM +0100, Pavel Hrdina wrote:
> When the firmware auto-selection was introduced it always picked first
> usable firmware based on the JSON descriptions on the host. It is
> possible to add/remove/change the JSON files but it will always be for
> the whole host.
> 
> This patch introduces support for configuring the auto-selection per VM
> by adding users an option to limit what features they would like to have
> available in the firmware.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  docs/formatdomain.rst                         | 31 +++++++
>  docs/schemas/domaincommon.rng                 | 23 +++++
>  src/conf/domain_conf.c                        | 83 ++++++++++++++++++-
>  src/conf/domain_conf.h                        | 10 +++
>  .../os-firmware-efi-invalid-type.xml          | 28 +++++++
>  ...os-firmware-invalid-type.x86_64-latest.err |  1 +
>  .../os-firmware-invalid-type.xml              | 28 +++++++
>  tests/qemuxml2argvtest.c                      |  1 +
>  ...aarch64-os-firmware-efi.aarch64-latest.xml |  1 +
>  .../os-firmware-bios.x86_64-latest.xml        |  1 +
>  .../os-firmware-efi-secboot.x86_64-latest.xml |  1 +
>  .../os-firmware-efi.x86_64-latest.xml         |  1 +
>  tests/vmx2xmldata/vmx2xml-firmware-efi.xml    |  1 +
>  13 files changed, 207 insertions(+), 3 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml
>  create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err
>  create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml


> @@ -19600,22 +19606,67 @@ virDomainDefParseBootFirmwareOptions(virDomainDefPtr def,
>                                       xmlXPathContextPtr ctxt)
>  {
>      g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt);
> +    g_autofree char *type = virXPathString("string(./os/firmware/@type)", ctxt);



> -    if (!firmware)
> +    if (!firmware && !type)
>          return 0;
>  
> -    fw = virDomainOsDefFirmwareTypeFromString(firmware);
> +    if (firmware && type && STRNEQ(firmware, type)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("firmware attribute and firmware type has to be the same"));
> +        return -1;
> +    }

Why do we need to introduce a second attribute that must be the same as the
attribute we already have ?

This feels like it introduces a error scenario that we don't really need to
have and is redundant data for the user.


> diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml
> index 627e285ae1..cb4f3ccfce 100644
> --- a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml
> +++ b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml
> @@ -6,6 +6,7 @@
>    <vcpu placement='static'>1</vcpu>
>    <os firmware='efi'>
>      <type arch='aarch64' machine='virt-4.0'>hvm</type>
> +    <firmware type='efi'/>

This just looks odd having to set  'efi' twice.

>      <kernel>/aarch64.kernel</kernel>
>      <initrd>/aarch64.initrd</initrd>
>      <cmdline>earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait</cmdline>

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 :|