Currently nodeDeviceCreateXML() and nodeDeviceDestroy() only support NPIV HBAs, but we want to be able to create mdev devices as well. This is a first step to enabling that support. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 91 ++++++++++++++++++---------- 1 file changed, 58 insertions(+), 33 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index ee175e1095..ba7ea50e5b 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -488,6 +488,21 @@ nodeDeviceFindNewDevice(virConnectPtr conn, } +static bool +nodeDeviceHasCapability(virNodeDeviceDefPtr def, virNodeDevCapType type) +{ + virNodeDevCapsDefPtr cap = def->caps; + + while (cap != NULL) { + if (cap->data.type == type) + return true; + cap = cap->next; + } + + return false; +} + + virNodeDevicePtr nodeDeviceCreateXML(virConnectPtr conn, const char *xmlDesc, @@ -513,24 +528,29 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virNodeDeviceCreateXMLEnsureACL(conn, def) < 0) return NULL; - if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) - return NULL; + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_SCSI_HOST)) { + if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) + return NULL; - if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def)) < 0) - return NULL; + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def)) < 0) + return NULL; - if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0) - return NULL; + if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0) + return NULL; - device = nodeDeviceFindNewDevice(conn, wwnn, wwpn); - /* We don't check the return value, because one way or another, - * we're returning what we get... */ + device = nodeDeviceFindNewDevice(conn, wwnn, wwpn); + /* We don't check the return value, because one way or another, + * we're returning what we get... */ - if (device == NULL) - virReportError(VIR_ERR_NO_NODE_DEVICE, - _("no node device for '%s' with matching " - "wwnn '%s' and wwpn '%s'"), - def->name, wwnn, wwpn); + if (device == NULL) + virReportError(VIR_ERR_NO_NODE_DEVICE, + _("no node device for '%s' with matching " + "wwnn '%s' and wwpn '%s'"), + def->name, wwnn, wwpn); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + } return device; } @@ -557,31 +577,36 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virNodeDeviceDestroyEnsureACL(device->conn, def) < 0) goto cleanup; - if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) - goto cleanup; + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_SCSI_HOST)) { + if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) + goto cleanup; - /* Because we're about to release the lock and thus run into a race - * possibility (however improbable) with a udevAddOneDevice change - * event which would essentially free the existing @def (obj->def) and - * replace it with something new, we need to grab the parent field - * and then find the parent obj in order to manage the vport */ - parent = g_strdup(def->parent); + /* Because we're about to release the lock and thus run into a race + * possibility (however improbable) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to grab the parent field + * and then find the parent obj in order to manage the vport */ + parent = g_strdup(def->parent); - virNodeDeviceObjEndAPI(&obj); + virNodeDeviceObjEndAPI(&obj); - if (!(obj = virNodeDeviceObjListFindByName(driver->devs, parent))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find parent '%s' definition"), parent); - goto cleanup; - } + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, parent))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find parent '%s' definition"), parent); + goto cleanup; + } - if (virSCSIHostGetNumber(parent, &parent_host) < 0) - goto cleanup; + if (virSCSIHostGetNumber(parent, &parent_host) < 0) + goto cleanup; - if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0) - goto cleanup; + if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0) + goto cleanup; - ret = 0; + ret = 0; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + } cleanup: virNodeDeviceObjEndAPI(&obj); -- 2.21.3

Mediated devices support arbitrary vendor-specific attributes that can be attached to a mediated device. These attributes are ordered, and are written to sysfs in order after a device is created. This patch adds support for these attributes to the mdev data types and XML schema. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- docs/formatnode.html.in | 7 +++++ docs/schemas/nodedev.rng | 6 ++++ src/conf/node_device_conf.c | 59 +++++++++++++++++++++++++++++++++++-- src/conf/node_device_conf.h | 2 ++ src/libvirt_private.syms | 2 ++ src/util/virmdev.c | 12 ++++++++ src/util/virmdev.h | 11 +++++++ 7 files changed, 96 insertions(+), 3 deletions(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 76eae928de..a46b73254b 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -393,6 +393,13 @@ which holds the IOMMU group number the mediated device belongs to. </dd> + <dt><code>attr</code></dt> + <dd> + This optional element can occur multiple times. It represents a + vendor-specific attribute that is used to configure this + mediated device. It has two required attributes: + <code>name</code> and <code>value</code>. + </dd> </dl> </dd> <dt><code>ccw</code></dt> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index fe6ffa0b53..a1ce09af54 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -634,6 +634,12 @@ <ref name='unsignedInt'/> </attribute> </element> + <zeroOrMore> + <element name="attr"> + <attribute name="name"/> + <attribute name="value"/> + </element> + </zeroOrMore> </define> <define name='capccwdev'> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index bccdbd0af8..045d146433 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -500,6 +500,21 @@ virNodeDeviceCapStorageDefFormat(virBufferPtr buf, virBufferAddLit(buf, "<capability type='hotpluggable'/>\n"); } +static void +virNodeDeviceCapMdevDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + size_t i; + + virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.type); + virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n", + data->mdev.iommuGroupNumber); + for (i = 0; i < data->mdev.nattributes; i++) { + virMediatedDeviceAttrPtr attr = data->mdev.attributes[i]; + virBufferAsprintf(buf, "<attr name='%s' value='%s'/>\n", + attr->name, attr->value); + } +} char * virNodeDeviceDefFormat(const virNodeDeviceDef *def) @@ -583,9 +598,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type)); break; case VIR_NODE_DEV_CAP_MDEV: - virBufferEscapeString(&buf, "<type id='%s'/>\n", data->mdev.type); - virBufferAsprintf(&buf, "<iommuGroup number='%u'/>\n", - data->mdev.iommuGroupNumber); + virNodeDeviceCapMdevDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_CCW_DEV: virBufferAsprintf(&buf, "<cssid>0x%x</cssid>\n", @@ -1757,6 +1770,35 @@ virNodeDevCapSystemParseXML(xmlXPathContextPtr ctxt, return ret; } +static int +virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt, + xmlNodePtr node, + virNodeDevCapMdevPtr mdev) +{ + xmlNodePtr orig_node = node; + ctxt->node = node; + int ret = -1; + g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew(); + + attr->name = virXPathString("string(./@name)", ctxt); + attr->value = virXPathString("string(./@value)", ctxt); + if (!attr->name || !attr->value) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("mdev attribute missing name or value")); + goto cleanup; + } + + if (VIR_APPEND_ELEMENT(mdev->attributes, + mdev->nattributes, + attr) < 0) + goto cleanup; + + ret = 0; + + cleanup: + ctxt->node = orig_node; + return ret; +} static int virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, @@ -1766,6 +1808,9 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, { VIR_XPATH_NODE_AUTORESTORE(ctxt); int ret = -1; + int nattrs = 0; + xmlNodePtr *attrs; + size_t i; ctxt->node = node; @@ -1783,6 +1828,11 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, "'%s'")) < 0) goto out; + if ((nattrs = virXPathNodeSet("./attr", ctxt, &attrs)) < 0) + goto out; + for (i = 0; i < nattrs; i++) + virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], mdev); + ret = 0; out: return ret; @@ -2172,6 +2222,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) break; case VIR_NODE_DEV_CAP_MDEV: VIR_FREE(data->mdev.type); + for (i = 0; i < data->mdev.nattributes; i++) + virMediatedDeviceAttrFree(data->mdev.attributes[i]); + VIR_FREE(data->mdev.attributes); break; case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_DRM: diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 9e4b0847fb..e3e1e788d4 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -141,6 +141,8 @@ typedef virNodeDevCapMdev *virNodeDevCapMdevPtr; struct _virNodeDevCapMdev { char *type; unsigned int iommuGroupNumber; + virMediatedDeviceAttrPtr *attributes; + size_t nattributes; }; typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6af44fe1c..b8e6f058c3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2486,6 +2486,8 @@ virMacMapRemove; virMacMapWriteFile; # util/virmdev.h +virMediatedDeviceAttrFree; +virMediatedDeviceAttrNew; virMediatedDeviceFree; virMediatedDeviceGetIOMMUGroupDev; virMediatedDeviceGetIOMMUGroupNum; diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 51a88a91d7..b8023dd991 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -512,3 +512,15 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath, return 0; } + +virMediatedDeviceAttrPtr virMediatedDeviceAttrNew(void) +{ + return g_new0(virMediatedDeviceAttr, 1); +} + +void virMediatedDeviceAttrFree(virMediatedDeviceAttrPtr attr) +{ + g_free(attr->name); + g_free(attr->value); + g_free(attr); +} diff --git a/src/util/virmdev.h b/src/util/virmdev.h index 51f7f608a2..eb167ccb48 100644 --- a/src/util/virmdev.h +++ b/src/util/virmdev.h @@ -37,6 +37,17 @@ typedef struct _virMediatedDevice virMediatedDevice; typedef virMediatedDevice *virMediatedDevicePtr; typedef struct _virMediatedDeviceList virMediatedDeviceList; typedef virMediatedDeviceList *virMediatedDeviceListPtr; +typedef struct _virMediatedDeviceAttr virMediatedDeviceAttr; +typedef virMediatedDeviceAttr *virMediatedDeviceAttrPtr; + +struct _virMediatedDeviceAttr { + char *name; + char *value; +}; + +virMediatedDeviceAttrPtr virMediatedDeviceAttrNew(void); +void virMediatedDeviceAttrFree(virMediatedDeviceAttrPtr attr); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceAttr, virMediatedDeviceAttrFree); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceList, virObjectUnref); -- 2.21.3

On Tue, Jun 09, 2020 at 04:43:42PM -0500, Jonathon Jongsma wrote: Only contextually related, but this patch should be preceded by one that makes iommugroup an optional element (this change would have to go to the parser as well). Since before this series, mdev XMLs were either not internally parsed at all or it would have come from inside libvirt, I'm saying this because even though we wouldn't break backwards compatibility, because we'd be relaxing the RNG and parser (which is ok), but I'd still like to see that change to take effect before this series is fully applied. With Michal's comments: Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;

On 6/9/20 11:43 PM, Jonathon Jongsma wrote: Agreed. mdevs are Linux specific and /dev/stdin is UNIX-wide understood (which is a strict superset of the former), so we are safe on that front. This will need to look slightly different - see 07/10. This virFindFileInPath() and @mdevctl variable aren't necessary. virCommandRun() will call virFindFileInPath() under the hood (see v6.4.0-66-gf603b99ad9). And also, if the function fails, it does so silently. And this doesn't report error neither.

In order to test the nodedev driver, we need to link against a non-loadable module. Similar to other loadable modules already in the repository, create an _impl library that can be linked against the unit tests and then create a loadable module from that. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/Makefile.inc.am | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/node_device/Makefile.inc.am b/src/node_device/Makefile.inc.am index 788563665f..5993165b56 100644 --- a/src/node_device/Makefile.inc.am +++ b/src/node_device/Makefile.inc.am @@ -34,34 +34,37 @@ EXTRA_DIST += \ if WITH_NODE_DEVICES # Needed to keep automake quiet about conditionals +noinst_LTLIBRARIES += libvirt_driver_nodedev_impl.la +libvirt_driver_nodedev_la_SOURCES = +libvirt_driver_nodedev_la_LIBADD = libvirt_driver_nodedev_impl.la mod_LTLIBRARIES += libvirt_driver_nodedev.la -libvirt_driver_nodedev_la_SOURCES = $(NODE_DEVICE_DRIVER_SOURCES) +libvirt_driver_nodedev_impl_la_SOURCES = $(NODE_DEVICE_DRIVER_SOURCES) -libvirt_driver_nodedev_la_CFLAGS = \ +libvirt_driver_nodedev_impl_la_CFLAGS = \ -I$(srcdir)/access \ -I$(builddir)/access \ -I$(srcdir)/conf \ $(AM_CFLAGS) \ $(LIBNL_CFLAGS) \ $(NULL) -libvirt_driver_nodedev_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF) -libvirt_driver_nodedev_la_LIBADD = \ +libvirt_driver_nodedev_impl_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF) +libvirt_driver_nodedev_impl_la_LIBADD = \ libvirt.la \ $(GLIB_LIBS) \ $(NULL) if WITH_HAL -libvirt_driver_nodedev_la_SOURCES += $(NODE_DEVICE_DRIVER_HAL_SOURCES) -libvirt_driver_nodedev_la_CFLAGS += $(HAL_CFLAGS) -libvirt_driver_nodedev_la_LIBADD += $(HAL_LIBS) +libvirt_driver_nodedev_impl_la_SOURCES += $(NODE_DEVICE_DRIVER_HAL_SOURCES) +libvirt_driver_nodedev_impl_la_CFLAGS += $(HAL_CFLAGS) +libvirt_driver_nodedev_impl_la_LIBADD += $(HAL_LIBS) endif WITH_HAL if WITH_UDEV -libvirt_driver_nodedev_la_SOURCES += $(NODE_DEVICE_DRIVER_UDEV_SOURCES) -libvirt_driver_nodedev_la_CFLAGS += \ +libvirt_driver_nodedev_impl_la_SOURCES += $(NODE_DEVICE_DRIVER_UDEV_SOURCES) +libvirt_driver_nodedev_impl_la_CFLAGS += \ $(UDEV_CFLAGS) \ $(PCIACCESS_CFLAGS) \ $(NULL) -libvirt_driver_nodedev_la_LIBADD += \ +libvirt_driver_nodedev_impl_la_LIBADD += \ $(UDEV_LIBS) \ $(PCIACCESS_LIBS) \ $(NULL) -- 2.21.3

Test that we run 'mdevctl' with the proper arguments when creating new mediated devices with virNodeDeviceCreateXML(). Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- build-aux/syntax-check.mk | 2 +- tests/Makefile.am | 14 + ...019_36ea_4111_8f0a_8c9a70e21366-start.argv | 1 + ...019_36ea_4111_8f0a_8c9a70e21366-start.json | 1 + ...d39_495e_4243_ad9f_beb3f14c23d9-start.argv | 1 + ...d39_495e_4243_ad9f_beb3f14c23d9-start.json | 1 + ...916_1ca8_49ac_b176_871d16c13076-start.argv | 1 + ...916_1ca8_49ac_b176_871d16c13076-start.json | 1 + tests/nodedevmdevctltest.c | 257 ++++++++++++++++++ ...v_d069d019_36ea_4111_8f0a_8c9a70e21366.xml | 8 + ...v_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml | 10 + ...v_fedc4916_1ca8_49ac_b176_871d16c13076.xml | 9 + 12 files changed, 305 insertions(+), 1 deletion(-) create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json create mode 100644 tests/nodedevmdevctltest.c create mode 100644 tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml create mode 100644 tests/nodedevschemadata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml create mode 100644 tests/nodedevschemadata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index bf8832a2a5..d47a92b530 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -2015,7 +2015,7 @@ exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/vir(file|event)\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c)|tools/nss/libvirt_nss_(leases|macs)\.c)$$) exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ - (^tests/(virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) + (^tests/(nodedevmdevctl|virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ (^(src/(util/(vircommand|virdaemon)|lxc/lxc_controller)|tests/testutils)\.c$$) diff --git a/tests/Makefile.am b/tests/Makefile.am index f5766a7790..13cbdbb31e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -388,6 +388,10 @@ test_programs += storagevolxml2xmltest test_programs += nodedevxml2xmltest +if WITH_NODE_DEVICES +test_programs += nodedevmdevctltest +endif WITH_NODE_DEVICES + test_programs += interfacexml2xmltest test_programs += cputest @@ -970,6 +974,16 @@ nodedevxml2xmltest_SOURCES = \ testutils.c testutils.h nodedevxml2xmltest_LDADD = $(LDADDS) +if WITH_NODE_DEVICES +nodedevmdevctltest_SOURCES = \ + nodedevmdevctltest.c \ + testutils.c testutils.h + +nodedevmdevctltest_LDADD = \ + ../src/libvirt_driver_nodedev_impl.la \ + $(LDADDS) +endif WITH_NODE_DEVICES + interfacexml2xmltest_SOURCES = \ interfacexml2xmltest.c \ testutils.c testutils.h diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv new file mode 100644 index 0000000000..dae6dedf7f --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv @@ -0,0 +1 @@ +/usr/sbin/mdevctl start -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json new file mode 100644 index 0000000000..bfc6dcace3 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual"} \ No newline at end of file diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv new file mode 100644 index 0000000000..dae6dedf7f --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv @@ -0,0 +1 @@ +/usr/sbin/mdevctl start -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json new file mode 100644 index 0000000000..e5b22b2c44 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute-1":"attribute-value-1"},{"example-attribute-2":"attribute-value-2"}]} \ No newline at end of file diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv new file mode 100644 index 0000000000..dae6dedf7f --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv @@ -0,0 +1 @@ +/usr/sbin/mdevctl start -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json new file mode 100644 index 0000000000..2e03d0bd8e --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute":"attribute-value"}]} \ No newline at end of file diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c new file mode 100644 index 0000000000..32a22246c2 --- /dev/null +++ b/tests/nodedevmdevctltest.c @@ -0,0 +1,257 @@ +#include <config.h> + +#include "internal.h" +#include "testutils.h" +#include "datatypes.h" +#include "node_device/node_device_driver.h" +#include "vircommand.h" +#define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW +#include "vircommandpriv.h" + +#define VIR_FROM_THIS VIR_FROM_NODEDEV + +struct testInfo { + bool shouldFail; + const char *virt_type; + int create; + const char *filename; +}; + +// capture stdin passed to command +static void +testCommandDryRunCallback(const char *const*args G_GNUC_UNUSED, + const char *const*env G_GNUC_UNUSED, + const char *input, + char **output G_GNUC_UNUSED, + char **error G_GNUC_UNUSED, + int *status G_GNUC_UNUSED, + void *opaque G_GNUC_UNUSED) +{ + char **stdinbuf = opaque; + + *stdinbuf = g_strdup(input); +} + +static int +testMdevctlStart(bool shouldFail G_GNUC_UNUSED, + const char *virt_type, + int create, + const char *mdevxml, + const char *startcmdfile, + const char *startjsonfile) +{ + virNodeDeviceDefPtr def = NULL; + virNodeDeviceObjPtr obj = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *actualCmdline = NULL; + int ret = -1; + g_autofree char *uuid = NULL; + g_autofree char *stdinbuf = NULL; + g_autoptr(virCommand) cmd = NULL; + + if (!(def = virNodeDeviceDefParseFile(mdevxml, create, virt_type))) + goto cleanup; + + // this function will set a stdin buffer containing the json configuration + // of the device. This json value is captured in the mock wrapper above + cmd = nodeDeviceGetMdevctlStartCommand(def, false, &uuid); + + if (!cmd) + goto cleanup; + + virCommandSetDryRun(&buf, testCommandDryRunCallback, &stdinbuf); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (!(actualCmdline = virBufferCurrentContent(&buf))) + goto cleanup; + + if (virTestCompareToFile(actualCmdline, startcmdfile) < 0) + goto cleanup; + + if (virTestCompareToFile(stdinbuf, startjsonfile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + virCommandSetDryRun(NULL, NULL, NULL); + virNodeDeviceObjEndAPI(&obj); + return ret; +} + +static int +testMdevctlStartHelper(const void *data) +{ + const struct testInfo *info = data; + + g_autofree char *mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", + abs_srcdir, info->filename); + g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.argv", + abs_srcdir, info->filename); + g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.json", + abs_srcdir, info->filename); + + return testMdevctlStart(info->shouldFail, info->virt_type, + info->create, mdevxml, cmdlinefile, + jsonfile); +} + +static void +nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv) +{ + if (!drv) + return; + + virCondDestroy(&drv->initCond); + virMutexDestroy(&drv->lock); + VIR_FREE(drv->stateDir); + VIR_FREE(drv); +} + +/* Add a fake root 'computer' device */ +static virNodeDeviceDefPtr +fakeRootDevice(void) +{ + virNodeDeviceDefPtr def = NULL; + + if (VIR_ALLOC(def) != 0 || VIR_ALLOC(def->caps) != 0) { + virNodeDeviceDefFree(def); + return NULL; + } + + def->name = g_strdup("computer"); + + return def; +} + +/* Add a fake pci device that can be used as a parent device for mediated + * devices. For our purposes, it only needs to have a name that matches the + * parent of the mdev, and it needs a PCI address + */ +static virNodeDeviceDefPtr +fakeParentDevice(void) +{ + virNodeDeviceDefPtr def = NULL; + virNodeDevCapPCIDevPtr pci_dev; + + if (VIR_ALLOC(def) != 0 || VIR_ALLOC(def->caps) != 0) { + virNodeDeviceDefFree(def); + return NULL; + } + + def->name = g_strdup("pci_0000_00_02_0"); + def->parent = g_strdup("computer"); + + def->caps->data.type = VIR_NODE_DEV_CAP_PCI_DEV; + pci_dev = &def->caps->data.pci_dev; + pci_dev->domain = 0; + pci_dev->bus = 0; + pci_dev->slot = 2; + pci_dev->function = 0; + + return def; +} + +static int +addDevice(virNodeDeviceDefPtr def) +{ + if (!def) + return -1; + + virNodeDeviceObjPtr obj = virNodeDeviceObjListAssignDef(driver->devs, def); + + if (!obj) { + virNodeDeviceDefFree(def); + return -1; + } + + virNodeDeviceObjEndAPI(&obj); + return 0; +} + +static int +nodedevTestDriverAddTestDevices(void) +{ + if (addDevice(fakeRootDevice()) < 0 || + addDevice(fakeParentDevice()) < 0) + return -1; + + return 0; +} + +/* Bare minimum driver init to be able to test nodedev functionality */ +static int +nodedevTestDriverInit(void) +{ + int ret = -1; + char statedir[] = abs_builddir "/nodedevstatedir-XXXXXX"; + if (VIR_ALLOC(driver) < 0) + return -1; + + if (!g_mkdtemp(statedir)) { + fprintf(stderr, "Cannot create fake stateDir"); + goto error; + } + + driver->stateDir = g_strdup(statedir); + driver->lockFD = -1; + if (virMutexInit(&driver->lock) < 0 || + virCondInit(&driver->initCond) < 0) { + VIR_TEST_DEBUG("Unable to initialize test nodedev driver"); + goto error; + } + + if (!(driver->devs = virNodeDeviceObjListNew())) + goto error; + + return 0; + + error: + nodedevTestDriverFree(driver); + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + + if (nodedevTestDriverInit() < 0) + return EXIT_FAILURE; + + if (nodedevTestDriverAddTestDevices() < 0) { + ret = EXIT_FAILURE; + goto done; + } + + // add a mock device to the device list so it can be used as a parent + // reference + +#define DO_TEST_FULL(shouldFail, virt_type, create, filename) \ + do { \ + struct testInfo info = { shouldFail, virt_type, create, filename, }; \ + if (virTestRun("mdevctl start " filename, \ + testMdevctlStartHelper, &info) < 0) \ + ret = -1; \ + } \ + while (0); + +#define DO_TEST(...) \ + DO_TEST_FULL(false, "QEMU", CREATE_DEVICE, __VA_ARGS__) +#define DO_TEST_FAIL(...) \ + DO_TEST_FULL(true, "QEMU", CREATE_DEVICE, __VA_ARGS__) + + // Test mdevctl start commands + DO_TEST("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); + DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); + DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + + done: + nodedevTestDriverFree(driver); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) diff --git a/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml b/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml new file mode 100644 index 0000000000..36d50630e4 --- /dev/null +++ b/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml @@ -0,0 +1,8 @@ +<device> + <name>mdev_d069d019_36ea_4111_8f0a_8c9a70e21366</name> + <parent>pci_0000_00_02_0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_8'/> + <iommuGroup number='0'/> + </capability> +</device> diff --git a/tests/nodedevschemadata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml b/tests/nodedevschemadata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml new file mode 100644 index 0000000000..1aa0afe085 --- /dev/null +++ b/tests/nodedevschemadata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml @@ -0,0 +1,10 @@ +<device> + <name>mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9</name> + <parent>pci_0000_00_02_0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_8'/> + <iommuGroup number='0'/> + <attr name='example-attribute-1' value='attribute-value-1'/> + <attr name='example-attribute-2' value='attribute-value-2'/> + </capability> +</device> diff --git a/tests/nodedevschemadata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml b/tests/nodedevschemadata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml new file mode 100644 index 0000000000..da2a702304 --- /dev/null +++ b/tests/nodedevschemadata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml @@ -0,0 +1,9 @@ +<device> + <name>mdev_fedc4916_1ca8_49ac_b176_871d16c13076</name> + <parent>pci_0000_00_02_0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_8'/> + <iommuGroup number='0'/> + <attr name='example-attribute' value='attribute-value'/> + </capability> +</device> -- 2.21.3

On Wed, 2020-06-10 at 20:01 +0200, Michal Privoznik wrote: That's a good point. But is this adequate? What if there's a distro that installs mdevctl into /usr/bin instead of /usr/sbin. Configure will find the /usr/bin/mdevctl binary and the test output will not match... How do we get around that issue for other argv tests? (e.g. qemu?) Or do we just assume that tests will fail on machines that install things in non-standard locations? Oh yes, this was leftover from the previous version of the patch where I created a temp file for the json config in the state dir. It can be dropped. Jonathon

On 6/11/20 3:40 PM, Erik Skultety wrote: Well, is iproute installed even for minimal installation? Because fedora builders certainly don't have it: https://kojipkgs.fedoraproject.org//packages/libvirt/6.1.0/4.fc32/data/lo... checking for radvd... /usr/sbin/radvd checking for tc... tc (if tc was found then the full path would be printed, like it is for radvd) and we certainly don't want to fail configure if mdevctl wasn't found. What we can do then, is to generate the expected cmd line on the fly - just like virnetdevbandwidthtest does, or skip argv[0] and compare only the rest of the args. Michal

On 6/11/20 4:32 PM, Erik Skultety wrote: Configure and spec file are two different things. If the spec file declares something as build dependency it doesn't have to be that way on other distributions. Not to mention other OSes like FreeBSD where I bet is no notion of mediated devices, or when somebody wants to build just the client (e.g. mingw). Requiring mdevctl during configure wouldn't be step in the right direction then. That might be a good step, but if we discard my suggestion to change m4/virt-external-programs.m4 and keep what Jonathon proposed, then I don't see a need for that. Configure will not hardcode any path to mdevctl and virCommandRun() will then use virFindFileInPath() to determine its location at runtime (in virExec()). In the end, I guess we should do what virnetdevbandwidthtest does and use MDEVCTL macro to construct expected output. Then, whatever value the macro has (whatever path mdevctl is found under), the expected output will have the same right one. Michal

Add the ability to destroy mdev node devices via the mdevctl utility. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 46 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 3 ++ 2 files changed, 49 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index dbc7eb4d1e..c956bb55fc 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -790,6 +790,45 @@ nodeDeviceCreateXML(virConnectPtr conn, } +virCommandPtr +nodeDeviceGetMdevctlStopCommand(const char *uuid, + bool persist) +{ + g_autofree char *mdevctl = virFindFileInPath(MDEVCTL); + const char *subcommand; + + if (!mdevctl) + return NULL; + + if (persist) + subcommand = "undefine"; + else + subcommand = "stop"; + + virCommandPtr cmd = virCommandNewArgList(mdevctl, + subcommand, + "-u", + uuid, + NULL); + + return cmd; +} + +static int +virMdevctlStop(virNodeDeviceDefPtr def) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid, false); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + int nodeDeviceDestroy(virNodeDevicePtr device) { @@ -836,6 +875,13 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0) goto cleanup; + ret = 0; + } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + if (virMdevctlStop(def) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to stop mediated device")); + goto cleanup; + } ret = 0; } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 576f75375f..794cb5dc1c 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -122,3 +122,6 @@ virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, bool persist, char **uuid_out); +virCommandPtr +nodeDeviceGetMdevctlStopCommand(const char *uuid, + bool persist); -- 2.21.3

On Tue, Jun 09, 2020 at 04:43:48PM -0500, Jonathon Jongsma wrote: "undefine" is a NOP on active mdevs IIRC, so again the helper name is confusing. Like I mentioned already, we could have a generic translator to the mdevctl subcommands. Regards, Erik

Test that we run 'mdevctl' with the proper arguments when we destroy mediated devices with virNodeDeviceDestroy() Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- tests/nodedevmdevctldata/mdevctl-stop.argv | 1 + tests/nodedevmdevctltest.c | 43 ++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 tests/nodedevmdevctldata/mdevctl-stop.argv diff --git a/tests/nodedevmdevctldata/mdevctl-stop.argv b/tests/nodedevmdevctldata/mdevctl-stop.argv new file mode 100644 index 0000000000..25ee7145ce --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-stop.argv @@ -0,0 +1 @@ +/usr/sbin/mdevctl stop -u e2451f73-c95b-4124-b900-e008af37c576 diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 32a22246c2..58ebf976e2 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -98,6 +98,42 @@ testMdevctlStartHelper(const void *data) jsonfile); } +static int +testMdevctlStopHelper(const void *data) +{ + const char *uuid = data; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *actualCmdline = NULL; + int ret = -1; + g_autoptr(virCommand) cmd = NULL; + + g_autofree char *cmdlinefile = + g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", + abs_srcdir); + + cmd = nodeDeviceGetMdevctlStopCommand(uuid, false); + + if (!cmd) + goto cleanup; + + virCommandSetDryRun(&buf, NULL, NULL); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (!(actualCmdline = virBufferCurrentContent(&buf))) + goto cleanup; + + if (virTestCompareToFile(actualCmdline, cmdlinefile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + virCommandSetDryRun(NULL, NULL, NULL); + return ret; +} + static void nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv) { @@ -248,6 +284,13 @@ mymain(void) DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + // Test mdevctl stop command, pass an arbitrary uuid + if (virTestRun("mdevctl stop", testMdevctlStopHelper, + "e2451f73-c95b-4124-b900-e008af37c576") < 0) + + + ret = -1; + done: nodedevTestDriverFree(driver); -- 2.21.3

As noted by Erik Skultety, we use the same XML schema to report existing devices and to define new devices. However, some schema elements are "read-only". In other words, they are used to report information from the node device driver and cannot be used to define a new device. Note these in the documentation. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- docs/formatnode.html.in | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index a46b73254b..651411502c 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -31,11 +31,16 @@ name is just the bus type and address, as in "pci_0000_00_02_1" or "usb_1_5_3", but some devices are able to provide more specific names, such as - "net_eth1_00_27_13_6a_fe_00". + "net_eth1_00_27_13_6a_fe_00". This is a read-only field that is + reported by the device driver. If this element is set when defining a + new device, it will be ignored. + </dd> <dt><code>path</code></dt> <dd> - Fully qualified sysfs path to the device. + Fully qualified sysfs path to the device. This is a read-only field + that is reported by the device driver. If this element is set when + defining a new device, it will be ignored. </dd> <dt><code>parent</code></dt> <dd> -- 2.21.3