[libvirt] [PATCH 0/3] qemu: Allow disabling ROM for PCI devices
All the details are in the bug report referenced in 2/3. Andrea Bolognani (3): conf: Add rom.enabled attribute for PCI devices qemu: Format rom.enabled attribute for PCI devices news: Document rom.enabled attribute for PCI devices docs/formatdomain.html.in | 3 +++ docs/news.xml | 10 +++++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/device_conf.h | 1 + src/conf/domain_conf.c | 26 +++++++++++++++++++++++- 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 + 11 files changed, 132 insertions(+), 3 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 -- 2.14.3
The attribute can be used to disable ROM loading completely for a device. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 3 +++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/device_conf.h | 1 + src/conf/domain_conf.c | 26 +++++++++++++++++++++++++- 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ada0df227f..0afc310e25 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4476,6 +4476,9 @@ virtual function of an sr-iov capable ethernet device (which has no boot ROMs for the VFs). <span class="since">Since 0.9.10 (QEMU and KVM only)</span>. + The optional <code>enabled</code> attribute can be set to + <code>no</code> to disable PCI ROM loading completely for the device. + <span class="since">Since 4.3.0 (QEMU and KVM only)</span>. </dd> <dt><code>address</code></dt> <dd>The <code>address</code> element for USB devices has a diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4cab55f05d..3569b92127 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5108,6 +5108,11 @@ <define name="rom"> <element name="rom"> + <optional> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + </optional> <optional> <attribute name="bar"> <ref name="virOnOff"/> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index f87d6f1fc6..a31ce9c376 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -153,6 +153,7 @@ struct _virDomainDeviceInfo { } master; /* rombar and romfile are only used for pci hostdev and network * devices. */ + int romenabled; /* enum virTristateBool */ int rombar; /* enum virTristateSwitch */ char *romfile; /* bootIndex is only used for disk, network interface, hostdev diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35666c1347..3c152441df 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6095,9 +6095,17 @@ virDomainDeviceInfoFormat(virBufferPtr buf, } if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) && - (info->rombar != VIR_TRISTATE_SWITCH_ABSENT || info->romfile)) { + (info->romenabled != VIR_TRISTATE_BOOL_ABSENT || + info->rombar != VIR_TRISTATE_SWITCH_ABSENT || + info->romfile)) { virBufferAddLit(buf, "<rom"); + if (info->romenabled != VIR_TRISTATE_BOOL_ABSENT) { + const char *romenabled = virTristateBoolTypeToString(info->romenabled); + + if (romenabled) + virBufferAsprintf(buf, " enabled='%s'", romenabled); + } if (info->rombar != VIR_TRISTATE_SWITCH_ABSENT) { const char *rombar = virTristateSwitchTypeToString(info->rombar); @@ -6738,6 +6746,7 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, xmlNodePtr boot = NULL; xmlNodePtr rom = NULL; char *type = NULL; + char *romenabled = NULL; char *rombar = NULL; char *aliasStr = NULL; int ret = -1; @@ -6791,6 +6800,12 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, } if (rom) { + if ((romenabled = virXMLPropString(rom, "enabled")) && + ((info->romenabled = virTristateBoolTypeFromString(romenabled)) <= 0)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown rom enabled value '%s'"), romenabled); + goto cleanup; + } if ((rombar = virXMLPropString(rom, "bar")) && ((info->rombar = virTristateSwitchTypeFromString(rombar)) <= 0)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -6798,6 +6813,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, goto cleanup; } info->romfile = virXMLPropString(rom, "file"); + + if (info->romenabled == VIR_TRISTATE_BOOL_NO && + (info->rombar != VIR_TRISTATE_SWITCH_ABSENT || info->romfile)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("ROM tuning is not supported when ROM is disabled")); + goto cleanup; + } } if (address && @@ -6811,6 +6834,7 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, virDomainDeviceInfoClear(info); VIR_FREE(type); VIR_FREE(rombar); + VIR_FREE(romenabled); VIR_FREE(aliasStr); return ret; } -- 2.14.3
On Fri, Apr 20, 2018 at 17:44:29 +0200, Andrea Bolognani wrote:
The attribute can be used to disable ROM loading completely for a device.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 3 +++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/device_conf.h | 1 + src/conf/domain_conf.c | 26 +++++++++++++++++++++++++- 4 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ada0df227f..0afc310e25 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4476,6 +4476,9 @@ virtual function of an sr-iov capable ethernet device (which has no boot ROMs for the VFs). <span class="since">Since 0.9.10 (QEMU and KVM only)</span>. + The optional <code>enabled</code> attribute can be set to + <code>no</code> to disable PCI ROM loading completely for the device. + <span class="since">Since 4.3.0 (QEMU and KVM only)</span>.
Maybe you should mention that any other configration may not be supported in that case. [...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35666c1347..3c152441df 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -6798,6 +6813,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, goto cleanup; } info->romfile = virXMLPropString(rom, "file"); + + if (info->romenabled == VIR_TRISTATE_BOOL_NO && + (info->rombar != VIR_TRISTATE_SWITCH_ABSENT || info->romfile)) {
I'd explicitly allow empty string in info->romfile, but that would mean that this needs to be moved to the qemu post-parse callback, since that is a qemu quirk. Justification is that, mgmt tools will be able to use enabled='no' together with the empty file string without having to do any probing whether that is a valid configuration.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s",
Above line can be merged into previous one.
+ _("ROM tuning is not supported when ROM is disabled")); + goto cleanup; + } }
if (address &&
On Mon, 2018-04-23 at 08:53 +0200, Peter Krempa wrote:
+ The optional <code>enabled</code> attribute can be set to + <code>no</code> to disable PCI ROM loading completely for the device. + <span class="since">Since 4.3.0 (QEMU and KVM only)</span>.
Maybe you should mention that any other configration may not be supported in that case.
Good idea.
@@ -6798,6 +6813,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, goto cleanup; } info->romfile = virXMLPropString(rom, "file"); + + if (info->romenabled == VIR_TRISTATE_BOOL_NO && + (info->rombar != VIR_TRISTATE_SWITCH_ABSENT || info->romfile)) {
I'd explicitly allow empty string in info->romfile, but that would mean that this needs to be moved to the qemu post-parse callback, since that is a qemu quirk.
Justification is that, mgmt tools will be able to use enabled='no' together with the empty file string without having to do any probing whether that is a valid configuration.
But enabled='no' would be rejected by earlier libvirt releases, which makes the point about avoiding feature detection moot, no? I would expect management applications that already use (invalid, according to the schema, but working fine by all other counts) file='' to keep using that until they bump their required libvirt version to 4.3.0, and management applications that didn't already use the existing kludge to just go straight for enabled='no'. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Apr 23, 2018 at 11:10:01 +0200, Andrea Bolognani wrote:
On Mon, 2018-04-23 at 08:53 +0200, Peter Krempa wrote:
+ The optional <code>enabled</code> attribute can be set to + <code>no</code> to disable PCI ROM loading completely for the device. + <span class="since">Since 4.3.0 (QEMU and KVM only)</span>.
Maybe you should mention that any other configration may not be supported in that case.
Good idea.
@@ -6798,6 +6813,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, goto cleanup; } info->romfile = virXMLPropString(rom, "file"); + + if (info->romenabled == VIR_TRISTATE_BOOL_NO && + (info->rombar != VIR_TRISTATE_SWITCH_ABSENT || info->romfile)) {
I'd explicitly allow empty string in info->romfile, but that would mean that this needs to be moved to the qemu post-parse callback, since that is a qemu quirk.
Justification is that, mgmt tools will be able to use enabled='no' together with the empty file string without having to do any probing whether that is a valid configuration.
But enabled='no' would be rejected by earlier libvirt releases, which makes the point about avoiding feature detection moot, no?
No. Only if they enable schema validation. Since that is an opt-in you still can define such XML and the option will be ignored.
I would expect management applications that already use (invalid, according to the schema, but working fine by all other counts) file='' to keep using that until they bump their required libvirt version to 4.3.0, and management applications that didn't already use the existing kludge to just go straight for enabled='no'.
I guess that is fair enough. ACK then.
The attribute can be used to disable ROM loading completely for a device. 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; + } + 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.args b/tests/qemuxml2argvdata/pci-rom-disabled.args new file mode 100644 index 0000000000..8c9dc2fb80 --- /dev/null +++ b/tests/qemuxml2argvdata/pci-rom-disabled.args @@ -0,0 +1,26 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest \ +-S \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-netdev user,id=hostnet0 \ +-device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:24:a5:9f,bus=pci.0,\ +addr=0x3,romfile= 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'/> + </interface> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 74d930ebe2..ae9893a84e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1554,6 +1554,7 @@ mymain(void) DO_TEST_PARSE_ERROR("hostdev-mdev-invalid-target-address", QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("pci-rom", NONE); + DO_TEST("pci-rom-disabled", NONE); DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, GIC_NONE, NONE); DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, GIC_NONE, NONE); diff --git a/tests/qemuxml2xmloutdata/pci-rom-disabled.xml b/tests/qemuxml2xmloutdata/pci-rom-disabled.xml new file mode 100644 index 0000000000..6a95064ebf --- /dev/null +++ b/tests/qemuxml2xmloutdata/pci-rom-disabled.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pci-root'/> + <controller type='usb' index='0' model='none'/> + <interface type='user'> + <mac address='52:54:00:24:a5:9f'/> + <model type='virtio'/> + <rom enabled='no'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 9e77b9fb13..6c1f0b0fa6 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -474,6 +474,7 @@ mymain(void) DO_TEST("hostdev-vfio", NONE); DO_TEST("hostdev-mdev-precreated", NONE); DO_TEST("pci-rom", NONE); + DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-serial-dev-chardev", NONE); DO_TEST("encrypted-disk", NONE); -- 2.14.3
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.
On Mon, 2018-04-23 at 08:58 +0200, Peter Krempa wrote:
The test case will need to be added separately anyways.
Here it is: https://www.redhat.com/archives/libvir-list/2018-April/msg02122.html -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/news.xml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index dec3f134ce..de95d919a2 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -56,6 +56,16 @@ host-passthrough CPU model. </description> </change> + <change> + <summary> + qemu: Optionally disable ROM loading for PCI devices + </summary> + <description> + The <code>enabled</code> attribute of the <code>rom</code> + sub-element, usable for PCI devices, can be used for situations + where loading a PCI ROM is not desiderable. + </description> + </change> </section> <section title="Removed features"> <change> -- 2.14.3
On Fri, Apr 20, 2018 at 17:44:31 +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/news.xml | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml index dec3f134ce..de95d919a2 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -56,6 +56,16 @@ host-passthrough CPU model. </description> </change> + <change> + <summary> + qemu: Optionally disable ROM loading for PCI devices + </summary> + <description> + The <code>enabled</code> attribute of the <code>rom</code> + sub-element, usable for PCI devices, can be used for situations + where loading a PCI ROM is not desiderable.
I'm not sure that this qualifies as a feature. ACK regardless
On Mon, 2018-04-23 at 09:00 +0200, Peter Krempa wrote:
On Fri, Apr 20, 2018 at 17:44:31 +0200, Andrea Bolognani wrote:
+ <summary> + qemu: Optionally disable ROM loading for PCI devices + </summary> + <description> + The <code>enabled</code> attribute of the <code>rom</code> + sub-element, usable for PCI devices, can be used for situations + where loading a PCI ROM is not desiderable.
I'm not sure that this qualifies as a feature.
Yeah, "Improvements" is probably a better fit. I've moved it. -- Andrea Bolognani / Red Hat / Virtualization
Even though we just introduced the rom.enabled attribute to properly cover the use case, there might be guests out there that use the only previously available way of disabling PCI ROM loading by not opting in to schema validation. To make sure such guests will keep working going forward, introduce a test case covering the legacy workaround. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../qemuxml2argvdata/pci-rom-disabled-invalid.args | 1 + .../qemuxml2argvdata/pci-rom-disabled-invalid.xml | 25 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../pci-rom-disabled-invalid.xml | 29 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 57 insertions(+) create mode 120000 tests/qemuxml2argvdata/pci-rom-disabled-invalid.args create mode 100644 tests/qemuxml2argvdata/pci-rom-disabled-invalid.xml create mode 100644 tests/qemuxml2xmloutdata/pci-rom-disabled-invalid.xml diff --git a/tests/qemuxml2argvdata/pci-rom-disabled-invalid.args b/tests/qemuxml2argvdata/pci-rom-disabled-invalid.args new file mode 120000 index 0000000000..0dffe3c624 --- /dev/null +++ b/tests/qemuxml2argvdata/pci-rom-disabled-invalid.args @@ -0,0 +1 @@ +pci-rom-disabled.args \ No newline at end of file diff --git a/tests/qemuxml2argvdata/pci-rom-disabled-invalid.xml b/tests/qemuxml2argvdata/pci-rom-disabled-invalid.xml new file mode 100644 index 0000000000..5ef58d03ae --- /dev/null +++ b/tests/qemuxml2argvdata/pci-rom-disabled-invalid.xml @@ -0,0 +1,25 @@ +<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'/> + <!-- + This method of disabling PCI ROM loading is still supported + for backwards compatibility reasons, but <rom enabled='no'/> + should be used instead. + --> + <rom file=''/> + </interface> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ae9893a84e..5b3bd4a996 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1555,6 +1555,7 @@ mymain(void) QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("pci-rom", NONE); DO_TEST("pci-rom-disabled", NONE); + DO_TEST("pci-rom-disabled-invalid", NONE); DO_TEST_FULL("restore-v2", "exec:cat", 7, 0, 0, GIC_NONE, NONE); DO_TEST_FULL("restore-v2-fd", "stdio", 7, 0, 0, GIC_NONE, NONE); diff --git a/tests/qemuxml2xmloutdata/pci-rom-disabled-invalid.xml b/tests/qemuxml2xmloutdata/pci-rom-disabled-invalid.xml new file mode 100644 index 0000000000..745d19d8ad --- /dev/null +++ b/tests/qemuxml2xmloutdata/pci-rom-disabled-invalid.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pci-root'/> + <controller type='usb' index='0' model='none'/> + <interface type='user'> + <mac address='52:54:00:24:a5:9f'/> + <model type='virtio'/> + <rom file=''/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6c1f0b0fa6..4b5aa2315e 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -475,6 +475,7 @@ mymain(void) DO_TEST("hostdev-mdev-precreated", NONE); DO_TEST("pci-rom", NONE); DO_TEST("pci-rom-disabled", NONE); + DO_TEST("pci-rom-disabled-invalid", NONE); DO_TEST("pci-serial-dev-chardev", NONE); DO_TEST("encrypted-disk", NONE); -- 2.14.3
On Mon, Apr 23, 2018 at 13:54:34 +0200, Andrea Bolognani wrote:
Even though we just introduced the rom.enabled attribute to properly cover the use case, there might be guests out there that use the only previously available way of disabling PCI ROM loading by not opting in to schema validation.
To make sure such guests will keep working going forward, introduce a test case covering the legacy workaround.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../qemuxml2argvdata/pci-rom-disabled-invalid.args | 1 + .../qemuxml2argvdata/pci-rom-disabled-invalid.xml | 25 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../pci-rom-disabled-invalid.xml | 29 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 57 insertions(+) create mode 120000 tests/qemuxml2argvdata/pci-rom-disabled-invalid.args create mode 100644 tests/qemuxml2argvdata/pci-rom-disabled-invalid.xml create mode 100644 tests/qemuxml2xmloutdata/pci-rom-disabled-invalid.xml
ACK
participants (2)
-
Andrea Bolognani -
Peter Krempa