[libvirt] [PATCH 2/3] qemu: Format rom.enabled attribute for PCI devices

[Peter Krempa]

On Fri, Apr 20, 2018 at 17:44:30 +0200, Andrea Bolognani wrote:
> The attribute can be used to disable ROM loading completely
> for a device.

You could mention that this is necessary because even if you turn the
ROM BAR off, some firmware might still load it.

> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1425058
> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> ---
>  src/qemu/qemu_command.c                       | 13 ++++++++++--
>  tests/qemuxml2argvdata/pci-rom-disabled.args  | 26 ++++++++++++++++++++++++
>  tests/qemuxml2argvdata/pci-rom-disabled.xml   | 20 ++++++++++++++++++
>  tests/qemuxml2argvtest.c                      |  1 +
>  tests/qemuxml2xmloutdata/pci-rom-disabled.xml | 29 +++++++++++++++++++++++++++
>  tests/qemuxml2xmltest.c                       |  1 +
>  6 files changed, 88 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/pci-rom-disabled.args
>  create mode 100644 tests/qemuxml2argvdata/pci-rom-disabled.xml
>  create mode 100644 tests/qemuxml2xmloutdata/pci-rom-disabled.xml
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b666f3715f..84c4e1e350 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -442,13 +442,20 @@ static int
>  qemuBuildRomStr(virBufferPtr buf,
>                  virDomainDeviceInfoPtr info)
>  {
> -    if (info->rombar || info->romfile) {
> +    if (info->romenabled || info->rombar || info->romfile) {
>          if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           "%s", _("rombar and romfile are supported only for PCI devices"));
> +                           "%s", _("ROM tuning is only supported for PCI devices"));
>              return -1;
>          }
>  
> +        /* Passing an empty romfile= tells QEMU to disable ROM entirely for
> +         * this device, and makes other settings irrelevant */
> +        if (info->romenabled == VIR_TRISTATE_BOOL_NO) {
> +            virBufferAddLit(buf, ",romfile=");
> +            goto done;

You can return early rather than having to jump. This function needs to
be refactored anyways, so no need to be nice to others comming after
you.

> +        }
> +
>          switch (info->rombar) {
>          case VIR_TRISTATE_SWITCH_OFF:
>              virBufferAddLit(buf, ",rombar=0");
> @@ -464,6 +471,8 @@ qemuBuildRomStr(virBufferPtr buf,
>             virQEMUBuildBufferEscapeComma(buf, info->romfile);
>          }
>      }
> +
> + done:
>      return 0;
>  }

[...]

> diff --git a/tests/qemuxml2argvdata/pci-rom-disabled.xml b/tests/qemuxml2argvdata/pci-rom-disabled.xml
> new file mode 100644
> index 0000000000..1c12052382
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/pci-rom-disabled.xml
> @@ -0,0 +1,20 @@
> +<domain type='qemu'>
> +  <name>guest</name>
> +  <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219100</memory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc'>hvm</type>
> +  </os>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-x86_64</emulator>
> +    <controller type='pci' model='pci-root'/>
> +    <controller type='usb' model='none'/>
> +    <interface type='user'>
> +      <mac address='52:54:00:24:a5:9f'/>
> +      <model type='virtio'/>
> +      <rom enabled='no'/>

If we are going to explicitly keep around the quirk with the empty file
I think we should add a test case for it. It will not be fun though
since such XML is invalid.

> +    </interface>
> +    <memballoon model='none'/>
> +  </devices>
> +</domain>

ACK if you get rid of that jump.

The test case will need to be added separately anyways.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180423/602b34bc/attachment-0001.sig>