[PATCH V4 0/3] Xen: Support PCI permissive setting with writeFiltering attribute
This is V4 of Marek's series to support the xl.cfg(5) permissive setting on PCI devices. Previous versions of the series V3 https://www.redhat.com/archives/libvir-list/2020-August/msg00465.html V2 https://www.redhat.com/archives/libvir-list/2020-April/msg01230.html Changes from V3: - Add a check to qemu_validate to report error if writeFiltering is used in a qemu domain. - Rebase to master. Changes from V2: - Instead of using a permisssive attribute on the <hostdev> element, use a writeFiltering attribute on the <hostdev>'s <source> element. Rational being that the filtering of writes to the PCI config space is done at the source. Jim Fehlig (3): Xen: Add writeFiltering option for PCI devices Xen: Add support for writeFiltering in config converter News: Advertise support for writeFiltering attribute of PCI hostdevs NEWS.rst | 7 ++ docs/formatdomain.rst | 7 +- docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 14 ++++ src/conf/domain_conf.h | 1 + src/libxl/libxl_conf.c | 1 + src/libxl/xen_common.c | 77 +++++++++++++++---- src/qemu/qemu_validate.c | 7 ++ .../libxlxml2domconfigdata/moredevs-hvm.json | 6 ++ tests/libxlxml2domconfigdata/moredevs-hvm.xml | 5 ++ tests/xlconfigdata/test-fullvirt-pci.cfg | 25 ++++++ tests/xlconfigdata/test-fullvirt-pci.xml | 53 +++++++++++++ tests/xlconfigtest.c | 1 + 13 files changed, 195 insertions(+), 14 deletions(-) create mode 100644 tests/xlconfigdata/test-fullvirt-pci.cfg create mode 100644 tests/xlconfigdata/test-fullvirt-pci.xml -- 2.28.0
By default Xen only allows guests to write "known safe" values into PCI configuration space, yet many devices require writes to other areas of the configuration space in order to operate properly. To allow writing any values Xen supports the 'permissive' setting, see xl.cfg(5) man page. This change models Xen's permissive setting by adding a writeFiltering attribute on the <source> element of a PCI hostdev. When writeFiltering is set to 'no', the Xen permissive setting will be enabled and guests will be able to write any values into the device's configuration space. The permissive setting remains disabled in the absense of the writeFiltering attribute, of if it is explicitly set to 'yes'. Signed-off-by: Jim Fehlig <jfehlig@suse.com> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- docs/formatdomain.rst | 7 ++++++- docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 14 ++++++++++++++ src/conf/domain_conf.h | 1 + src/libxl/libxl_conf.c | 1 + src/qemu/qemu_validate.c | 7 +++++++ tests/libxlxml2domconfigdata/moredevs-hvm.json | 6 ++++++ tests/libxlxml2domconfigdata/moredevs-hvm.xml | 5 +++++ 8 files changed, 45 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 8365fc8bbb..a7ab690b96 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3733,7 +3733,7 @@ or: ... <devices> <hostdev mode='subsystem' type='pci' managed='yes'> - <source> + <source writeFiltering='no'> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> <boot order='1'/> @@ -3899,6 +3899,11 @@ or: ``pci`` PCI devices can only be described by their ``address``. + :since:`Since 6.7.0 (Xen only)` , the ``source`` element of a PCI device + may contain the ``writeFiltering`` attribute to control write access to + the PCI configuration space. By default Xen only allows writes of known + safe values to the configuration space. Setting ``writeFiltering='no'`` + will allow all writes to the device's PCI configuration space. ``scsi`` SCSI devices are described by both the ``adapter`` and ``address`` elements. The ``address`` element includes a ``bus`` attribute (a 2-digit diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 0d0dcbc5ce..b4eb7486b9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4982,6 +4982,11 @@ <optional> <ref name="startupPolicy"/> </optional> + <optional> + <attribute name="writeFiltering"> + <ref name="virYesNo"/> + </attribute> + </optional> <element name="address"> <ref name="pciaddress"/> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8e7981bf25..bba31cfddb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8095,8 +8095,18 @@ virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node, virDomainHostdevDefPtr def, unsigned int flags) { + g_autofree char *filtering = NULL; xmlNodePtr cur; + if ((filtering = virXMLPropString(node, "writeFiltering"))) { + if ((def->writeFiltering = virTristateBoolTypeFromString(filtering)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown pci writeFiltering setting '%s'"), + filtering); + return -1; + } + } + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -26119,6 +26129,10 @@ virDomainHostdevDefFormatSubsysPCI(virBufferPtr buf, g_auto(virBuffer) origstatesChildBuf = VIR_BUFFER_INIT_CHILD(&sourceChildBuf); virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci; + if (def->writeFiltering != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(&sourceAttrBuf, " writeFiltering='%s'", + virTristateBoolTypeToString(def->writeFiltering)); + if (pcisrc->backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { const char *backend = virDomainHostdevSubsysPCIBackendTypeToString(pcisrc->backend); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 68be32614c..2f17053198 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -345,6 +345,7 @@ struct _virDomainHostdevDef { bool missing; bool readonly; bool shareable; + virTristateBool writeFiltering; union { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 7c2c015015..0056f6fe66 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -2279,6 +2279,7 @@ libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev) pcidev->bus = pcisrc->addr.bus; pcidev->dev = pcisrc->addr.slot; pcidev->func = pcisrc->addr.function; + pcidev->permissive = hostdev->writeFiltering == VIR_TRISTATE_BOOL_NO; return 0; } diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..97a9d25570 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1833,6 +1833,13 @@ qemuValidateDomainDeviceDefHostdev(const virDomainHostdevDef *hostdev, return -1; } } + + if (hostdev->writeFiltering != VIR_TRISTATE_BOOL_ABSENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Write filtering of PCI device configuration " + "space is not supported by qemu")); + return -1; + } break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.json b/tests/libxlxml2domconfigdata/moredevs-hvm.json index 7bfd68bd67..474aa2cef6 100644 --- a/tests/libxlxml2domconfigdata/moredevs-hvm.json +++ b/tests/libxlxml2domconfigdata/moredevs-hvm.json @@ -88,6 +88,12 @@ "dev": 16, "bus": 10, "rdm_policy": "invalid" + }, + { + "dev": 8, + "bus": 10, + "permissive": true, + "rdm_policy": "invalid" } ], "vfbs": [ diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.xml b/tests/libxlxml2domconfigdata/moredevs-hvm.xml index f7eb09fa3b..89ad80631d 100644 --- a/tests/libxlxml2domconfigdata/moredevs-hvm.xml +++ b/tests/libxlxml2domconfigdata/moredevs-hvm.xml @@ -48,6 +48,11 @@ <address type='pci' domain='0x0000' bus='0x0a' slot='0x10' function='0x0'/> </source> </interface> + <hostdev mode='subsystem' type='pci' managed='yes'> + <source writeFiltering='no'> + <address domain='0x0000' bus='0x0a' slot='0x08' function='0x0'/> + </source> + </hostdev> <graphics type='vnc'/> <video> <model type='cirrus' vram='8192' heads='1' primary='yes'/> -- 2.28.0
On Mon, Aug 24, 2020 at 08:26:55AM -0600, Jim Fehlig wrote:
By default Xen only allows guests to write "known safe" values into PCI configuration space, yet many devices require writes to other areas of the configuration space in order to operate properly. To allow writing any values Xen supports the 'permissive' setting, see xl.cfg(5) man page.
This change models Xen's permissive setting by adding a writeFiltering attribute on the <source> element of a PCI hostdev. When writeFiltering is set to 'no', the Xen permissive setting will be enabled and guests will be able to write any values into the device's configuration space. The permissive setting remains disabled in the absense of the writeFiltering attribute, of if it is explicitly set to 'yes'.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- docs/formatdomain.rst | 7 ++++++- docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 14 ++++++++++++++ src/conf/domain_conf.h | 1 + src/libxl/libxl_conf.c | 1 + src/qemu/qemu_validate.c | 7 +++++++ tests/libxlxml2domconfigdata/moredevs-hvm.json | 6 ++++++ tests/libxlxml2domconfigdata/moredevs-hvm.xml | 5 +++++ 8 files changed, 45 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Add support for the writeFiltering attribute in the domXML to native config converter. Also include a test. Signed-off-by: Jim Fehlig <jfehlig@suse.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/xen_common.c | 77 ++++++++++++++++++++---- tests/xlconfigdata/test-fullvirt-pci.cfg | 25 ++++++++ tests/xlconfigdata/test-fullvirt-pci.xml | 53 ++++++++++++++++ tests/xlconfigtest.c | 1 + 4 files changed, 143 insertions(+), 13 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 73ce412fe7..56702a2a76 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -373,16 +373,18 @@ xenParsePCI(char *entry) { virDomainHostdevDefPtr hostdev = NULL; VIR_AUTOSTRINGLIST tokens = NULL; + VIR_AUTOSTRINGLIST options = NULL; size_t ntokens = 0; size_t nexttoken = 0; - char *slotstr; - char *funcstr; + char *str; + char *nextstr; int domain = 0x0; int bus; int slot; int func; + virTristateBool filtered = VIR_TRISTATE_BOOL_ABSENT; - /* pci=['00:1b.0','0000:00:13.0'] */ + /* pci=['00:1b.0','0000:00:13.0,permissive=1'] */ if (!(tokens = virStringSplitCount(entry, ":", 3, &ntokens))) return NULL; @@ -398,24 +400,57 @@ xenParsePCI(char *entry) return NULL; nexttoken++; - /* slot and function */ - slotstr = tokens[nexttoken]; - if (!(funcstr = strchr(slotstr, '.'))) { + /* slot, function, and options */ + str = tokens[nexttoken]; + if (!(nextstr = strchr(str, '.'))) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Malformed PCI address %s"), slotstr); + _("Malformed PCI address %s"), str); return NULL; } - *funcstr = '\0'; - funcstr++; - if (virStrToLong_i(slotstr, NULL, 16, &slot) < 0) + *nextstr = '\0'; + nextstr++; + if (virStrToLong_i(str, NULL, 16, &slot) < 0) return NULL; - if (virStrToLong_i(funcstr, NULL, 16, &func) < 0) + str = nextstr++; + + nextstr = strchr(str, ','); + if (nextstr) { + *nextstr = '\0'; + nextstr++; + } + if (virStrToLong_i(str, NULL, 16, &func) < 0) return NULL; + str = nextstr; + if (str && (options = virStringSplit(str, ",", 0))) { + size_t i; + + for (i = 0; options[i] != NULL; i++) { + char *val; + + if (!(val = strchr(options[i], '='))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed PCI options %s"), str); + return NULL; + } + *val = '\0'; + val++; + if (STREQ(options[i], "permissive")) { + int intval; + + /* xl.cfg(5) specifies false as 0 and true as any other numeric value */ + if (virStrToLong_i(val, NULL, 10, &intval) < 0) + return NULL; + filtered = intval ? VIR_TRISTATE_BOOL_NO : VIR_TRISTATE_BOOL_YES; + } + } + } + if (!(hostdev = virDomainHostdevDefNew())) return NULL; hostdev->managed = false; + hostdev->writeFiltering = filtered; hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; hostdev->source.subsys.u.pci.addr.domain = domain; hostdev->source.subsys.u.pci.addr.bus = bus; @@ -1830,12 +1865,28 @@ xenFormatPCI(virConfPtr conf, virDomainDefPtr def) def->hostdevs[i]->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { virConfValuePtr val, tmp; char *buf; + const char *permissive_str = NULL; - buf = g_strdup_printf("%04x:%02x:%02x.%x", + switch (def->hostdevs[i]->writeFiltering) { + case VIR_TRISTATE_BOOL_YES: + permissive_str = ",permissive=0"; + break; + case VIR_TRISTATE_BOOL_NO: + permissive_str = ",permissive=1"; + break; + case VIR_TRISTATE_BOOL_ABSENT: + case VIR_TRISTATE_BOOL_LAST: + permissive_str = ""; + break; + } + + buf = g_strdup_printf("%04x:%02x:%02x.%x%s", def->hostdevs[i]->source.subsys.u.pci.addr.domain, def->hostdevs[i]->source.subsys.u.pci.addr.bus, def->hostdevs[i]->source.subsys.u.pci.addr.slot, - def->hostdevs[i]->source.subsys.u.pci.addr.function); + def->hostdevs[i]->source.subsys.u.pci.addr.function, + permissive_str); + if (VIR_ALLOC(val) < 0) { VIR_FREE(buf); diff --git a/tests/xlconfigdata/test-fullvirt-pci.cfg b/tests/xlconfigdata/test-fullvirt-pci.cfg new file mode 100644 index 0000000000..5a3f572e25 --- /dev/null +++ b/tests/xlconfigdata/test-fullvirt-pci.cfg @@ -0,0 +1,25 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +pae = 1 +acpi = 1 +apic = 1 +viridian = 0 +rtc_timeoffset = 0 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-system-i386" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +pci = [ "0000:01:1a.1", "0000:02:00.0,permissive=1" ] +parallel = "none" +serial = "none" +builder = "hvm" +boot = "d" +disk = [ "format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ] diff --git a/tests/xlconfigdata/test-fullvirt-pci.xml b/tests/xlconfigdata/test-fullvirt-pci.xml new file mode 100644 index 0000000000..75aa6ac25b --- /dev/null +++ b/tests/xlconfigdata/test-fullvirt-pci.xml @@ -0,0 +1,53 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='xenfv'>hvm</type> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='variable' adjustment='0' basis='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator> + <disk type='block' device='disk'> + <driver name='phy' type='raw'/> + <source dev='/dev/HostVG/XenGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='xenbus' index='0'/> + <controller type='ide' index='0'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1'> + <listen type='address' address='127.0.0.1'/> + </graphics> + <video> + <model type='cirrus' vram='8192' heads='1' primary='yes'/> + </video> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='xen'/> + <source> + <address domain='0x0000' bus='0x01' slot='0x1a' function='0x1'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='xen'/> + <source writeFiltering='no'> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> + </source> + </hostdev> + <memballoon model='xen'/> + </devices> +</domain> diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c index b2e045dfa5..35f437bde1 100644 --- a/tests/xlconfigtest.c +++ b/tests/xlconfigtest.c @@ -260,6 +260,7 @@ mymain(void) DO_TEST("fullvirt-nestedhvm-disabled"); DO_TEST("fullvirt-cpuid"); DO_TEST("fullvirt-acpi-slic"); + DO_TEST("fullvirt-pci"); #ifdef LIBXL_HAVE_VNUMA DO_TEST("fullvirt-vnuma"); DO_TEST_PARSE("fullvirt-vnuma-autocomplete", false); -- 2.28.0
On Mon, Aug 24, 2020 at 08:26:56AM -0600, Jim Fehlig wrote:
Add support for the writeFiltering attribute in the domXML to native config converter. Also include a test.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/xen_common.c | 77 ++++++++++++++++++++---- tests/xlconfigdata/test-fullvirt-pci.cfg | 25 ++++++++ tests/xlconfigdata/test-fullvirt-pci.xml | 53 ++++++++++++++++ tests/xlconfigtest.c | 1 + 4 files changed, 143 insertions(+), 13 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- NEWS.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 0669051ee6..46bde7ea7a 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -13,6 +13,13 @@ v6.7.0 (unreleased) * **New features** + * xen: Add ``writeFiltering`` attribute for PCI devices + + By default Xen filters guest writes to the PCI configuration space of a + PCI hostdev, which may cause problems for some devices. The ``writeFiltering`` + attribute of the device's ``<source>`` element can be used to disable the + filtering and allow all guest writes to the configuration space. + * **Improvements** * **Bug fixes** -- 2.28.0
On Mon, Aug 24, 2020 at 08:26:57AM -0600, Jim Fehlig wrote:
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- NEWS.rst | 7 +++++++ 1 file changed, 7 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
participants (2)
-
Daniel P. Berrangé -
Jim Fehlig