[PATCH v2 1/3] libxl: Add 'permissive' option for PCI devices
From: Simon Gaiser <simon@invisiblethingslab.com> By setting the permissive flag the Xen guest access to the PCI config space is not filtered. This might be a security risk, but it's required for some devices and the IOMMU and interrupt remapping should (mostly?) contain it. Because of it (and that the attribute is Xen only), in an example include "permissive='no'" - to not expose users copying from documentation to extra risk. Example usage: <devices> ... <hostdev mode='subsystem' type='pci' managed='yes' permissive='yes'> <source> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> </hostdev> </devices> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - improve documentation (version info, example) - update schema - use virTristateBool - add a test - improve commit message - news entry --- docs/formatdomain.html.in | 7 +++++-- docs/news.xml | 11 +++++++++++- docs/schemas/domaincommon.rng | 10 ++++++++++- src/conf/domain_conf.c | 19 +++++++++++++++++++- src/conf/domain_conf.h | 1 +- src/libxl/libxl_conf.c | 1 +- tests/libxlxml2domconfigdata/moredevs-hvm.json | 6 ++++++- tests/libxlxml2domconfigdata/moredevs-hvm.xml | 5 +++++- 8 files changed, 58 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c530573..0d1146e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4987,7 +4987,7 @@ <pre> ... <devices> - <hostdev mode='subsystem' type='pci' managed='yes'> + <hostdev mode='subsystem' type='pci' managed='yes' permissive='no'> <source> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> @@ -5082,7 +5082,10 @@ (or <code>virsh nodedev-detach</code> before starting the guest or hot-plugging the device and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or - stopping the guest. + stopping the guest. When <code>permissive</code> + (<span class="since">since 6.3.0, Xen only</span>) is "yes" + the pci config space access will not be filtered. This might be + a security issue. The default is "no". </dd> <dt><code>scsi</code></dt> <dd>For SCSI devices, user is responsible to make sure the device diff --git a/docs/news.xml b/docs/news.xml index 5835013..a8e992a 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -109,6 +109,17 @@ and/or fine tuned per individual host. </description> </change> + <change> + <summary> + xen: Add support for 'permissive' PCI device option + </summary> + <description> + <code>permissive</code> is a Xen-specific PCI device option that + disables config space write filtering. It is needed for proper + functioning of some devices using non-standard config space + registers. + </description> + </change> </section> <section title="Improvements"> </section> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7f18e5b..5a71222 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3065,6 +3065,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="permissive"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave> <element name="source"> <optional> @@ -4899,6 +4904,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="permissive"> + <ref name="virYesNo"/> + </attribute> + </optional> <choice> <ref name="hostdevsubsyspci"/> <ref name="hostdevsubsysusb"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89cd8c5..fe1a864 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8434,6 +8434,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, virDomainHostdevSubsysSCSIVHostPtr scsihostsrc = &def->source.subsys.u.scsi_host; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &def->source.subsys.u.mdev; g_autofree char *managed = NULL; + g_autofree char *permissive = NULL; g_autofree char *sgio = NULL; g_autofree char *rawio = NULL; g_autofree char *backendStr = NULL; @@ -8449,6 +8450,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, if ((managed = virXMLPropString(node, "managed")) != NULL) ignore_value(virStringParseYesNo(managed, &def->managed)); + if ((permissive = virXMLPropString(node, "permissive")) != NULL) { + if ((def->permissive = virTristateBoolTypeFromString(permissive)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown hostdev permissive setting '%s'"), + permissive); + return -1; + } + } + sgio = virXMLPropString(node, "sgio"); rawio = virXMLPropString(node, "rawio"); model = virXMLPropString(node, "model"); @@ -26091,6 +26101,9 @@ virDomainActualNetDefFormat(virBufferPtr buf, virDomainHostdevDefPtr hostdef = virDomainNetGetActualHostdev(def); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (hostdef && hostdef->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(hostdef->permissive)); } if (def->trustGuestRxFilters) virBufferAsprintf(buf, " trustGuestRxFilters='%s'", @@ -26279,6 +26292,9 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<interface type='%s'", typeStr); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (hostdef && hostdef->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(hostdef->permissive)); if (def->trustGuestRxFilters) virBufferAsprintf(buf, " trustGuestRxFilters='%s'", virTristateBoolTypeToString(def->trustGuestRxFilters)); @@ -28063,6 +28079,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virBufferAsprintf(buf, " managed='%s'", def->managed ? "yes" : "no"); + if (def->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(def->permissive)); if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && scsisrc->sgio) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ecb80ef..24ec03c 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 permissive; union { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 458dfc2..23dd0e4 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -2284,6 +2284,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->permissive == VIR_TRISTATE_BOOL_YES; return 0; } diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.json b/tests/libxlxml2domconfigdata/moredevs-hvm.json index 7bfd68b..474aa2c 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 f7eb09f..1b6b738 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' permissive='yes'> + <source> + <address domain='0x0000' bus='0x0a' slot='0x08' function='0x0'/> + </source> + </hostdev> <graphics type='vnc'/> <video> <model type='cirrus' vram='8192' heads='1' primary='yes'/> base-commit: c3ace7e234ccd43d5a008d93e122e6d47cd58e17 -- git-series 0.9.1
Add support for xl.cfg(5) pci device 'permissive' option in domXML-to-xenconfig converter. And a test for it. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - new patch --- src/libxl/xen_common.c | 51 +++++++++++++++++++++--- tests/xlconfigdata/test-fullvirt-pci.cfg | 25 ++++++++++++- tests/xlconfigdata/test-fullvirt-pci.xml | 53 +++++++++++++++++++++++++- tests/xlconfigtest.c | 1 +- 4 files changed, 125 insertions(+), 5 deletions(-) create mode 100644 tests/xlconfigdata/test-fullvirt-pci.cfg create mode 100644 tests/xlconfigdata/test-fullvirt-pci.xml diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 5c37e43..050b2a0 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -381,10 +381,11 @@ xenParsePCI(char *entry) int busID; int slotID; int funcID; + virTristateBool permissivebool = VIR_TRISTATE_BOOL_ABSENT; domain[0] = bus[0] = slot[0] = func[0] = '\0'; - /* pci=['0000:00:1b.0','0000:00:13.0'] */ + /* pci=['0000:00:1b.0','0000:00:13.0,permissive=1'] */ if (!(key = entry)) return NULL; if (!(nextkey = strchr(key, ':'))) @@ -414,14 +415,38 @@ xenParsePCI(char *entry) } key = nextkey + 1; - if (strlen(key) != 1) + if (!(nextkey = strchrnul(key, ','))) return NULL; - if (virStrncpy(func, key, 1, sizeof(func)) < 0) { + if (virStrncpy(func, key, (nextkey - key), sizeof(func)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Function %s too big for destination"), key); return NULL; } + /* options */ + while (*nextkey != '\0') { + char *data; + key = nextkey + 1; + if (!(data = strchr(key, '='))) + return NULL; + data++; + if (!(nextkey = strchrnul(key, ','))) + return NULL; + if (STRPREFIX(key, "permissive=")) { + char valuestr[5]; + int valueint; + if (virStrncpy(valuestr, data, (nextkey - data), sizeof(valuestr)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Permissive %s too big for destination"), data); + return NULL; + } + /* xl.cfg(5) specifies false as 0 and true as any other numeric value */ + if (virStrToLong_i(valuestr, NULL, 10, &valueint) < 0) + return NULL; + permissivebool = valueint ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + } + } + if (virStrToLong_i(domain, NULL, 16, &domainID) < 0) return NULL; if (virStrToLong_i(bus, NULL, 16, &busID) < 0) @@ -435,6 +460,7 @@ xenParsePCI(char *entry) return NULL; hostdev->managed = false; + hostdev->permissive = permissivebool; hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; hostdev->source.subsys.u.pci.addr.domain = domainID; hostdev->source.subsys.u.pci.addr.bus = busID; @@ -1857,12 +1883,27 @@ 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; + + switch (def->hostdevs[i]->permissive) { + case VIR_TRISTATE_BOOL_YES: + permissive_str = ",permissive=1"; + break; + case VIR_TRISTATE_BOOL_NO: + permissive_str = ",permissive=0"; + break; + case VIR_TRISTATE_BOOL_ABSENT: + case VIR_TRISTATE_BOOL_LAST: + permissive_str = ""; + break; + } - buf = g_strdup_printf("%04x:%02x:%02x.%x", + 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 0000000..5a3f572 --- /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 0000000..dec390a --- /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' permissive='yes'> + <driver name='xen'/> + <source> + <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 b2e045d..35f437b 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); -- git-series 0.9.1
On 4/24/20 9:07 PM, Marek Marczykowski-Górecki wrote:
Add support for xl.cfg(5) pci device 'permissive' option in domXML-to-xenconfig converter. And a test for it.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - new patch --- src/libxl/xen_common.c | 51 +++++++++++++++++++++--- tests/xlconfigdata/test-fullvirt-pci.cfg | 25 ++++++++++++- tests/xlconfigdata/test-fullvirt-pci.xml | 53 +++++++++++++++++++++++++- tests/xlconfigtest.c | 1 +- 4 files changed, 125 insertions(+), 5 deletions(-) create mode 100644 tests/xlconfigdata/test-fullvirt-pci.cfg create mode 100644 tests/xlconfigdata/test-fullvirt-pci.xml
Assuming we are fine with the 'permissive' attribute name, this patch looks good. Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 5c37e43..050b2a0 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -381,10 +381,11 @@ xenParsePCI(char *entry) int busID; int slotID; int funcID; + virTristateBool permissivebool = VIR_TRISTATE_BOOL_ABSENT;
domain[0] = bus[0] = slot[0] = func[0] = '\0';
- /* pci=['0000:00:1b.0','0000:00:13.0'] */ + /* pci=['0000:00:1b.0','0000:00:13.0,permissive=1'] */ if (!(key = entry)) return NULL; if (!(nextkey = strchr(key, ':'))) @@ -414,14 +415,38 @@ xenParsePCI(char *entry) }
key = nextkey + 1; - if (strlen(key) != 1) + if (!(nextkey = strchrnul(key, ','))) return NULL; - if (virStrncpy(func, key, 1, sizeof(func)) < 0) { + if (virStrncpy(func, key, (nextkey - key), sizeof(func)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Function %s too big for destination"), key); return NULL; }
+ /* options */ + while (*nextkey != '\0') { + char *data; + key = nextkey + 1; + if (!(data = strchr(key, '='))) + return NULL; + data++; + if (!(nextkey = strchrnul(key, ','))) + return NULL; + if (STRPREFIX(key, "permissive=")) { + char valuestr[5]; + int valueint; + if (virStrncpy(valuestr, data, (nextkey - data), sizeof(valuestr)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Permissive %s too big for destination"), data); + return NULL; + } + /* xl.cfg(5) specifies false as 0 and true as any other numeric value */ + if (virStrToLong_i(valuestr, NULL, 10, &valueint) < 0) + return NULL; + permissivebool = valueint ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + } + } + if (virStrToLong_i(domain, NULL, 16, &domainID) < 0) return NULL; if (virStrToLong_i(bus, NULL, 16, &busID) < 0) @@ -435,6 +460,7 @@ xenParsePCI(char *entry) return NULL;
hostdev->managed = false; + hostdev->permissive = permissivebool; hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; hostdev->source.subsys.u.pci.addr.domain = domainID; hostdev->source.subsys.u.pci.addr.bus = busID; @@ -1857,12 +1883,27 @@ 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; + + switch (def->hostdevs[i]->permissive) { + case VIR_TRISTATE_BOOL_YES: + permissive_str = ",permissive=1"; + break; + case VIR_TRISTATE_BOOL_NO: + permissive_str = ",permissive=0"; + break; + case VIR_TRISTATE_BOOL_ABSENT: + case VIR_TRISTATE_BOOL_LAST: + permissive_str = ""; + break; + }
- buf = g_strdup_printf("%04x:%02x:%02x.%x", + 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 0000000..5a3f572 --- /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 0000000..dec390a --- /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' permissive='yes'> + <driver name='xen'/> + <source> + <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 b2e045d..35f437b 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);
Add support for xl.cfg(5) pci device 'seize' option in domXML-to-xenconfig converter. And a test for it. It is functional equivalent of 'managed' attribute of a hostdev, so map it directly. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - new patch --- src/libxl/xen_common.c | 19 +++++++++++++------ tests/xlconfigdata/test-fullvirt-pci.cfg | 2 +- tests/xlconfigdata/test-fullvirt-pci.xml | 2 +- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 050b2a0..8ae4aaa 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -382,6 +382,7 @@ xenParsePCI(char *entry) int slotID; int funcID; virTristateBool permissivebool = VIR_TRISTATE_BOOL_ABSENT; + virTristateBool seizebool = VIR_TRISTATE_BOOL_ABSENT; domain[0] = bus[0] = slot[0] = func[0] = '\0'; @@ -432,18 +433,23 @@ xenParsePCI(char *entry) data++; if (!(nextkey = strchrnul(key, ','))) return NULL; - if (STRPREFIX(key, "permissive=")) { + if (STRPREFIX(key, "permissive=") || STRPREFIX(key, "seize=")) { char valuestr[5]; int valueint; + virTristateBool valuebool; if (virStrncpy(valuestr, data, (nextkey - data), sizeof(valuestr)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Permissive %s too big for destination"), data); + _("%s %s too big for destination"), key, data); return NULL; } /* xl.cfg(5) specifies false as 0 and true as any other numeric value */ if (virStrToLong_i(valuestr, NULL, 10, &valueint) < 0) return NULL; - permissivebool = valueint ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + valuebool = valueint ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + if (STRPREFIX(key, "permissive=")) + permissivebool = valuebool; + else if (STRPREFIX(key, "seize=")) + seizebool = valuebool; } } @@ -459,7 +465,7 @@ xenParsePCI(char *entry) if (!(hostdev = virDomainHostdevDefNew())) return NULL; - hostdev->managed = false; + hostdev->managed = seizebool == VIR_TRISTATE_BOOL_YES; hostdev->permissive = permissivebool; hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; hostdev->source.subsys.u.pci.addr.domain = domainID; @@ -1898,12 +1904,13 @@ xenFormatPCI(virConfPtr conf, virDomainDefPtr def) break; } - buf = g_strdup_printf("%04x:%02x:%02x.%x%s", + buf = g_strdup_printf("%04x:%02x:%02x.%x%s%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, - permissive_str); + permissive_str, + def->hostdevs[i]->managed ? ",seize=1" : ""); if (VIR_ALLOC(val) < 0) { VIR_FREE(buf); diff --git a/tests/xlconfigdata/test-fullvirt-pci.cfg b/tests/xlconfigdata/test-fullvirt-pci.cfg index 5a3f572..dcf2acd 100644 --- a/tests/xlconfigdata/test-fullvirt-pci.cfg +++ b/tests/xlconfigdata/test-fullvirt-pci.cfg @@ -17,7 +17,7 @@ sdl = 0 vnc = 1 vncunused = 1 vnclisten = "127.0.0.1" -pci = [ "0000:01:1a.1", "0000:02:00.0,permissive=1" ] +pci = [ "0000:01:1a.1", "0000:02:00.0,permissive=1,seize=1" ] parallel = "none" serial = "none" builder = "hvm" diff --git a/tests/xlconfigdata/test-fullvirt-pci.xml b/tests/xlconfigdata/test-fullvirt-pci.xml index dec390a..30fa1f5 100644 --- a/tests/xlconfigdata/test-fullvirt-pci.xml +++ b/tests/xlconfigdata/test-fullvirt-pci.xml @@ -42,7 +42,7 @@ <address domain='0x0000' bus='0x01' slot='0x1a' function='0x1'/> </source> </hostdev> - <hostdev mode='subsystem' type='pci' managed='no' permissive='yes'> + <hostdev mode='subsystem' type='pci' managed='yes' permissive='yes'> <driver name='xen'/> <source> <address domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> -- git-series 0.9.1
On 4/24/20 9:07 PM, Marek Marczykowski-Górecki wrote:
Add support for xl.cfg(5) pci device 'seize' option in domXML-to-xenconfig converter. And a test for it. It is functional equivalent of 'managed' attribute of a hostdev, so map it directly.
This is fine for the converter. We'd never want to set the seize field of libxl_device_pci struct since that would interfere with libvirt's management of the device.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - new patch --- src/libxl/xen_common.c | 19 +++++++++++++------ tests/xlconfigdata/test-fullvirt-pci.cfg | 2 +- tests/xlconfigdata/test-fullvirt-pci.xml | 2 +- 3 files changed, 15 insertions(+), 8 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com> This one could really be pushed separately but it depends on the test files you added in the previous patch. That's fine though. I'll wait and push it with the others after we've vetted the permissive attribute. Regards, Jim
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 050b2a0..8ae4aaa 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -382,6 +382,7 @@ xenParsePCI(char *entry) int slotID; int funcID; virTristateBool permissivebool = VIR_TRISTATE_BOOL_ABSENT; + virTristateBool seizebool = VIR_TRISTATE_BOOL_ABSENT;
domain[0] = bus[0] = slot[0] = func[0] = '\0';
@@ -432,18 +433,23 @@ xenParsePCI(char *entry) data++; if (!(nextkey = strchrnul(key, ','))) return NULL; - if (STRPREFIX(key, "permissive=")) { + if (STRPREFIX(key, "permissive=") || STRPREFIX(key, "seize=")) { char valuestr[5]; int valueint; + virTristateBool valuebool; if (virStrncpy(valuestr, data, (nextkey - data), sizeof(valuestr)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Permissive %s too big for destination"), data); + _("%s %s too big for destination"), key, data); return NULL; } /* xl.cfg(5) specifies false as 0 and true as any other numeric value */ if (virStrToLong_i(valuestr, NULL, 10, &valueint) < 0) return NULL; - permissivebool = valueint ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + valuebool = valueint ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + if (STRPREFIX(key, "permissive=")) + permissivebool = valuebool; + else if (STRPREFIX(key, "seize=")) + seizebool = valuebool; } }
@@ -459,7 +465,7 @@ xenParsePCI(char *entry) if (!(hostdev = virDomainHostdevDefNew())) return NULL;
- hostdev->managed = false; + hostdev->managed = seizebool == VIR_TRISTATE_BOOL_YES; hostdev->permissive = permissivebool; hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; hostdev->source.subsys.u.pci.addr.domain = domainID; @@ -1898,12 +1904,13 @@ xenFormatPCI(virConfPtr conf, virDomainDefPtr def) break; }
- buf = g_strdup_printf("%04x:%02x:%02x.%x%s", + buf = g_strdup_printf("%04x:%02x:%02x.%x%s%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, - permissive_str); + permissive_str, + def->hostdevs[i]->managed ? ",seize=1" : "");
if (VIR_ALLOC(val) < 0) { VIR_FREE(buf); diff --git a/tests/xlconfigdata/test-fullvirt-pci.cfg b/tests/xlconfigdata/test-fullvirt-pci.cfg index 5a3f572..dcf2acd 100644 --- a/tests/xlconfigdata/test-fullvirt-pci.cfg +++ b/tests/xlconfigdata/test-fullvirt-pci.cfg @@ -17,7 +17,7 @@ sdl = 0 vnc = 1 vncunused = 1 vnclisten = "127.0.0.1" -pci = [ "0000:01:1a.1", "0000:02:00.0,permissive=1" ] +pci = [ "0000:01:1a.1", "0000:02:00.0,permissive=1,seize=1" ] parallel = "none" serial = "none" builder = "hvm" diff --git a/tests/xlconfigdata/test-fullvirt-pci.xml b/tests/xlconfigdata/test-fullvirt-pci.xml index dec390a..30fa1f5 100644 --- a/tests/xlconfigdata/test-fullvirt-pci.xml +++ b/tests/xlconfigdata/test-fullvirt-pci.xml @@ -42,7 +42,7 @@ <address domain='0x0000' bus='0x01' slot='0x1a' function='0x1'/> </source> </hostdev> - <hostdev mode='subsystem' type='pci' managed='no' permissive='yes'> + <hostdev mode='subsystem' type='pci' managed='yes' permissive='yes'> <driver name='xen'/> <source> <address domain='0x0000' bus='0x02' slot='0x00' function='0x0'/>
On 4/24/20 9:07 PM, Marek Marczykowski-Górecki wrote:
From: Simon Gaiser <simon@invisiblethingslab.com>
By setting the permissive flag the Xen guest access to the PCI config space is not filtered. This might be a security risk, but it's required for some devices and the IOMMU and interrupt remapping should (mostly?) contain it. Because of it (and that the attribute is Xen only), in an example include "permissive='no'" - to not expose users copying from documentation to extra risk.
Example usage:
<devices> ... <hostdev mode='subsystem' type='pci' managed='yes' permissive='yes'>
As you know I always struggle with XML modeling and naming, and IIRC the 'managed' attribute has been a source of pain in the past, so it would be nice to have someone else opine on the introduction of 'permissive' attribute. I see Laine reviewed V1 and as I recall has commented on the managed attribute in the past, so let's see if he has any advice :-). One option I considered was 'filtered' and 'unfiltered' values for a "config space" attribute, similar to sgio for scsi hostdevs. But I can't think of a good name for an attribute that "filters writes to the PCI hostdev config space".
<source> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> </hostdev> </devices>
Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - improve documentation (version info, example) - update schema - use virTristateBool - add a test - improve commit message - news entry --- docs/formatdomain.html.in | 7 +++++-- docs/news.xml | 11 +++++++++++-
Additions to news is usually done in a sparate patch.
docs/schemas/domaincommon.rng | 10 ++++++++++- src/conf/domain_conf.c | 19 +++++++++++++++++++- src/conf/domain_conf.h | 1 +- src/libxl/libxl_conf.c | 1 +- tests/libxlxml2domconfigdata/moredevs-hvm.json | 6 ++++++- tests/libxlxml2domconfigdata/moredevs-hvm.xml | 5 +++++- 8 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c530573..0d1146e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4987,7 +4987,7 @@ <pre> ... <devices> - <hostdev mode='subsystem' type='pci' managed='yes'> + <hostdev mode='subsystem' type='pci' managed='yes' permissive='no'> <source> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> @@ -5082,7 +5082,10 @@ (or <code>virsh nodedev-detach</code> before starting the guest or hot-plugging the device and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or - stopping the guest. + stopping the guest. When <code>permissive</code> + (<span class="since">since 6.3.0, Xen only</span>) is "yes"
6.4.0 Regards, Jim
+ the pci config space access will not be filtered. This might be + a security issue. The default is "no". </dd> <dt><code>scsi</code></dt> <dd>For SCSI devices, user is responsible to make sure the device diff --git a/docs/news.xml b/docs/news.xml index 5835013..a8e992a 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -109,6 +109,17 @@ and/or fine tuned per individual host. </description> </change> + <change> + <summary> + xen: Add support for 'permissive' PCI device option + </summary> + <description> + <code>permissive</code> is a Xen-specific PCI device option that + disables config space write filtering. It is needed for proper + functioning of some devices using non-standard config space + registers. + </description> + </change> </section> <section title="Improvements"> </section> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7f18e5b..5a71222 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3065,6 +3065,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="permissive"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave> <element name="source"> <optional> @@ -4899,6 +4904,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="permissive"> + <ref name="virYesNo"/> + </attribute> + </optional> <choice> <ref name="hostdevsubsyspci"/> <ref name="hostdevsubsysusb"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89cd8c5..fe1a864 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8434,6 +8434,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, virDomainHostdevSubsysSCSIVHostPtr scsihostsrc = &def->source.subsys.u.scsi_host; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &def->source.subsys.u.mdev; g_autofree char *managed = NULL; + g_autofree char *permissive = NULL; g_autofree char *sgio = NULL; g_autofree char *rawio = NULL; g_autofree char *backendStr = NULL; @@ -8449,6 +8450,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, if ((managed = virXMLPropString(node, "managed")) != NULL) ignore_value(virStringParseYesNo(managed, &def->managed));
+ if ((permissive = virXMLPropString(node, "permissive")) != NULL) { + if ((def->permissive = virTristateBoolTypeFromString(permissive)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown hostdev permissive setting '%s'"), + permissive); + return -1; + } + } + sgio = virXMLPropString(node, "sgio"); rawio = virXMLPropString(node, "rawio"); model = virXMLPropString(node, "model"); @@ -26091,6 +26101,9 @@ virDomainActualNetDefFormat(virBufferPtr buf, virDomainHostdevDefPtr hostdef = virDomainNetGetActualHostdev(def); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (hostdef && hostdef->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(hostdef->permissive)); } if (def->trustGuestRxFilters) virBufferAsprintf(buf, " trustGuestRxFilters='%s'", @@ -26279,6 +26292,9 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<interface type='%s'", typeStr); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (hostdef && hostdef->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(hostdef->permissive)); if (def->trustGuestRxFilters) virBufferAsprintf(buf, " trustGuestRxFilters='%s'", virTristateBoolTypeToString(def->trustGuestRxFilters)); @@ -28063,6 +28079,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virBufferAsprintf(buf, " managed='%s'", def->managed ? "yes" : "no"); + if (def->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(def->permissive));
if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && scsisrc->sgio) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ecb80ef..24ec03c 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 permissive; union { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 458dfc2..23dd0e4 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -2284,6 +2284,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->permissive == VIR_TRISTATE_BOOL_YES;
return 0; } diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.json b/tests/libxlxml2domconfigdata/moredevs-hvm.json index 7bfd68b..474aa2c 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 f7eb09f..1b6b738 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' permissive='yes'> + <source> + <address domain='0x0000' bus='0x0a' slot='0x08' function='0x0'/> + </source> + </hostdev> <graphics type='vnc'/> <video> <model type='cirrus' vram='8192' heads='1' primary='yes'/>
base-commit: c3ace7e234ccd43d5a008d93e122e6d47cd58e17
On 5/8/20 4:54 PM, Jim Fehlig wrote:
On 4/24/20 9:07 PM, Marek Marczykowski-Górecki wrote:
From: Simon Gaiser <simon@invisiblethingslab.com>
By setting the permissive flag the Xen guest access to the PCI config space is not filtered. This might be a security risk, but it's required for some devices and the IOMMU and interrupt remapping should (mostly?) contain it. Because of it (and that the attribute is Xen only), in an example include "permissive='no'" - to not expose users copying from documentation to extra risk.
Example usage:
<devices> ... <hostdev mode='subsystem' type='pci' managed='yes' permissive='yes'>
As you know I always struggle with XML modeling and naming, and IIRC the 'managed' attribute has been a source of pain in the past, so it would be nice to have someone else opine on the introduction of 'permissive' attribute. I see Laine reviewed V1 and as I recall has commented on the managed attribute in the past, so let's see if he has any advice :-).
Sorry, this message got lost in the firehose, and Jim just now reminded me of it on IRC. I seriously don't remember what I may have said in the past about the managed='yes' attribute. Could have been something about the default being 'yes' instead of 'no', or could have been about its placement as an attribute of the toplevel <hostdev> element rather than being made a part of the <source> subelement (since it is really associated with what is done on the host side to get the device setup and hooked into the qemu process). For this "permissive" attribute, I don't really know enough about it to know where it should go. Does it affect how the device appears in the guest? (in which case maybe it should be a part of <target>) or only the trappings around it in the host? (so maybe it should be a part of <source>). If not totally one or the other, could the setting of this option be changed and guest ABI would remain unchanged (but just some operations would now fail / succeed when they had done the opposite in the past)? In that case, again maybe <source>. I guess the answers to that would help guide me if I was making the decision about where to put the new setting. As for the *name* of it? That's even more difficult for me to have any opinion on since, again, I really have no understanding of what it's doing or why it's necessary. One thing I would ask (maybe I did before) - wherever you put it, we (people who use qemu) would *really* appreciate if you added a clause to the post-parse validation in the qemu driver to cause an error if this option is found in a qemu domain config :-)
One option I considered was 'filtered' and 'unfiltered' values for a "config space" attribute, similar to sgio for scsi hostdevs. But I can't think of a good name for an attribute that "filters writes to the PCI hostdev config space".
<source> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> </hostdev> </devices>
Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - improve documentation (version info, example) - update schema - use virTristateBool - add a test - improve commit message - news entry --- docs/formatdomain.html.in | 7 +++++-- docs/news.xml | 11 +++++++++++-
Additions to news is usually done in a sparate patch.
docs/schemas/domaincommon.rng | 10 ++++++++++- src/conf/domain_conf.c | 19 +++++++++++++++++++- src/conf/domain_conf.h | 1 +- src/libxl/libxl_conf.c | 1 +- tests/libxlxml2domconfigdata/moredevs-hvm.json | 6 ++++++- tests/libxlxml2domconfigdata/moredevs-hvm.xml | 5 +++++- 8 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c530573..0d1146e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4987,7 +4987,7 @@ <pre> ... <devices> - <hostdev mode='subsystem' type='pci' managed='yes'> + <hostdev mode='subsystem' type='pci' managed='yes' permissive='no'> <source> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> @@ -5082,7 +5082,10 @@ (or <code>virsh nodedev-detach</code> before starting the guest or hot-plugging the device and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or - stopping the guest. + stopping the guest. When <code>permissive</code> + (<span class="since">since 6.3.0, Xen only</span>) is "yes"
6.4.0
Regards, Jim
+ the pci config space access will not be filtered. This might be + a security issue. The default is "no". </dd> <dt><code>scsi</code></dt> <dd>For SCSI devices, user is responsible to make sure the device diff --git a/docs/news.xml b/docs/news.xml index 5835013..a8e992a 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -109,6 +109,17 @@ and/or fine tuned per individual host. </description> </change> + <change> + <summary> + xen: Add support for 'permissive' PCI device option + </summary> + <description> + <code>permissive</code> is a Xen-specific PCI device option that + disables config space write filtering. It is needed for proper + functioning of some devices using non-standard config space + registers. + </description> + </change> </section> <section title="Improvements"> </section> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7f18e5b..5a71222 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3065,6 +3065,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="permissive"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave> <element name="source"> <optional> @@ -4899,6 +4904,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="permissive"> + <ref name="virYesNo"/> + </attribute> + </optional> <choice> <ref name="hostdevsubsyspci"/> <ref name="hostdevsubsysusb"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89cd8c5..fe1a864 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8434,6 +8434,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, virDomainHostdevSubsysSCSIVHostPtr scsihostsrc = &def->source.subsys.u.scsi_host; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &def->source.subsys.u.mdev; g_autofree char *managed = NULL; + g_autofree char *permissive = NULL; g_autofree char *sgio = NULL; g_autofree char *rawio = NULL; g_autofree char *backendStr = NULL; @@ -8449,6 +8450,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, if ((managed = virXMLPropString(node, "managed")) != NULL) ignore_value(virStringParseYesNo(managed, &def->managed)); + if ((permissive = virXMLPropString(node, "permissive")) != NULL) { + if ((def->permissive = virTristateBoolTypeFromString(permissive)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown hostdev permissive setting '%s'"), + permissive); + return -1; + } + } + sgio = virXMLPropString(node, "sgio"); rawio = virXMLPropString(node, "rawio"); model = virXMLPropString(node, "model"); @@ -26091,6 +26101,9 @@ virDomainActualNetDefFormat(virBufferPtr buf, virDomainHostdevDefPtr hostdef = virDomainNetGetActualHostdev(def); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (hostdef && hostdef->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(hostdef->permissive)); } if (def->trustGuestRxFilters) virBufferAsprintf(buf, " trustGuestRxFilters='%s'", @@ -26279,6 +26292,9 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<interface type='%s'", typeStr); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (hostdef && hostdef->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(hostdef->permissive)); if (def->trustGuestRxFilters) virBufferAsprintf(buf, " trustGuestRxFilters='%s'", virTristateBoolTypeToString(def->trustGuestRxFilters)); @@ -28063,6 +28079,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virBufferAsprintf(buf, " managed='%s'", def->managed ? "yes" : "no"); + if (def->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(def->permissive)); if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && scsisrc->sgio) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ecb80ef..24ec03c 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 permissive; union { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 458dfc2..23dd0e4 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -2284,6 +2284,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->permissive == VIR_TRISTATE_BOOL_YES; return 0; } diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.json b/tests/libxlxml2domconfigdata/moredevs-hvm.json index 7bfd68b..474aa2c 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 f7eb09f..1b6b738 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' permissive='yes'> + <source> + <address domain='0x0000' bus='0x0a' slot='0x08' function='0x0'/> + </source> + </hostdev> <graphics type='vnc'/> <video> <model type='cirrus' vram='8192' heads='1' primary='yes'/>
base-commit: c3ace7e234ccd43d5a008d93e122e6d47cd58e17
On 5/21/20 11:01 AM, Laine Stump wrote:
On 5/8/20 4:54 PM, Jim Fehlig wrote:
On 4/24/20 9:07 PM, Marek Marczykowski-Górecki wrote:
From: Simon Gaiser <simon@invisiblethingslab.com>
By setting the permissive flag the Xen guest access to the PCI config space is not filtered. This might be a security risk, but it's required for some devices and the IOMMU and interrupt remapping should (mostly?) contain it. Because of it (and that the attribute is Xen only), in an example include "permissive='no'" - to not expose users copying from documentation to extra risk.
Example usage:
<devices> ... <hostdev mode='subsystem' type='pci' managed='yes' permissive='yes'>
As you know I always struggle with XML modeling and naming, and IIRC the 'managed' attribute has been a source of pain in the past, so it would be nice to have someone else opine on the introduction of 'permissive' attribute. I see Laine reviewed V1 and as I recall has commented on the managed attribute in the past, so let's see if he has any advice :-).
Sorry, this message got lost in the firehose, and Jim just now reminded me of it on IRC.
Hi Laine, Thanks for you comments!
I seriously don't remember what I may have said in the past about the managed='yes' attribute. Could have been something about the default being 'yes' instead of 'no', or could have been about its placement as an attribute of the toplevel <hostdev> element rather than being made a part of the <source> subelement (since it is really associated with what is done on the host side to get the device setup and hooked into the qemu process).
For this "permissive" attribute, I don't really know enough about it to know where it should go. Does it affect how the device appears in the guest? (in which case maybe it should be a part of <target>) or only the trappings around it in the host? (so maybe it should be a part of <source>). If not totally one or the other, could the setting of this option be changed and guest ABI would remain unchanged (but just some operations would now fail / succeed when they had done the opposite in the past)? In that case, again maybe <source>.
I don't follow xen development enough these days to confidently answer your questions. I'm quite sure the setting doesn't affect how the device appears in the guest, only some filtering of PCI config spaces access. Marek, can you verify that? If so, I lean towards your suggestion of adding the attribute to <source>.
I guess the answers to that would help guide me if I was making the decision about where to put the new setting.
As for the *name* of it? That's even more difficult for me to have any opinion on since, again, I really have no understanding of what it's doing or why it's necessary.
From the xl.cfg(5) man page: By default pciback only allows PV guests to write "known safe" values into PCI configuration space, likewise QEMU (both qemu-xen and qemu-xen-traditional) imposes the same constraint on HVM guests. However, many devices require writes to other areas of the configuration space in order to operate properly. This option tells the backend (pciback or QEMU) to allow all writes to the PCI configuration space of this device by this domain. It sounds more and more like a config option for the <source>. Do any of these options sound plausible? <source permissive='on|off'> <address .../> </source> <source write-filtering='on|off'> <address .../> </source> <source> <address .../> <config-space filtering='on|off'/> </source> Regards, Jim
One thing I would ask (maybe I did before) - wherever you put it, we (people who use qemu) would *really* appreciate if you added a clause to the post-parse validation in the qemu driver to cause an error if this option is found in a qemu domain config :-)
I think you mentioned it in V1. Marek, can you add such a check in the qemu driver for V3? Regards, Jim
One option I considered was 'filtered' and 'unfiltered' values for a "config space" attribute, similar to sgio for scsi hostdevs. But I can't think of a good name for an attribute that "filters writes to the PCI hostdev config space".
<source> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> </hostdev> </devices>
Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - improve documentation (version info, example) - update schema - use virTristateBool - add a test - improve commit message - news entry --- docs/formatdomain.html.in | 7 +++++-- docs/news.xml | 11 +++++++++++-
Additions to news is usually done in a sparate patch.
docs/schemas/domaincommon.rng | 10 ++++++++++- src/conf/domain_conf.c | 19 +++++++++++++++++++- src/conf/domain_conf.h | 1 +- src/libxl/libxl_conf.c | 1 +- tests/libxlxml2domconfigdata/moredevs-hvm.json | 6 ++++++- tests/libxlxml2domconfigdata/moredevs-hvm.xml | 5 +++++- 8 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c530573..0d1146e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4987,7 +4987,7 @@ <pre> ... <devices> - <hostdev mode='subsystem' type='pci' managed='yes'> + <hostdev mode='subsystem' type='pci' managed='yes' permissive='no'> <source> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/> </source> @@ -5082,7 +5082,10 @@ (or <code>virsh nodedev-detach</code> before starting the guest or hot-plugging the device and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or - stopping the guest. + stopping the guest. When <code>permissive</code> + (<span class="since">since 6.3.0, Xen only</span>) is "yes"
6.4.0
Regards, Jim
+ the pci config space access will not be filtered. This might be + a security issue. The default is "no". </dd> <dt><code>scsi</code></dt> <dd>For SCSI devices, user is responsible to make sure the device diff --git a/docs/news.xml b/docs/news.xml index 5835013..a8e992a 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -109,6 +109,17 @@ and/or fine tuned per individual host. </description> </change> + <change> + <summary> + xen: Add support for 'permissive' PCI device option + </summary> + <description> + <code>permissive</code> is a Xen-specific PCI device option that + disables config space write filtering. It is needed for proper + functioning of some devices using non-standard config space + registers. + </description> + </change> </section> <section title="Improvements"> </section> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7f18e5b..5a71222 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3065,6 +3065,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="permissive"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave> <element name="source"> <optional> @@ -4899,6 +4904,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="permissive"> + <ref name="virYesNo"/> + </attribute> + </optional> <choice> <ref name="hostdevsubsyspci"/> <ref name="hostdevsubsysusb"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89cd8c5..fe1a864 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8434,6 +8434,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, virDomainHostdevSubsysSCSIVHostPtr scsihostsrc = &def->source.subsys.u.scsi_host; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &def->source.subsys.u.mdev; g_autofree char *managed = NULL; + g_autofree char *permissive = NULL; g_autofree char *sgio = NULL; g_autofree char *rawio = NULL; g_autofree char *backendStr = NULL; @@ -8449,6 +8450,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, if ((managed = virXMLPropString(node, "managed")) != NULL) ignore_value(virStringParseYesNo(managed, &def->managed)); + if ((permissive = virXMLPropString(node, "permissive")) != NULL) { + if ((def->permissive = virTristateBoolTypeFromString(permissive)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown hostdev permissive setting '%s'"), + permissive); + return -1; + } + } + sgio = virXMLPropString(node, "sgio"); rawio = virXMLPropString(node, "rawio"); model = virXMLPropString(node, "model"); @@ -26091,6 +26101,9 @@ virDomainActualNetDefFormat(virBufferPtr buf, virDomainHostdevDefPtr hostdef = virDomainNetGetActualHostdev(def); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (hostdef && hostdef->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(hostdef->permissive)); } if (def->trustGuestRxFilters) virBufferAsprintf(buf, " trustGuestRxFilters='%s'", @@ -26279,6 +26292,9 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<interface type='%s'", typeStr); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (hostdef && hostdef->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(hostdef->permissive)); if (def->trustGuestRxFilters) virBufferAsprintf(buf, " trustGuestRxFilters='%s'", virTristateBoolTypeToString(def->trustGuestRxFilters)); @@ -28063,6 +28079,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virBufferAsprintf(buf, " managed='%s'", def->managed ? "yes" : "no"); + if (def->permissive) + virBufferAsprintf(buf, " permissive='%s'", + virTristateBoolTypeToString(def->permissive)); if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && scsisrc->sgio) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ecb80ef..24ec03c 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 permissive; union { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 458dfc2..23dd0e4 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -2284,6 +2284,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->permissive == VIR_TRISTATE_BOOL_YES; return 0; } diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.json b/tests/libxlxml2domconfigdata/moredevs-hvm.json index 7bfd68b..474aa2c 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 f7eb09f..1b6b738 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' permissive='yes'> + <source> + <address domain='0x0000' bus='0x0a' slot='0x08' function='0x0'/> + </source> + </hostdev> <graphics type='vnc'/> <video> <model type='cirrus' vram='8192' heads='1' primary='yes'/>
base-commit: c3ace7e234ccd43d5a008d93e122e6d47cd58e17
participants (3)
-
Jim Fehlig -
Laine Stump -
Marek Marczykowski-Górecki