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@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.