The attribute can be used to disable ROM loading completely for a device. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- 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 Mon, 2018-04-23 at 08:53 +0200, Peter Krempa wrote: Good idea. 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

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(a)redhat.com&gt; --- 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:31 +0200, Andrea Bolognani wrote: I'm not sure that this qualifies as a feature. ACK regardless

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(a)redhat.com&gt; --- .../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