When an mdevctl command fails, there is not much information available to the user about why it failed. This is partly because we were not making use of the error message that mdevctl itself prints upon failure. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 46 +++++++++++++++++----------- src/node_device/node_device_driver.h | 6 ++-- tests/nodedevmdevctltest.c | 6 ++-- 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 543e5bb90a..3851a3568f 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -700,7 +700,8 @@ nodeDeviceFindAddressByName(const char *name) virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out) + char **uuid_out, + char **errmsg) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -725,15 +726,17 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, virCommandSetInputBuffer(cmd, json); virCommandSetOutputBuffer(cmd, uuid_out); + virCommandSetErrorBuffer(cmd, errmsg); return cmd; } static int -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) { int status; - g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid); + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid, + errmsg); if (!cmd) return -1; @@ -754,6 +757,7 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDefPtr def) { g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; if (!def->parent) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -761,9 +765,10 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, return NULL; } - if (virMdevctlStart(def, &uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to start mediated device")); + if (virMdevctlStart(def, &uuid, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to start mediated device '%s': %s"), def->name, + errmsg && errmsg[0] ? errmsg : "Unknown Error"); return NULL; } @@ -828,23 +833,25 @@ nodeDeviceCreateXML(virConnectPtr conn, virCommandPtr -nodeDeviceGetMdevctlStopCommand(const char *uuid) -{ - return virCommandNewArgList(MDEVCTL, - "stop", - "-u", - uuid, - NULL); +nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg) +{ + virCommandPtr cmd = virCommandNewArgList(MDEVCTL, + "stop", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; } static int -virMdevctlStop(virNodeDeviceDefPtr def) +virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { int status; g_autoptr(virCommand) cmd = NULL; - cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid); + cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid, errmsg); if (virCommandRun(cmd, &status) < 0 || status != 0) return -1; @@ -901,9 +908,12 @@ nodeDeviceDestroy(virNodeDevicePtr device) 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")); + g_autofree char *errmsg = NULL; + + if (virMdevctlStop(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': %s"), def->name, + errmsg && errmsg[0] ? errmsg : "Unknown error"); goto cleanup; } ret = 0; diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 2113d2b0a5..4a40aa51f6 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -115,6 +115,8 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out); + char **uuid_out, + char **errmsg); virCommandPtr -nodeDeviceGetMdevctlStopCommand(const char *uuid); +nodeDeviceGetMdevctlStopCommand(const char *uuid, + char **errmsg); diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 1bad65549b..c12feaac55 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -58,6 +58,7 @@ testMdevctlStart(const char *virt_type, const char *actualCmdline = NULL; int ret = -1; g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; g_autofree char *stdinbuf = NULL; g_autoptr(virCommand) cmd = NULL; @@ -66,7 +67,7 @@ testMdevctlStart(const char *virt_type, /* this function will set a stdin buffer containing the json configuration * of the device. The json value is captured in the callback above */ - cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid); + cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid, &errmsg); if (!cmd) goto cleanup; @@ -117,11 +118,12 @@ testMdevctlStop(const void *data) const char *actualCmdline = NULL; int ret = -1; g_autoptr(virCommand) cmd = NULL; + g_autofree char *errmsg = NULL; g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", abs_srcdir); - cmd = nodeDeviceGetMdevctlStopCommand(uuid); + cmd = nodeDeviceGetMdevctlStopCommand(uuid, &errmsg); if (!cmd) goto cleanup; -- 2.26.2

we will be able to define mediated devices that can be started or stopped, so we need to be able to indicate whether the device is active or not, similar to other resources (storage pools, domains, etc.) Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- src/conf/virnodedeviceobj.c | 16 ++++++++++++++++ src/conf/virnodedeviceobj.h | 7 +++++++ src/libvirt_private.syms | 2 ++ src/node_device/node_device_udev.c | 3 +++ 4 files changed, 28 insertions(+) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index c9bda77b2e..7155f77a94 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -39,6 +39,7 @@ struct _virNodeDeviceObj { virNodeDeviceDefPtr def; /* device definition */ bool skipUpdateCaps; /* whether to skip checking host caps, used by testdriver */ + bool active; }; struct _virNodeDeviceObjList { @@ -976,3 +977,18 @@ virNodeDeviceObjSetSkipUpdateCaps(virNodeDeviceObjPtr obj, { obj->skipUpdateCaps = skipUpdateCaps; } + + +bool +virNodeDeviceObjIsActive(virNodeDeviceObjPtr obj) +{ + return obj->active; +} + + +void +virNodeDeviceObjSetActive(virNodeDeviceObjPtr obj, + bool active) +{ + obj->active = active; +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 6efdb23d36..c119f4c51f 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -121,3 +121,10 @@ virNodeDeviceObjSetSkipUpdateCaps(virNodeDeviceObjPtr obj, virNodeDeviceObjPtr virNodeDeviceObjListFindMediatedDeviceByUUID(virNodeDeviceObjListPtr devs, const char *uuid); + +bool +virNodeDeviceObjIsActive(virNodeDeviceObjPtr obj); + +void +virNodeDeviceObjSetActive(virNodeDeviceObjPtr obj, + bool active); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2a3bbdc577..17461bcea0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1265,6 +1265,7 @@ virNetworkPortDefSaveStatus; # conf/virnodedeviceobj.h virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; +virNodeDeviceObjIsActive; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; @@ -1277,6 +1278,7 @@ virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; +virNodeDeviceObjSetActive; # conf/virnwfilterbindingdef.h diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 09048fb23f..3bae579a6c 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1539,6 +1539,7 @@ udevAddOneDevice(struct udev_device *device) else event = virNodeDeviceEventUpdateNew(objdef->name); + virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjEndAPI(&obj); ret = 0; @@ -1924,6 +1925,8 @@ udevSetupSystemDev(void) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + virNodeDeviceObjSetActive(obj, true); + virNodeDeviceObjEndAPI(&obj); ret = 0; -- 2.26.2

It doesn't make sense to list all of the flag values in the function documentation. This is unnecessary duplication, we already refer to the enum type. Also, remove reference to exclusive groups of flags, since that does not apply to this API. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 3 +-- src/libvirt-nodedev.c | 30 +----------------------------- 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 1a0e60b81f..2deead0791 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -60,8 +60,7 @@ int virNodeListDevices (virConnectPtr conn, /* * virConnectListAllNodeDevices: * - * Flags used to filter the returned node devices. Flags in each group - * are exclusive. */ + * Flags used to filter the returned node devices. */ typedef enum { /* filter the devices by cap type */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM = 1 << 0, /* System capability */ diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 375b907852..fb707b570f 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -78,35 +78,7 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) * objects. * * Normally, all node devices are returned; however, @flags can be used to - * filter the results for a smaller list of targeted node devices. The valid - * flags are divided into groups, where each group contains bits that - * describe mutually exclusive attributes of a node device, and where all bits - * within a group describe all possible node devices. - * - * Only one group of the @flags is provided to filter the node devices by - * capability type, flags include: - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_INTERFACE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_TARGET - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_STORAGE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_FC_HOST - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV_TYPES - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX - * VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE - * VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE + * filter the results for a smaller list of targeted node devices. * * Returns the number of node devices found or -1 and sets @devices to NULL in * case of error. On success, the array stored into @devices is guaranteed to -- 2.26.2

This function will parse the list of mediated devices that are returned by mdevctl and convert it into our internal node device representation. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 143 ++++++++++++++++++ src/node_device/node_device_driver.h | 4 + .../mdevctl-list-multiple.json | 59 ++++++++ .../mdevctl-list-multiple.out.xml | 39 +++++ tests/nodedevmdevctltest.c | 56 ++++++- 5 files changed, 299 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.json create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index a39129820d..759a788282 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -860,6 +860,149 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) } +static void mdevGenerateDeviceName(virNodeDeviceDefPtr dev) +{ + nodeDeviceGenerateName(dev, "mdev", dev->caps->data.mdev.uuid, NULL); +} + + +static virNodeDeviceDefPtr +nodeDeviceParseMdevctlChildDevice(const char *parent, + virJSONValuePtr json) +{ + virNodeDevCapMdevPtr mdev; + const char *uuid; + virJSONValuePtr props; + virJSONValuePtr attrs; + g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); + + /* the child object should have a single key equal to its uuid. + * The value is an object describing the properties of the mdev */ + if (virJSONValueObjectKeysNumber(json) != 1) + return NULL; + + uuid = virJSONValueObjectGetKey(json, 0); + props = virJSONValueObjectGetValue(json, 0); + + child->parent = g_strdup(parent); + child->caps = g_new0(virNodeDevCapsDef, 1); + child->caps->data.type = VIR_NODE_DEV_CAP_MDEV; + + mdev = &child->caps->data.mdev; + mdev->uuid = g_strdup(uuid); + mdev->type = + g_strdup(virJSONValueObjectGetString(props, "mdev_type")); + + attrs = virJSONValueObjectGet(props, "attrs"); + + if (attrs && virJSONValueIsArray(attrs)) { + size_t i; + int nattrs = virJSONValueArraySize(attrs); + + mdev->attributes = g_new0(virMediatedDeviceAttrPtr, nattrs); + mdev->nattributes = nattrs; + + for (i = 0; i < nattrs; i++) { + virJSONValuePtr attr = virJSONValueArrayGet(attrs, i); + virMediatedDeviceAttrPtr attribute; + virJSONValuePtr value; + + if (!virJSONValueIsObject(attr) || + virJSONValueObjectKeysNumber(attr) != 1) + return NULL; + + attribute = g_new0(virMediatedDeviceAttr, 1); + attribute->name = g_strdup(virJSONValueObjectGetKey(attr, 0)); + value = virJSONValueObjectGetValue(attr, 0); + attribute->value = g_strdup(virJSONValueGetString(value)); + mdev->attributes[i] = attribute; + } + } + mdevGenerateDeviceName(child); + + return g_steal_pointer(&child); +} + + +int +nodeDeviceParseMdevctlJSON(const char *jsonstring, + virNodeDeviceDefPtr **devs) +{ + int n; + g_autoptr(virJSONValue) json_devicelist = NULL; + virNodeDeviceDefPtr *outdevs = NULL; + size_t noutdevs = 0; + size_t i; + size_t j; + + json_devicelist = virJSONValueFromString(jsonstring); + + if (!json_devicelist || !virJSONValueIsArray(json_devicelist)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("mdevctl JSON response contains no devices")); + goto error; + } + + n = virJSONValueArraySize(json_devicelist); + + for (i = 0; i < n; i++) { + virJSONValuePtr obj = virJSONValueArrayGet(json_devicelist, i); + const char *parent; + virJSONValuePtr child_array; + int nchildren; + + if (!virJSONValueIsObject(obj)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Parent device is not an object")); + goto error; + } + + /* mdevctl returns an array of objects. Each object is a parent device + * object containing a single key-value pair which maps from the name + * of the parent device to an array of child devices */ + if (virJSONValueObjectKeysNumber(obj) != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected format for parent device object")); + goto error; + } + + parent = virJSONValueObjectGetKey(obj, 0); + child_array = virJSONValueObjectGetValue(obj, 0); + + if (!virJSONValueIsArray(child_array)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Parent device's JSON object data is not an array")); + goto error; + } + + nchildren = virJSONValueArraySize(child_array); + + for (j = 0; j < nchildren; j++) { + g_autoptr(virNodeDeviceDef) child = NULL; + virJSONValuePtr child_obj = virJSONValueArrayGet(child_array, j); + + if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to parse child device")); + goto error; + } + + if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0) + goto error; + } + } + + *devs = outdevs; + return noutdevs; + + error: + for (i = 0; i < noutdevs; i++) + virNodeDeviceDefFree(outdevs[i]); + VIR_FREE(outdevs); + return -1; +} + + int nodeDeviceDestroy(virNodeDevicePtr device) { diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index cc24abb342..fe11420260 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -121,6 +121,10 @@ virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); +int +nodeDeviceParseMdevctlJSON(const char *jsonstring, + virNodeDeviceDefPtr **devs); + void nodeDeviceGenerateName(virNodeDeviceDefPtr def, const char *subsystem, diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.json b/tests/nodedevmdevctldata/mdevctl-list-multiple.json new file mode 100644 index 0000000000..eefcd90c62 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.json @@ -0,0 +1,59 @@ +[ + { + "0000:00:02.0": [ + { + "200f228a-c80a-4d50-bfb7-f5a0e4e34045": { + "mdev_type": "i915-GVTg_V5_4", + "start": "manual" + } + }, + { + "de807ffc-1923-4d5f-b6c9-b20ecebc6d4b": { + "mdev_type": "i915-GVTg_V5_4", + "start": "auto" + } + }, + { + "435722ea-5f43-468a-874f-da34f1217f13": { + "mdev_type": "i915-GVTg_V5_8", + "start": "manual", + "attrs": [ + { + "testattr": "42" + } + ] + } + } + ] + }, + { + "matrix": [ + { "783e6dbb-ea0e-411f-94e2-717eaad438bf": { + "mdev_type": "vfio_ap-passthrough", + "start": "manual", + "attrs": [ + { + "assign_adapter": "5" + }, + { + "assign_adapter": "6" + }, + { + "assign_domain": "0xab" + }, + { + "assign_control_domain": "0xab" + }, + { + "assign_domain": "4" + }, + { + "assign_control_domain": "4" + } + ] + } + } + ] + } +] + diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml new file mode 100644 index 0000000000..543ad916b7 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml @@ -0,0 +1,39 @@ +<device> + <name>mdev_200f228a_c80a_4d50_bfb7_f5a0e4e34045</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_4'/> + <iommuGroup number='0'/> + </capability> +</device> +<device> + <name>mdev_de807ffc_1923_4d5f_b6c9_b20ecebc6d4b</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_4'/> + <iommuGroup number='0'/> + </capability> +</device> +<device> + <name>mdev_435722ea_5f43_468a_874f_da34f1217f13</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_8'/> + <iommuGroup number='0'/> + <attr name='testattr' value='42'/> + </capability> +</device> +<device> + <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name> + <parent>matrix</parent> + <capability type='mdev'> + <type id='vfio_ap-passthrough'/> + <iommuGroup number='0'/> + <attr name='assign_adapter' value='5'/> + <attr name='assign_adapter' value='6'/> + <attr name='assign_domain' value='0xab'/> + <attr name='assign_control_domain' value='0xab'/> + <attr name='assign_domain' value='4'/> + <attr name='assign_control_domain' value='4'/> + </capability> +</device> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 650e46a29f..a787653d21 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -145,6 +145,53 @@ testMdevctlStop(const void *data) return ret; } + +static int +testMdevctlParse(const void *data) +{ + g_autofree char *buf = NULL; + const char *filename = data; + g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s.json", + abs_srcdir, filename); + g_autofree char *xmloutfile = g_strdup_printf("%s/nodedevmdevctldata/%s.out.xml", + abs_srcdir, filename); + int ret = -1; + int nmdevs = 0; + virNodeDeviceDefPtr *mdevs = NULL; + virBuffer xmloutbuf = VIR_BUFFER_INITIALIZER; + size_t i; + + if (virFileReadAll(jsonfile, 1024*1024, &buf) < 0) { + VIR_TEST_DEBUG("Unable to read file %s", jsonfile); + return -1; + } + + if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) { + VIR_TEST_DEBUG("Unable to parse json for %s", filename); + return -1; + } + + for (i = 0; i < nmdevs; i++) { + g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]); + if (!devxml) + goto cleanup; + virBufferAddStr(&xmloutbuf, devxml); + } + + if (nodedevCompareToFile(virBufferCurrentContent(&xmloutbuf), xmloutfile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&xmloutbuf); + for (i = 0; i < nmdevs; i++) + virNodeDeviceDefFree(mdevs[i]); + g_free(mdevs); + + return ret; +} + static void nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv) { @@ -265,13 +312,13 @@ mymain(void) } #define DO_TEST_FULL(desc, func, info) \ - if (virTestRun(desc, func, &info) < 0) \ + if (virTestRun(desc, func, info) < 0) \ ret = -1; #define DO_TEST_START_FULL(virt_type, create, filename) \ do { \ struct startTestInfo info = { virt_type, create, filename }; \ - DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, info); \ + DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, &info); \ } \ while (0) @@ -281,6 +328,9 @@ mymain(void) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) +#define DO_TEST_PARSE_JSON(filename) \ + DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename) + /* Test mdevctl start commands */ DO_TEST_START("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); DO_TEST_START("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); @@ -289,6 +339,8 @@ mymain(void) /* Test mdevctl stop command, pass an arbitrary uuid */ DO_TEST_STOP("e2451f73-c95b-4124-b900-e008af37c576"); + DO_TEST_PARSE_JSON("mdevctl-list-multiple"); + done: nodedevTestDriverFree(driver); -- 2.26.2

Consistent with other objects (e.g. virDomainObj), add a field to indicate whether the node device is persistent or transient. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/virnodedeviceobj.c | 16 ++++++++++++++++ src/conf/virnodedeviceobj.h | 6 ++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 24 insertions(+) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index afcd9a46fd..f67940a696 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -40,6 +40,7 @@ struct _virNodeDeviceObj { bool skipUpdateCaps; /* whether to skip checking host caps, used by testdriver */ bool active; + bool persistent; }; struct _virNodeDeviceObjList { @@ -1003,3 +1004,18 @@ virNodeDeviceObjSetActive(virNodeDeviceObjPtr obj, { obj->active = active; } + + +bool +virNodeDeviceObjIsPersistent(virNodeDeviceObjPtr obj) +{ + return obj->persistent; +} + + +void +virNodeDeviceObjSetPersistent(virNodeDeviceObjPtr obj, + bool persistent) +{ + obj->persistent = persistent; +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index c119f4c51f..43af012103 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -128,3 +128,9 @@ virNodeDeviceObjIsActive(virNodeDeviceObjPtr obj); void virNodeDeviceObjSetActive(virNodeDeviceObjPtr obj, bool active); +bool +virNodeDeviceObjIsPersistent(virNodeDeviceObjPtr obj); + +void +virNodeDeviceObjSetPersistent(virNodeDeviceObjPtr obj, + bool persistent); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 17461bcea0..5c8a44a750 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1266,6 +1266,7 @@ virNetworkPortDefSaveStatus; virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; virNodeDeviceObjIsActive; +virNodeDeviceObjIsPersistent; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; @@ -1279,6 +1280,7 @@ virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; virNodeDeviceObjSetActive; +virNodeDeviceObjSetPersistent; # conf/virnwfilterbindingdef.h -- 2.26.2

At startup, query devices that are defined by 'mdevctl' and add them to the node device list. This adds a complication: we now have two potential sources of information for a node device: - udev for all devices and for activated mediated devices - mdevctl for persistent mediated devices Unfortunately, neither backend returns full information for a mediated device. For example, if a persistent mediated device in the list (with information provided from mdevctl) is 'started', that same device will now be detected by udev. If we simply overwrite the existing device definition with the new one provided by the udev backend, we will lose extra information that was provided by mdevctl (e.g. attributes, etc). To avoid this, make sure to copy the extra information into the new device definition. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 153 +++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++ src/node_device/node_device_udev.c | 31 +++++- 3 files changed, 185 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d3d04018be..ad21565adb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1164,3 +1164,156 @@ nodeDeviceGenerateName(virNodeDeviceDefPtr def, *(def->name + i) = '_'; } } + + +static int +virMdevctlListDefined(virNodeDeviceDefPtr **devs) +{ + int status; + g_autofree char *output = NULL; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + if (!output) + return -1; + + return nodeDeviceParseMdevctlJSON(output, devs); +} + + +int +nodeDeviceUpdateMediatedDevices(void) +{ + g_autofree virNodeDeviceDefPtr *defs = NULL; + int ndefs; + size_t i; + + if ((ndefs = virMdevctlListDefined(&defs)) < 0) { + virReportSystemError(errno, "%s", + _("failed to query mdevs from mdevctl")); + return -1; + } + + for (i = 0; i < ndefs; i++) { + virNodeDeviceObjPtr obj; + virObjectEventPtr event; + g_autoptr(virNodeDeviceDef) def = defs[i]; + g_autofree char *name = g_strdup(def->name); + bool defined = false; + + def->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + virNodeDeviceDefPtr d = g_steal_pointer(&def); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDefPtr olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, def); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + continue; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + } + + return 0; +} + + +/* returns true if any attributes were copied, else returns false */ +static bool +virMediatedDeviceAttrsCopy(virNodeDevCapMdevPtr dst, + virNodeDevCapMdevPtr src) +{ + bool ret = false; + size_t i; + + if (src->nattributes != dst->nattributes) { + ret = true; + for (i = 0; i < dst->nattributes; i++) + virMediatedDeviceAttrFree(dst->attributes[i]); + g_free(dst->attributes); + + dst->nattributes = src->nattributes; + dst->attributes = g_new0(virMediatedDeviceAttrPtr, + src->nattributes); + for (i = 0; i < dst->nattributes; i++) + dst->attributes[i] = virMediatedDeviceAttrNew(); + } + + for (i = 0; i < src->nattributes; i++) { + if (STRNEQ_NULLABLE(src->attributes[i]->name, + dst->attributes[i]->name)) { + ret = true; + g_free(dst->attributes[i]->name); + dst->attributes[i]->name = + g_strdup(src->attributes[i]->name); + } + if (STRNEQ_NULLABLE(src->attributes[i]->value, + dst->attributes[i]->value)) { + ret = true; + g_free(dst->attributes[i]->value); + dst->attributes[i]->value = + g_strdup(src->attributes[i]->value); + } + } + + return ret; +} + + +/* A mediated device definitions from mdevctl contains additional info that is + * not available from udev. Transfer this data to the new definition. + * Returns true if anything was copied, else returns false */ +bool +nodeDeviceDefCopyFromMdevctl(virNodeDeviceDefPtr dst, + virNodeDeviceDefPtr src) +{ + bool ret = false; + virNodeDevCapMdevPtr srcmdev = &src->caps->data.mdev; + virNodeDevCapMdevPtr dstmdev = &dst->caps->data.mdev; + + if (STRNEQ_NULLABLE(dstmdev->type, srcmdev->type)) { + ret = true; + g_free(dstmdev->type); + dstmdev->type = g_strdup(srcmdev->type); + } + + if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) { + ret = true; + g_free(dstmdev->uuid); + dstmdev->uuid = g_strdup(srcmdev->uuid); + } + + if (virMediatedDeviceAttrsCopy(dstmdev, srcmdev)) + ret = true; + + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 9626031fc4..1c8fc4cdc5 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -128,8 +128,14 @@ int nodeDeviceParseMdevctlJSON(const char *jsonstring, virNodeDeviceDefPtr **devs); +int +nodeDeviceUpdateMediatedDevices(void); + void nodeDeviceGenerateName(virNodeDeviceDefPtr def, const char *subsystem, const char *sysname, const char *s); + +bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDefPtr dst, + virNodeDeviceDefPtr src); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 5d1a192411..038941ec51 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1416,9 +1416,17 @@ udevRemoveOneDeviceSysPath(const char *path) VIR_NODE_DEVICE_EVENT_DELETED, 0); - VIR_DEBUG("Removing device '%s' with sysfs path '%s'", - def->name, path); - virNodeDeviceObjListRemove(driver->devs, obj); + /* If the device is a mediated device that has been 'stopped', it may still + * be defined by mdevctl and can therefore be started again. Don't drop it + * from the list of node devices */ + if (virNodeDeviceObjIsPersistent(obj)) { + VIR_FREE(def->sysfs_path); + virNodeDeviceObjSetActive(obj, false); + } else { + VIR_DEBUG("Removing device '%s' with sysfs path '%s'", + def->name, path); + virNodeDeviceObjListRemove(driver->devs, obj); + } virNodeDeviceObjEndAPI(&obj); virObjectEventStateQueue(driver->nodeDeviceEventState, event); @@ -1476,7 +1484,6 @@ udevSetParent(struct udev_device *device, return 0; } - static int udevAddOneDevice(struct udev_device *device) { @@ -1486,6 +1493,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEventPtr event = NULL; bool new_device = true; int ret = -1; + bool was_persistent = false; def = g_new0(virNodeDeviceDef, 1); @@ -1509,14 +1517,23 @@ udevAddOneDevice(struct udev_device *device) goto cleanup; if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + objdef = virNodeDeviceObjGetDef(obj); + + if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV) + nodeDeviceDefCopyFromMdevctl(def, objdef); + was_persistent = virNodeDeviceObjIsPersistent(obj); + /* If the device was defined by mdevctl and was never instantiated, it + * won't have a sysfs path. We need to emit a CREATED event... */ + new_device = (objdef->sysfs_path == NULL); + virNodeDeviceObjEndAPI(&obj); - new_device = false; } /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + virNodeDeviceObjSetPersistent(obj, was_persistent); objdef = virNodeDeviceObjGetDef(obj); if (new_device) @@ -1935,6 +1952,10 @@ nodeStateInitializeEnumerate(void *opaque) /* Populate with known devices */ if (udevEnumerateDevices(udev) != 0) goto error; + /* Load persistent mdevs (which might not be activated yet) and additional + * information about active mediated devices from mdevctl */ + if (nodeDeviceUpdateMediatedDevices() != 0) + goto error; nodeDeviceLock(); driver->initialized = true; -- 2.26.2

mdevctl does not currently provide any events when the list of defined devices changes, so we will need to poll mdevctl for the list of defined devices periodically. When a mediated device no longer exists from one iteration to the next, we need to treat it as an "undefine" event. When we get such an event, we remove the device from the list if it's not active. Otherwise, we simply mark it as non-persistent. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 67 ++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index ad21565adb..318ad43b9f 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1183,20 +1183,81 @@ virMdevctlListDefined(virNodeDeviceDefPtr **devs) } +typedef struct _virMdevctlForEachData virMdevctlForEachData; +struct _virMdevctlForEachData { + int ndefs; + virNodeDeviceDefPtr *defs; +}; + + +/* This function keeps the list of persistent mediated devices consistent + * between the nodedev driver and mdevctl. + * @obj is a device that is currently known by the nodedev driver, and @opaque + * contains the most recent list of devices defined by mdevctl. If @obj is no + * longer defined in mdevctl, mark it as undefined and possibly remove it from + * the driver as well. Returning 'true' from this function indicates that the + * device should be removed from the nodedev driver list. */ +static bool +removeMissingPersistentMdevs(virNodeDeviceObjPtr obj, + const void *opaque) +{ + bool remove = false; + const virMdevctlForEachData *data = opaque; + size_t i; + virNodeDeviceDefPtr def = virNodeDeviceObjGetDef(obj); + virObjectEventPtr event; + + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV) + return false; + + /* transient mdevs are populated via udev, so don't remove them from the + * nodedev driver just because they are not reported by by mdevctl */ + if (!virNodeDeviceObjIsPersistent(obj)) + return false; + + for (i = 0; i < data->ndefs; i++) { + /* OK, this mdev is still defined by mdevctl */ + if (STREQ(data->defs[i]->name, def->name)) + return false; + } + + event = virNodeDeviceEventLifecycleNew(def->name, + VIR_NODE_DEVICE_EVENT_UNDEFINED, + 0); + + /* The device is active, but no longer defined by mdevctl. Keep the device + * in the list, but mark it as non-persistent */ + if (virNodeDeviceObjIsActive(obj)) + virNodeDeviceObjSetPersistent(obj, false); + else + remove = true; + + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return remove; +} + + int nodeDeviceUpdateMediatedDevices(void) { g_autofree virNodeDeviceDefPtr *defs = NULL; - int ndefs; + virMdevctlForEachData data = { 0, }; size_t i; - if ((ndefs = virMdevctlListDefined(&defs)) < 0) { + if ((data.ndefs = virMdevctlListDefined(&defs)) < 0) { virReportSystemError(errno, "%s", _("failed to query mdevs from mdevctl")); return -1; } - for (i = 0; i < ndefs; i++) { + /* Any mdevs that were previously defined but were not returned in the + * latest mdevctl query should be removed from the device list */ + data.defs = defs; + virNodeDeviceObjListForEachRemove(driver->devs, + removeMissingPersistentMdevs, &data); + + for (i = 0; i < data.ndefs; i++) { virNodeDeviceObjPtr obj; virObjectEventPtr event; g_autoptr(virNodeDeviceDef) def = defs[i]; -- 2.26.2

Abstract out the function used to generate the commandline for 'mdevctl start' since they take the same arguments. Add tests to ensure that we're generating the command properly. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 34 +++++++-- src/node_device/node_device_driver.h | 6 ++ ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 1 + ...19_36ea_4111_8f0a_8c9a70e21366-define.json | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.json | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.argv | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.json | 1 + tests/nodedevmdevctltest.c | 76 +++++++++++++------ 9 files changed, 95 insertions(+), 27 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 318ad43b9f..8f0a5ca595 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -698,10 +698,14 @@ nodeDeviceFindAddressByName(const char *name) } -virCommandPtr -nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out, - char **errmsg) +/* the mdevctl 'start' and 'define' commands accept almost the exact same + * arguments, so provide a common implementation that can be wrapped by a more + * specific function */ +static virCommandPtr +nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDefPtr def, + const char *subcommand, + char **uuid_out, + char **errmsg) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -719,7 +723,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return NULL; } - cmd = virCommandNewArgList(MDEVCTL, "start", + cmd = virCommandNewArgList(MDEVCTL, subcommand, "-p", parent_addr, "--jsonfile", "/dev/stdin", NULL); @@ -731,6 +735,26 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return cmd; } +virCommandPtr +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, + char **uuid_out, + char **errmsg) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out, + errmsg); +} + +virCommandPtr +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def, + char **uuid_out, + char **errmsg) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "define", uuid_out, + errmsg); +} + + + static int virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) { diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 1c8fc4cdc5..74cc687484 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -117,6 +117,12 @@ virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, char **uuid_out, char **errmsg); + +virCommandPtr +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def, + char **uuid_out, + char **errmsg); + virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json new file mode 100644 index 0000000000..bfc6dcace3 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.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-define.argv b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json new file mode 100644 index 0000000000..e5b22b2c44 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.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-define.argv b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json new file mode 100644 index 0000000000..2e03d0bd8e --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.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 index 955732cb43..6162d593ec 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -10,10 +10,16 @@ #define VIR_FROM_THIS VIR_FROM_NODEDEV +typedef enum { + MDEVCTL_CMD_START, + MDEVCTL_CMD_DEFINE, +} MdevctlCmd; + struct startTestInfo { const char *virt_type; int create; const char *filename; + MdevctlCmd command; }; /* capture stdin passed to command */ @@ -45,12 +51,17 @@ nodedevCompareToFile(const char *actual, return virTestCompareToFile(replacedCmdline, filename); } + +typedef virCommandPtr (*MdevctlCmdFunc)(virNodeDeviceDefPtr, char **, char **); + + static int -testMdevctlStart(const char *virt_type, - int create, - const char *mdevxml, - const char *startcmdfile, - const char *startjsonfile) +testMdevctlStartOrDefine(const char *virt_type, + int create, + MdevctlCmdFunc mdevctl_cmd_func, + const char *mdevxml, + const char *cmdfile, + const char *jsonfile) { g_autoptr(virNodeDeviceDef) def = NULL; virNodeDeviceObjPtr obj = NULL; @@ -67,7 +78,7 @@ testMdevctlStart(const char *virt_type, /* this function will set a stdin buffer containing the json configuration * of the device. The json value is captured in the callback above */ - cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid, &errmsg); + cmd = mdevctl_cmd_func(def, &uuid, &errmsg); if (!cmd) goto cleanup; @@ -79,10 +90,10 @@ testMdevctlStart(const char *virt_type, if (!(actualCmdline = virBufferCurrentContent(&buf))) goto cleanup; - if (nodedevCompareToFile(actualCmdline, startcmdfile) < 0) + if (nodedevCompareToFile(actualCmdline, cmdfile) < 0) goto cleanup; - if (virTestCompareToFile(stdinbuf, startjsonfile) < 0) + if (virTestCompareToFile(stdinbuf, jsonfile) < 0) goto cleanup; ret = 0; @@ -94,20 +105,34 @@ testMdevctlStart(const char *virt_type, } static int -testMdevctlStartHelper(const void *data) +testMdevctlStartOrDefineHelper(const void *data) { const struct startTestInfo *info = data; + const char *cmd; + MdevctlCmdFunc func; + g_autofree char *mdevxml = NULL; + g_autofree char *cmdlinefile = NULL; + g_autofree char *jsonfile = NULL; + + if (info->command == MDEVCTL_CMD_START) { + cmd = "start"; + func = nodeDeviceGetMdevctlStartCommand; + } else if (info->command == MDEVCTL_CMD_DEFINE) { + cmd = "define"; + func = nodeDeviceGetMdevctlDefineCommand; + } else { + return -1; + } - 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); + mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir, + info->filename); + cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.argv", + abs_srcdir, info->filename, cmd); + jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.json", abs_srcdir, + info->filename, cmd); - return testMdevctlStart(info->virt_type, - info->create, mdevxml, cmdlinefile, - jsonfile); + return testMdevctlStartOrDefine(info->virt_type, info->create, func, + mdevxml, cmdlinefile, jsonfile); } static int @@ -349,15 +374,18 @@ mymain(void) if (virTestRun(desc, func, info) < 0) \ ret = -1; -#define DO_TEST_START_FULL(virt_type, create, filename) \ +#define DO_TEST_CMD(desc, virt_type, create, filename, command) \ do { \ - struct startTestInfo info = { virt_type, create, filename }; \ - DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, &info); \ + struct startTestInfo info = { virt_type, create, filename, command }; \ + DO_TEST_FULL(desc, testMdevctlStartOrDefineHelper, &info); \ } \ while (0) #define DO_TEST_START(filename) \ - DO_TEST_START_FULL("QEMU", CREATE_DEVICE, filename) + DO_TEST_CMD("mdevctl start " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_START) + +#define DO_TEST_DEFINE(filename) \ + DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_DEFINE) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) @@ -380,6 +408,10 @@ mymain(void) DO_TEST_PARSE_JSON("mdevctl-list-multiple"); + DO_TEST_DEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); + DO_TEST_DEFINE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); + DO_TEST_DEFINE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + done: nodedevTestDriverFree(driver); -- 2.26.2

Now that we can filter active and inactive node devices in virConnectListAllNodeDevices(), add these switches to the virsh command. Eventual output (once everything is hooked up): virsh # nodedev-list --cap mdev mdev_bd2ea955_3402_4252_8c17_7468083a0f26 virsh # nodedev-list --inactive --cap mdev mdev_07d8b8b0_7e04_4c0f_97ed_9214ce12723c mdev_927c040f_ae7d_4a35_966e_286ba6ebbe1c virsh # nodedev-list --all --cap mdev mdev_07d8b8b0_7e04_4c0f_97ed_9214ce12723c mdev_927c040f_ae7d_4a35_966e_286ba6ebbe1c mdev_bd2ea955_3402_4252_8c17_7468083a0f26 Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index b9fe9b8be1..7ab5b264fc 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -378,6 +378,14 @@ static const vshCmdOptDef opts_node_list_devices[] = { .completer = virshNodeDeviceCapabilityNameCompleter, .help = N_("capability names, separated by comma") }, + {.name = "inactive", + .type = VSH_OT_BOOL, + .help = N_("list inactive devices") + }, + {.name = "all", + .type = VSH_OT_BOOL, + .help = N_("list inactive & active devices") + }, {.name = NULL} }; @@ -393,18 +401,26 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) int ncaps = 0; virshNodeDeviceListPtr list = NULL; int cap_type = -1; + bool inactive = vshCommandOptBool(cmd, "inactive"); + bool all = vshCommandOptBool(cmd, "all"); ignore_value(vshCommandOptStringQuiet(ctl, cmd, "cap", &cap_str)); if (cap_str) { - if (tree) { - vshError(ctl, "%s", _("Options --tree and --cap are incompatible")); - return false; - } if ((ncaps = vshStringToArray(cap_str, &caps)) < 0) return false; } + if (all && inactive) { + vshError(ctl, "%s", _("Option --all is incompatible with --inactive")); + return false; + } + + if (tree && (cap_str || inactive || all)) { + vshError(ctl, "%s", _("Option --tree is incompatible with other options")); + return false; + } + for (i = 0; i < ncaps; i++) { if ((cap_type = virNodeDevCapTypeFromString(caps[i])) < 0) { vshError(ctl, "%s", _("Invalid capability type")); @@ -481,6 +497,11 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) } } + if (inactive || all) + flags |= VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE; + if (!inactive) + flags |= VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE; + if (!(list = virshNodeDeviceListCollect(ctl, caps, ncaps, flags))) { ret = false; goto cleanup; -- 2.26.2

mdevctl 'stop' and 'undefine' commands take the same uuid parameter, so refactor the test infrastructure to share common implementation for both of these commands. The 'undefine' command will be introduced in a following patch. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- tests/nodedevmdevctltest.c | 48 +++++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 6162d593ec..4be4ba82c0 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -12,7 +12,9 @@ typedef enum { MDEVCTL_CMD_START, + MDEVCTL_CMD_STOP, MDEVCTL_CMD_DEFINE, + MDEVCTL_CMD_UNDEFINE } MdevctlCmd; struct startTestInfo { @@ -135,20 +137,22 @@ testMdevctlStartOrDefineHelper(const void *data) mdevxml, cmdlinefile, jsonfile); } +typedef virCommandPtr (*GetStopUndefineCmdFunc)(const char *uuid, char **errbuf); +struct UuidCommandTestInfo { + const char *uuid; + MdevctlCmd command; +}; + static int -testMdevctlStop(const void *data) +testMdevctlUuidCommand(const char *uuid, GetStopUndefineCmdFunc func, const char *outfile) { - const char *uuid = data; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; const char *actualCmdline = NULL; int ret = -1; g_autoptr(virCommand) cmd = NULL; g_autofree char *errmsg = NULL; - g_autofree char *cmdlinefile = - g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", - abs_srcdir); - cmd = nodeDeviceGetMdevctlStopCommand(uuid, &errmsg); + cmd = func(uuid, &errmsg); if (!cmd) goto cleanup; @@ -160,7 +164,7 @@ testMdevctlStop(const void *data) if (!(actualCmdline = virBufferCurrentContent(&buf))) goto cleanup; - if (nodedevCompareToFile(actualCmdline, cmdlinefile) < 0) + if (nodedevCompareToFile(actualCmdline, outfile) < 0) goto cleanup; ret = 0; @@ -170,6 +174,27 @@ testMdevctlStop(const void *data) return ret; } +static int +testMdevctlUuidCommandHelper(const void *data) +{ + const struct UuidCommandTestInfo *info = data; + GetStopUndefineCmdFunc func; + const char *cmd; + g_autofree char *cmdlinefile = NULL; + + if (info->command == MDEVCTL_CMD_STOP) { + cmd = "stop"; + func = nodeDeviceGetMdevctlStopCommand; + } else { + return -1; + } + + cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-%s.argv", + abs_srcdir, cmd); + + return testMdevctlUuidCommand(info->uuid, func, cmdlinefile); +} + static int testMdevctlListDefined(const void *data G_GNUC_UNUSED) { @@ -387,8 +412,15 @@ mymain(void) #define DO_TEST_DEFINE(filename) \ DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_DEFINE) +#define DO_TEST_UUID_COMMAND_FULL(desc, uuid, command) \ + do { \ + struct UuidCommandTestInfo info = { uuid, command }; \ + DO_TEST_FULL(desc, testMdevctlUuidCommandHelper, &info); \ + } \ + while (0) + #define DO_TEST_STOP(uuid) \ - DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) + DO_TEST_UUID_COMMAND_FULL("mdevctl stop " uuid, uuid, MDEVCTL_CMD_STOP) #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) -- 2.26.2

Several functions accept providing a node device by name or by wwnn,wwpn pair. Extract the logic to do this into a function that can be used by both callers. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 62 +++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 2ad676fc91..ed45ceea5e 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -111,23 +111,18 @@ static const vshCmdOptDef opts_node_device_destroy[] = { {.name = NULL} }; -static bool -cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) +static virNodeDevicePtr +vshFindNodeDevice(vshControl *ctl, const char *value) { virNodeDevicePtr dev = NULL; - bool ret = false; - const char *device_value = NULL; char **arr = NULL; int narr; virshControlPtr priv = ctl->privData; - if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) - return false; - - if (strchr(device_value, ',')) { - narr = vshStringToArray(device_value, &arr); + if (strchr(value, ',')) { + narr = vshStringToArray(value, &arr); if (narr != 2) { - vshError(ctl, _("Malformed device value '%s'"), device_value); + vshError(ctl, _("Malformed device value '%s'"), value); goto cleanup; } @@ -136,14 +131,33 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) dev = virNodeDeviceLookupSCSIHostByWWN(priv->conn, arr[0], arr[1], 0); } else { - dev = virNodeDeviceLookupByName(priv->conn, device_value); + dev = virNodeDeviceLookupByName(priv->conn, value); } if (!dev) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + vshError(ctl, "%s '%s'", _("Could not find matching device"), value); goto cleanup; } + cleanup: + g_strfreev(arr); + return dev; +} + +static bool +cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) +{ + virNodeDevicePtr dev = NULL; + bool ret = false; + const char *device_value = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) + return false; + + dev = vshFindNodeDevice(ctl, device_value); + if (!dev) + goto cleanup; + if (virNodeDeviceDestroy(dev) == 0) { vshPrintExtra(ctl, _("Destroyed node device '%s'\n"), device_value); } else { @@ -153,7 +167,6 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - g_strfreev(arr); if (dev) virNodeDeviceFree(dev); return ret; @@ -578,33 +591,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) virNodeDevicePtr device = NULL; char *xml = NULL; const char *device_value = NULL; - char **arr = NULL; - int narr; bool ret = false; - virshControlPtr priv = ctl->privData; if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) return false; - if (strchr(device_value, ',')) { - narr = vshStringToArray(device_value, &arr); - if (narr != 2) { - vshError(ctl, _("Malformed device value '%s'"), device_value); - goto cleanup; - } + device = vshFindNodeDevice(ctl, device_value); - if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) - goto cleanup; - - device = virNodeDeviceLookupSCSIHostByWWN(priv->conn, arr[0], arr[1], 0); - } else { - device = virNodeDeviceLookupByName(priv->conn, device_value); - } - - if (!device) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + if (!device) goto cleanup; - } if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) goto cleanup; @@ -613,7 +608,6 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - g_strfreev(arr); VIR_FREE(xml); if (device) virNodeDeviceFree(device); -- 2.26.2

This new API function provides a way to start a persistently-defined mediate device that was defined by virNodeDeviceDefineXML() (or one that was defined externally via mdevctl) Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/driver-nodedev.h | 4 ++ src/libvirt-nodedev.c | 35 ++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 67 ++++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 +++- src/remote_protocol-structs | 4 ++ tests/nodedevmdevctldata/mdevctl-create.argv | 1 + tests/nodedevmdevctltest.c | 11 +++- 12 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 623017f1fd..cf51c3d085 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -137,6 +137,8 @@ virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, int virNodeDeviceUndefine(virNodeDevicePtr dev); +int virNodeDeviceCreate(virNodeDevicePtr dev); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index e18029ab48..d00dd5845c 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -82,6 +82,9 @@ typedef virNodeDevicePtr typedef int (*virDrvNodeDeviceUndefine)(virNodeDevicePtr dev); +typedef int +(*virDrvNodeDeviceCreate)(virNodeDevicePtr def); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -123,4 +126,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceDestroy nodeDeviceDestroy; virDrvNodeDeviceDefineXML nodeDeviceDefineXML; virDrvNodeDeviceUndefine nodeDeviceUndefine; + virDrvNodeDeviceCreate nodeDeviceCreate; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index a31f2bd124..7551b701e5 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -815,6 +815,41 @@ virNodeDeviceUndefine(virNodeDevicePtr dev) } +/** + * virNodeDeviceCreate: + * @dev: a device object + * + * Start a defined node device: + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNodeDeviceCreate(virNodeDevicePtr dev) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + virCheckReadOnlyGoto(dev->conn->flags, error); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceCreate) { + int retval = dev->conn->nodeDeviceDriver->nodeDeviceCreate(dev); + if (retval < 0) + goto error; + + return 0; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dev->conn); + return -1; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 1dec85fa80..f799db4884 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -888,6 +888,7 @@ LIBVIRT_7.2.0 { global: virNodeDeviceDefineXML; virNodeDeviceUndefine; + virNodeDeviceCreate; } LIBVIRT_7.1.0; # .... define new API here using predicted next version number .... diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 4770801ebc..e89d317152 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -901,6 +901,18 @@ nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg) return cmd; } +virCommandPtr +nodeDeviceGetMdevctlCreateCommand(const char *uuid, char **errmsg) +{ + virCommandPtr cmd = virCommandNewArgList(MDEVCTL, + "start", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; +} + static int virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { @@ -932,6 +944,21 @@ virMdevctlUndefine(virNodeDeviceDefPtr def, char **errmsg) } +static int +virMdevctlCreate(virNodeDeviceDefPtr def, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlCreateCommand(def->caps->data.mdev.uuid, errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommandPtr nodeDeviceGetMdevctlListCommand(bool defined, char **output) @@ -1257,6 +1284,46 @@ nodeDeviceUndefine(virNodeDevicePtr device) } +int nodeDeviceCreate(virNodeDevicePtr device) +{ + int ret = -1; + virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr def = NULL; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + + if (virNodeDeviceObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Device is already active")); + goto cleanup; + } + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceCreateEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlCreate(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to create mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error"); + goto cleanup; + } + ret = 0; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr device, diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 52be69c5d7..93b45e5cc8 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -157,3 +157,10 @@ nodeDeviceGenerateName(virNodeDeviceDefPtr def, bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDefPtr dst, virNodeDeviceDefPtr src); + +virCommandPtr +nodeDeviceGetMdevctlCreateCommand(const char *uuid, + char **errmsg); + +int +nodeDeviceCreate(virNodeDevicePtr dev); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 6558d6ce57..43971a3e76 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2318,6 +2318,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.2.0 */ + .nodeDeviceCreate = nodeDeviceCreate, /* 7.2.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 5c2c1a1641..507cad273d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8701,6 +8701,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */ .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ + .nodeDeviceCreate = remoteNodeDeviceCreate, /* 7.2.0 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 43f5c0cf36..755ba91676 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2158,6 +2158,10 @@ struct remote_node_device_undefine_args { remote_nonnull_string name; }; +struct remote_node_device_create_args { + remote_nonnull_string name; +}; + /* * Events Register/Deregister: @@ -6759,6 +6763,13 @@ enum remote_procedure { * @priority: high * @acl: node_device:delete */ - REMOTE_PROC_NODE_DEVICE_UNDEFINE = 428 + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 428, + + /** + * @generate: both + * @priority: high + * @acl: node_device:start + */ + REMOTE_PROC_NODE_DEVICE_CREATE = 429 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index b9074bbad0..87e0c40fac 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1610,6 +1610,9 @@ struct remote_node_device_define_xml_ret { struct remote_node_device_undefine_args { remote_nonnull_string name; }; +struct remote_node_device_create_args { + remote_nonnull_string name; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3611,4 +3614,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_MESSAGES = 426, REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 427, REMOTE_PROC_NODE_DEVICE_UNDEFINE = 428, + REMOTE_PROC_NODE_DEVICE_CREATE = 429, }; diff --git a/tests/nodedevmdevctldata/mdevctl-create.argv b/tests/nodedevmdevctldata/mdevctl-create.argv new file mode 100644 index 0000000000..802109340c --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-create.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ start -u 8a05ad83-3472-497d-8631-8142f31460e8 diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 577359cc21..324c882b53 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -14,7 +14,8 @@ typedef enum { MDEVCTL_CMD_START, MDEVCTL_CMD_STOP, MDEVCTL_CMD_DEFINE, - MDEVCTL_CMD_UNDEFINE + MDEVCTL_CMD_UNDEFINE, + MDEVCTL_CMD_CREATE, } MdevctlCmd; struct startTestInfo { @@ -188,6 +189,9 @@ testMdevctlUuidCommandHelper(const void *data) } else if (info->command == MDEVCTL_CMD_UNDEFINE) { cmd = "undefine"; func = nodeDeviceGetMdevctlUndefineCommand; + }else if (info->command == MDEVCTL_CMD_CREATE) { + cmd = "create"; + func = nodeDeviceGetMdevctlCreateCommand; } else { return -1; } @@ -428,6 +432,9 @@ mymain(void) #define DO_TEST_UNDEFINE(uuid) \ DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " uuid, uuid, MDEVCTL_CMD_UNDEFINE) +#define DO_TEST_CREATE(uuid) \ + DO_TEST_UUID_COMMAND_FULL("mdevctl create " uuid, uuid, MDEVCTL_CMD_CREATE) + #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) @@ -452,6 +459,8 @@ mymain(void) DO_TEST_UNDEFINE("d76a6b78-45ed-4149-a325-005f9abc5281"); + DO_TEST_CREATE("8a05ad83-3472-497d-8631-8142f31460e8"); + done: nodedevTestDriverFree(driver); -- 2.26.2

It will be useful to be able to specify a particular UUID for a mediated device when defining the node device. To accomodate that, allow this to be specified in the xml schema. This patch also parses and formats that value to the xml, but does not yet use it. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- docs/schemas/nodedev.rng | 43 +++++++++++-------- src/conf/node_device_conf.c | 14 ++++++ .../mdevctl-list-multiple.out.xml | 4 ++ 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 5840dc9f0d..777227c38a 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -606,27 +606,34 @@ </define> <define name="capmdev"> - <attribute name="type"> - <value>mdev</value> - </attribute> - <element name="type"> - <attribute name="id"> - <data type="string"/> + <interleave> + <attribute name="type"> + <value>mdev</value> </attribute> - </element> - <optional> - <element name="iommuGroup"> - <attribute name="number"> - <ref name="unsignedInt"/> + <element name="type"> + <attribute name="id"> + <data type="string"/> </attribute> </element> - </optional> - <zeroOrMore> - <element name="attr"> - <attribute name="name"/> - <attribute name="value"/> - </element> - </zeroOrMore> + <optional> + <element name="iommuGroup"> + <attribute name="number"> + <ref name="unsignedInt"/> + </attribute> + </element> + </optional> + <optional> + <element name="uuid"> + <ref name="UUID"/> + </element> + </optional> + <zeroOrMore> + <element name="attr"> + <attribute name="name"/> + <attribute name="value"/> + </element> + </zeroOrMore> + </interleave> </define> <define name="capccwdev"> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 1093a461af..b077d98a25 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -527,6 +527,7 @@ virNodeDeviceCapMdevDefFormat(virBufferPtr buf, size_t i; virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.type); + virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid); virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n", data->mdev.iommuGroupNumber); @@ -2024,6 +2025,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, int nattrs = 0; g_autofree xmlNodePtr *attrs = NULL; size_t i; + g_autofree char *uuidstr = NULL; ctxt->node = node; @@ -2033,6 +2035,18 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, goto out; } + if ((uuidstr = virXPathString("string(./uuid[1])", ctxt))) { + unsigned char uuidbuf[VIR_UUID_BUFLEN]; + /* make sure that the provided uuid is valid */ + if (virUUIDParse(uuidstr, uuidbuf) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid uuid '%s' for new mdev device"), uuidstr); + return -1; + } + mdev->uuid = g_new0(char, VIR_UUID_STRING_BUFLEN); + virUUIDFormat(uuidbuf, mdev->uuid); + } + /* 'iommuGroup' is optional, only report an error if the supplied value is * invalid (-2), not if it's missing (-1) */ if (virXPathUInt("number(./iommuGroup[1]/@number)", diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml index 543ad916b7..cf7e966256 100644 --- a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml @@ -3,6 +3,7 @@ <parent>0000:00:02.0</parent> <capability type='mdev'> <type id='i915-GVTg_V5_4'/> + <uuid>200f228a-c80a-4d50-bfb7-f5a0e4e34045</uuid> <iommuGroup number='0'/> </capability> </device> @@ -11,6 +12,7 @@ <parent>0000:00:02.0</parent> <capability type='mdev'> <type id='i915-GVTg_V5_4'/> + <uuid>de807ffc-1923-4d5f-b6c9-b20ecebc6d4b</uuid> <iommuGroup number='0'/> </capability> </device> @@ -19,6 +21,7 @@ <parent>0000:00:02.0</parent> <capability type='mdev'> <type id='i915-GVTg_V5_8'/> + <uuid>435722ea-5f43-468a-874f-da34f1217f13</uuid> <iommuGroup number='0'/> <attr name='testattr' value='42'/> </capability> @@ -28,6 +31,7 @@ <parent>matrix</parent> <capability type='mdev'> <type id='vfio_ap-passthrough'/> + <uuid>783e6dbb-ea0e-411f-94e2-717eaad438bf</uuid> <iommuGroup number='0'/> <attr name='assign_adapter' value='5'/> <attr name='assign_adapter' value='6'/> -- 2.26.2

Calling `mdevctl stop` for a mediated device that is in use by an active domain will block until that vm exits (or the vm closes the device). Since the nodedev driver cannot query the hypervisor driver to see whether any active domains are using the device, we resort to a workaround that relies on the fact that a vfio group can only be opened by one user at a time. If we get an EBUSY error when attempting to open the group file, we assume the device is in use and refuse to try to destroy that device. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_driver.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index a7ae3d5254..e702b30e54 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1177,8 +1177,27 @@ nodeDeviceDestroy(virNodeDevicePtr device) ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + /* If this mediated device is in use by a vm, attempting to stop it + * will block until the vm closes the device. The nodedev driver + * cannot query the hypervisor driver to determine whether the device + * is in use by any active domains, since that would introduce circular + * dependencies between daemons and add a risk of deadlocks. So we need + * to resort to a workaround. vfio only allows the group for a device + * to be opened by one user at a time. So if we get EBUSY when opening + * the group, we infer that the device is in use and therefore we + * shouldn't try to remove the device. */ + g_autofree char *vfiogroup = + virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); + VIR_AUTOCLOSE fd = open(vfiogroup, O_RDONLY); g_autofree char *errmsg = NULL; + if (fd < 0 && errno == EBUSY) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': device in use"), + def->name); + goto cleanup; + } + if (virMdevctlStop(def, &errmsg) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to destroy '%s': %s"), def->name, -- 2.26.2

To accomodate re-use of this functionality in a following patch, split out the processing of an individual mdev definition into a separate function. --- src/node_device/node_device_driver.c | 103 +++++++++++++++------------ 1 file changed, 57 insertions(+), 46 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index e702b30e54..af7d59809c 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1505,6 +1505,60 @@ removeMissingPersistentMdevs(virNodeDeviceObjPtr obj, } +/* takes ownership of @def and potentially frees it. @def should not be used + * after returning from this function */ +static int +nodeDeviceUpdateMediatedDevice(virNodeDeviceDefPtr def) +{ + virNodeDeviceObjPtr obj; + virObjectEventPtr event; + bool defined = false; + g_autoptr(virNodeDeviceDef) owned = def; + g_autofree char *name = g_strdup(owned->name); + + owned->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { + virNodeDeviceDefPtr d = g_steal_pointer(&owned); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDefPtr olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + return 0; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return 0; +} + + int nodeDeviceUpdateMediatedDevices(void) { @@ -1524,52 +1578,9 @@ nodeDeviceUpdateMediatedDevices(void) virNodeDeviceObjListForEachRemove(driver->devs, removeMissingPersistentMdevs, &data); - for (i = 0; i < data.ndefs; i++) { - virNodeDeviceObjPtr obj; - virObjectEventPtr event; - g_autoptr(virNodeDeviceDef) def = defs[i]; - g_autofree char *name = g_strdup(def->name); - bool defined = false; - - def->driver = g_strdup("vfio_mdev"); - - if (!(obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { - virNodeDeviceDefPtr d = g_steal_pointer(&def); - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { - virNodeDeviceDefFree(d); - return -1; - } - } else { - bool changed; - virNodeDeviceDefPtr olddef = virNodeDeviceObjGetDef(obj); - - defined = virNodeDeviceObjIsPersistent(obj); - /* Active devices contain some additional information (e.g. sysfs - * path) that is not provided by mdevctl, so re-use the existing - * definition and copy over new mdev data */ - changed = nodeDeviceDefCopyFromMdevctl(olddef, def); - - if (defined && !changed) { - /* if this device was already defined and the definition - * hasn't changed, there's nothing to do for this device */ - virNodeDeviceObjEndAPI(&obj); - continue; - } - } - - /* all devices returned by virMdevctlListDefined() are persistent */ - virNodeDeviceObjSetPersistent(obj, true); - - if (!defined) - event = virNodeDeviceEventLifecycleNew(name, - VIR_NODE_DEVICE_EVENT_DEFINED, - 0); - else - event = virNodeDeviceEventUpdateNew(name); - - virNodeDeviceObjEndAPI(&obj); - virObjectEventStateQueue(driver->nodeDeviceEventState, event); - } + for (i = 0; i < data.ndefs; i++) + if (nodeDeviceUpdateMediatedDevice(defs[i]) < 0) + return -1; return 0; } -- 2.26.2