With recent additions to the node device xml schema, an xml schema can
now describe a mdev device sufficiently for libvirt to create and start
the device using the mdevctl utility.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
libvirt.spec.in | 3 +
m4/virt-external-programs.m4 | 3 +
src/conf/virnodedeviceobj.c | 34 +++++
src/conf/virnodedeviceobj.h | 3 +
src/libvirt_private.syms | 1 +
src/node_device/node_device_driver.c | 177 +++++++++++++++++++++++++++
6 files changed, 221 insertions(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 6abf97df85..411846a9fc 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -466,6 +466,9 @@ Requires: dbus
# For uid creation during pre
Requires(pre): shadow-utils
+# For managing persistent mediated devices
+Recommends: mdevctl
+
%description daemon
Server side daemon required to manage the virtualization capabilities
of recent versions of Linux. Requires a hypervisor specific sub-RPM
diff --git a/m4/virt-external-programs.m4 b/m4/virt-external-programs.m4
index 9046e3bf07..bd3cb1f757 100644
--- a/m4/virt-external-programs.m4
+++ b/m4/virt-external-programs.m4
@@ -65,6 +65,7 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [
AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], [$LIBVIRT_SBIN_PATH])
AC_PATH_PROG([SCRUB], [scrub], [scrub], [$LIBVIRT_SBIN_PATH])
AC_PATH_PROG([ADDR2LINE], [addr2line], [addr2line], [$LIBVIRT_SBIN_PATH])
+ AC_PATH_PROG([MDEVCTL], [mdevctl], [mdevctl], [$LIBVIRT_SBIN_PATH])
AC_DEFINE_UNQUOTED([DMIDECODE], ["$DMIDECODE"],
[Location or name of the dmidecode program])
@@ -88,6 +89,8 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [
[Location or name of the scrub program (for wiping algorithms)])
AC_DEFINE_UNQUOTED([ADDR2LINE], ["$ADDR2LINE"],
[Location of addr2line program])
+ AC_DEFINE_UNQUOTED([MDEVCTL], ["$MDEVCTL"],
+ [Location or name of the mdevctl program])
AC_PATH_PROG([IP_PATH], [ip], [/sbin/ip], [$LIBVIRT_SBIN_PATH])
AC_DEFINE_UNQUOTED([IP_PATH], ["$IP_PATH"], [path to ip binary])
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 3a34a324ca..fd20d5f9e2 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -399,6 +399,40 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
&data);
}
+static int
+virNodeDeviceObjListFindMediatedDeviceByUUIDCallback(const void *payload,
+ const void *name G_GNUC_UNUSED,
+ const void *opaque G_GNUC_UNUSED)
+{
+ virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+ const char *uuid = (const char *) opaque;
+ virNodeDevCapsDefPtr cap;
+ int want = 0;
+
+ virObjectLock(obj);
+
+ for (cap = obj->def->caps; cap != NULL; cap = cap->next) {
+ if (cap->data.type == VIR_NODE_DEV_CAP_MDEV) {
+ if (STREQ(cap->data.mdev.uuid, uuid)) {
+ want = 1;
+ break;
+ }
+ }
+ }
+
+ virObjectUnlock(obj);
+ return want;
+}
+
+
+virNodeDeviceObjPtr
+virNodeDeviceObjListFindMediatedDeviceByUUID(virNodeDeviceObjListPtr devs,
+ const char *uuid)
+{
+ return virNodeDeviceObjListSearch(devs,
+ virNodeDeviceObjListFindMediatedDeviceByUUIDCallback,
+ uuid);
+}
static void
virNodeDeviceObjListDispose(void *obj)
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index c9df8dedab..6efdb23d36 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -118,3 +118,6 @@ virNodeDeviceObjListExport(virConnectPtr conn,
void
virNodeDeviceObjSetSkipUpdateCaps(virNodeDeviceObjPtr obj,
bool skipUpdateCaps);
+virNodeDeviceObjPtr
+virNodeDeviceObjListFindMediatedDeviceByUUID(virNodeDeviceObjListPtr devs,
+ const char *uuid);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 94b206aa21..0468f68f52 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1178,6 +1178,7 @@ virNodeDeviceObjListAssignDef;
virNodeDeviceObjListExport;
virNodeDeviceObjListFindByName;
virNodeDeviceObjListFindBySysfsPath;
+virNodeDeviceObjListFindMediatedDeviceByUUID;
virNodeDeviceObjListFindSCSIHostByWWNs;
virNodeDeviceObjListFree;
virNodeDeviceObjListGetNames;
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index f948dfbf69..c271f3bae5 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -30,6 +30,7 @@
#include "datatypes.h"
#include "viralloc.h"
#include "virfile.h"
+#include "virjson.h"
#include "virstring.h"
#include "node_device_conf.h"
#include "node_device_event.h"
@@ -40,6 +41,7 @@
#include "viraccessapicheck.h"
#include "virnetdev.h"
#include "virutil.h"
+#include "vircommand.h"
#define VIR_FROM_THIS VIR_FROM_NODEDEV
@@ -304,6 +306,30 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
return device;
}
+static virNodeDevicePtr
+nodeDeviceLookupMediatedDeviceByUUID(virConnectPtr conn,
+ const char *uuid,
+ unsigned int flags)
+{
+ virNodeDeviceObjPtr obj = NULL;
+ virNodeDeviceDefPtr def;
+ virNodeDevicePtr device = NULL;
+
+ virCheckFlags(0, NULL);
+
+ if (!(obj = virNodeDeviceObjListFindMediatedDeviceByUUID(driver->devs,
+ uuid)))
+ return NULL;
+
+ def = virNodeDeviceObjGetDef(obj);
+
+ if ((device = virGetNodeDevice(conn, def->name)))
+ device->parentName = g_strdup(def->parent);
+
+ virNodeDeviceObjEndAPI(&obj);
+ return device;
+}
+
char *
nodeDeviceGetXMLDesc(virNodeDevicePtr device,
@@ -488,6 +514,23 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
return device;
}
+static virNodeDevicePtr
+nodeDeviceFindNewMediatedDeviceFunc(virConnectPtr conn,
+ const void *opaque)
+{
+ const char *uuid = opaque;
+ return nodeDeviceLookupMediatedDeviceByUUID(conn, uuid, 0);
+}
+
+static virNodeDevicePtr
+nodeDeviceFindNewMediatedDevice(virConnectPtr conn,
+ const char *mdev_uuid)
+{
+ return nodeDeviceFindNewDevice(conn,
+ nodeDeviceFindNewMediatedDeviceFunc,
+ mdev_uuid);
+}
+
typedef struct _NewSCISHostFuncData
{
const char *wwnn;
@@ -525,6 +568,68 @@ nodeDeviceHasCapability(virNodeDeviceDefPtr def, virNodeDevCapType type)
return false;
}
+static int
+virNodeDeviceDefToMdevctlConfig(virNodeDeviceDefPtr def, char **buf)
+{
+ size_t i;
+ virNodeDevCapMdevPtr mdev = &def->caps->data.mdev;
+ g_autoptr(virJSONValue) json = virJSONValueNewObject();
+
+ if (virJSONValueObjectAppendString(json, "mdev_type", mdev->type) < 0)
+ return -1;
+ if (virJSONValueObjectAppendString(json, "start", "manual") < 0)
+ return -1;
+ if (mdev->attributes) {
+ g_autoptr(virJSONValue) attributes = virJSONValueNewArray();
+ for (i = 0; i < mdev->nattributes; i++) {
+ virMediatedDeviceAttrPtr attr = mdev->attributes[i];
+ g_autoptr(virJSONValue) jsonattr = virJSONValueNewObject();
+
+ if (virJSONValueObjectAppendString(jsonattr, attr->name, attr->value) < 0)
+ return -1;
+ if (virJSONValueArrayAppend(attributes, g_steal_pointer(&jsonattr)) < 0)
+ return -1;
+ }
+ if (virJSONValueObjectAppend(json, "attrs", g_steal_pointer(&attributes)) < 0)
+ return -1;
+ }
+
+ *buf = virJSONValueToString(json, false);
+ if (*buf == NULL)
+ return -1;
+
+ return 0;
+}
+
+static int
+virMdevctlStart(const char *parent, const char *json_file, char **uuid)
+{
+ int status;
+ g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);
+ if (!mdevctl)
+ return -1;
+
+ g_autoptr(virCommand) cmd = virCommandNewArgList(mdevctl,
+ "start",
+ "-p",
+ parent,
+ "--jsonfile",
+ json_file,
+ NULL);
+ virCommandAddEnvPassCommon(cmd);
+ virCommandSetOutputBuffer(cmd, uuid);
+
+ /* an auto-generated uuid is returned via stdout if no uuid is specified in
+ * the mdevctl args */
+ if (virCommandRun(cmd, &status) < 0 || status != 0)
+ return -1;
+
+ /* remove newline */
+ *uuid = g_strstrip(*uuid);
+
+ return 0;
+}
+
virNodeDevicePtr
nodeDeviceCreateXML(virConnectPtr conn,
const char *xmlDesc,
@@ -569,6 +674,78 @@ nodeDeviceCreateXML(virConnectPtr conn,
_("no node device for '%s' with matching "
"wwnn '%s' and wwpn '%s'"),
def->name, wwnn, wwpn);
+ } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
+ virNodeDeviceObjPtr parent_dev = NULL;
+ virNodeDeviceDefPtr parent_def = NULL;
+ virNodeDevCapsDefPtr parent_caps = NULL;
+ g_autofree char *uuid = NULL;
+ g_autofree char *parent_pci = NULL;
+
+ if (def->parent == NULL) {
+ virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
+ _("cannot create a mediated device without a parent"));
+ return NULL;
+ }
+
+ if ((parent_dev = virNodeDeviceObjListFindByName(driver->devs, def->parent)) == NULL) {
+ virReportError(VIR_ERR_NO_NODE_DEVICE,
+ _("could not find parent device '%s'"), def->parent);
+ return NULL;
+ }
+
+ parent_def = virNodeDeviceObjGetDef(parent_dev);
+ for (parent_caps = parent_def->caps; parent_caps != NULL; parent_caps = parent_caps->next) {
+ if (parent_caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
+ virPCIDeviceAddress addr = {
+ .domain = parent_caps->data.pci_dev.domain,
+ .bus = parent_caps->data.pci_dev.bus,
+ .slot = parent_caps->data.pci_dev.slot,
+ .function = parent_caps->data.pci_dev.function
+ };
+
+ parent_pci = virPCIDeviceAddressAsString(&addr);
+ break;
+ }
+ }
+
+ virNodeDeviceObjEndAPI(&parent_dev);
+
+ if (parent_pci == NULL) {
+ virReportError(VIR_ERR_NO_NODE_DEVICE,
+ _("unable to find PCI address for parent device '%s'"), def->parent);
+ return NULL;
+ }
+
+ /* Convert node device definition to a JSON string formatted for
+ * mdevctl */
+ g_autofree char *json = NULL;
+ if (virNodeDeviceDefToMdevctlConfig(def, &json) < 0) {
+ virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
+ _("couldn't convert node device def to mdevctl JSON"));
+ return NULL;
+ }
+
+ g_autofree char *tmp = g_build_filename(driver->stateDir, "mdev-json-XXXXXX", NULL);
+ gint tmpfd;
+
+ if ((tmpfd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) {
+ virReportSystemError(errno, "%s", _("Failed to open temp file for write"));
+ return NULL;
+ }
+ if (safewrite(tmpfd, json, strlen(json)) != strlen(json) ||
+ VIR_CLOSE(tmpfd) < 0) {
+ virReportSystemError(errno, "%s", _("Failed to write temp file"));
+ return NULL;
+ }
+
+ if (virMdevctlStart(parent_pci, tmp, &uuid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to start mediated device"));
+ return NULL;
+ }
+
+ if (uuid && uuid[0] != '\0')
+ device = nodeDeviceFindNewMediatedDevice(conn, uuid);
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Unsupported device type"));
--
2.21.1
On 4/30/20 11:42 PM, Jonathon Jongsma wrote:
> With recent additions to the node device xml schema, an xml schema can
> now describe a mdev device sufficiently for libvirt to create and start
> the device using the mdevctl utility.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
> libvirt.spec.in | 3 +
> m4/virt-external-programs.m4 | 3 +
> src/conf/virnodedeviceobj.c | 34 +++++
> src/conf/virnodedeviceobj.h | 3 +
> src/libvirt_private.syms | 1 +
> src/node_device/node_device_driver.c | 177 +++++++++++++++++++++++++++
> 6 files changed, 221 insertions(+)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 6abf97df85..411846a9fc 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -466,6 +466,9 @@ Requires: dbus
> # For uid creation during pre
> Requires(pre): shadow-utils
>
> +# For managing persistent mediated devices
> +Recommends: mdevctl
> +
I wonder whether we could make this for nodedev driver RPM only.
Because, if somebody installs say client library + virsh only (for
remote mgmt), then there is no reason they should install mdevctl with that.
> %description daemon
> Server side daemon required to manage the virtualization capabilities
> of recent versions of Linux. Requires a hypervisor specific sub-RPM
> diff --git a/m4/virt-external-programs.m4 b/m4/virt-external-programs.m4
> index 9046e3bf07..bd3cb1f757 100644
> --- a/m4/virt-external-programs.m4
> +++ b/m4/virt-external-programs.m4
> @@ -65,6 +65,7 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [
> AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], [$LIBVIRT_SBIN_PATH])
> AC_PATH_PROG([SCRUB], [scrub], [scrub], [$LIBVIRT_SBIN_PATH])
> AC_PATH_PROG([ADDR2LINE], [addr2line], [addr2line], [$LIBVIRT_SBIN_PATH])
> + AC_PATH_PROG([MDEVCTL], [mdevctl], [mdevctl], [$LIBVIRT_SBIN_PATH])
>
> AC_DEFINE_UNQUOTED([DMIDECODE], ["$DMIDECODE"],
> [Location or name of the dmidecode program])
> @@ -88,6 +89,8 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [
> [Location or name of the scrub program (for wiping algorithms)])
> AC_DEFINE_UNQUOTED([ADDR2LINE], ["$ADDR2LINE"],
> [Location of addr2line program])
> + AC_DEFINE_UNQUOTED([MDEVCTL], ["$MDEVCTL"],
> + [Location or name of the mdevctl program])
>
> AC_PATH_PROG([IP_PATH], [ip], [/sbin/ip], [$LIBVIRT_SBIN_PATH])
> AC_DEFINE_UNQUOTED([IP_PATH], ["$IP_PATH"], [path to ip binary])
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index 3a34a324ca..fd20d5f9e2 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -399,6 +399,40 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
> &data);
> }
>
> +static int
> +virNodeDeviceObjListFindMediatedDeviceByUUIDCallback(const void *payload,
> + const void *name G_GNUC_UNUSED,
> + const void *opaque G_GNUC_UNUSED)
> +{
> + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> + const char *uuid = (const char *) opaque;
> + virNodeDevCapsDefPtr cap;
> + int want = 0;
> +
> + virObjectLock(obj);
> +
> + for (cap = obj->def->caps; cap != NULL; cap = cap->next) {
> + if (cap->data.type == VIR_NODE_DEV_CAP_MDEV) {
> + if (STREQ(cap->data.mdev.uuid, uuid)) {
> + want = 1;
> + break;
> + }
> + }
> + }
> +
> + virObjectUnlock(obj);
> + return want;
> +}
> +
> +
> +virNodeDeviceObjPtr
> +virNodeDeviceObjListFindMediatedDeviceByUUID(virNodeDeviceObjListPtr devs,
> + const char *uuid)
> +{
> + return virNodeDeviceObjListSearch(devs,
> + virNodeDeviceObjListFindMediatedDeviceByUUIDCallback,
> + uuid);
> +}
>
> static void
> virNodeDeviceObjListDispose(void *obj)
> diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
> index c9df8dedab..6efdb23d36 100644
> --- a/src/conf/virnodedeviceobj.h
> +++ b/src/conf/virnodedeviceobj.h
> @@ -118,3 +118,6 @@ virNodeDeviceObjListExport(virConnectPtr conn,
> void
> virNodeDeviceObjSetSkipUpdateCaps(virNodeDeviceObjPtr obj,
> bool skipUpdateCaps);
> +virNodeDeviceObjPtr
> +virNodeDeviceObjListFindMediatedDeviceByUUID(virNodeDeviceObjListPtr devs,
> + const char *uuid);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 94b206aa21..0468f68f52 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1178,6 +1178,7 @@ virNodeDeviceObjListAssignDef;
> virNodeDeviceObjListExport;
> virNodeDeviceObjListFindByName;
> virNodeDeviceObjListFindBySysfsPath;
> +virNodeDeviceObjListFindMediatedDeviceByUUID;
> virNodeDeviceObjListFindSCSIHostByWWNs;
> virNodeDeviceObjListFree;
> virNodeDeviceObjListGetNames;
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index f948dfbf69..c271f3bae5 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -30,6 +30,7 @@
> #include "datatypes.h"
> #include "viralloc.h"
> #include "virfile.h"
> +#include "virjson.h"
> #include "virstring.h"
> #include "node_device_conf.h"
> #include "node_device_event.h"
> @@ -40,6 +41,7 @@
> #include "viraccessapicheck.h"
> #include "virnetdev.h"
> #include "virutil.h"
> +#include "vircommand.h"
>
> #define VIR_FROM_THIS VIR_FROM_NODEDEV
>
> @@ -304,6 +306,30 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
> return device;
> }
>
> +static virNodeDevicePtr
> +nodeDeviceLookupMediatedDeviceByUUID(virConnectPtr conn,
> + const char *uuid,
> + unsigned int flags)
> +{
> + virNodeDeviceObjPtr obj = NULL;
> + virNodeDeviceDefPtr def;
> + virNodeDevicePtr device = NULL;
> +
> + virCheckFlags(0, NULL);
> +
> + if (!(obj = virNodeDeviceObjListFindMediatedDeviceByUUID(driver->devs,
> + uuid)))
> + return NULL;
> +
> + def = virNodeDeviceObjGetDef(obj);
> +
> + if ((device = virGetNodeDevice(conn, def->name)))
> + device->parentName = g_strdup(def->parent);
> +
> + virNodeDeviceObjEndAPI(&obj);
> + return device;
> +}
> +
>
> char *
> nodeDeviceGetXMLDesc(virNodeDevicePtr device,
> @@ -488,6 +514,23 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
> return device;
> }
>
> +static virNodeDevicePtr
> +nodeDeviceFindNewMediatedDeviceFunc(virConnectPtr conn,
> + const void *opaque)
> +{
> + const char *uuid = opaque;
> + return nodeDeviceLookupMediatedDeviceByUUID(conn, uuid, 0);
> +}
> +
> +static virNodeDevicePtr
> +nodeDeviceFindNewMediatedDevice(virConnectPtr conn,
> + const char *mdev_uuid)
> +{
> + return nodeDeviceFindNewDevice(conn,
> + nodeDeviceFindNewMediatedDeviceFunc,
> + mdev_uuid);
> +}
> +
> typedef struct _NewSCISHostFuncData
> {
> const char *wwnn;
> @@ -525,6 +568,68 @@ nodeDeviceHasCapability(virNodeDeviceDefPtr def, virNodeDevCapType type)
> return false;
> }
>
> +static int
> +virNodeDeviceDefToMdevctlConfig(virNodeDeviceDefPtr def, char **buf)
> +{
> + size_t i;
> + virNodeDevCapMdevPtr mdev = &def->caps->data.mdev;
> + g_autoptr(virJSONValue) json = virJSONValueNewObject();
> +
> + if (virJSONValueObjectAppendString(json, "mdev_type", mdev->type) < 0)
> + return -1;
> + if (virJSONValueObjectAppendString(json, "start", "manual") < 0)
> + return -1;
> + if (mdev->attributes) {
> + g_autoptr(virJSONValue) attributes = virJSONValueNewArray();
> + for (i = 0; i < mdev->nattributes; i++) {
> + virMediatedDeviceAttrPtr attr = mdev->attributes[i];
> + g_autoptr(virJSONValue) jsonattr = virJSONValueNewObject();
> +
> + if (virJSONValueObjectAppendString(jsonattr, attr->name, attr->value) < 0)
> + return -1;
> + if (virJSONValueArrayAppend(attributes, g_steal_pointer(&jsonattr)) < 0)
> + return -1;
> + }
> + if (virJSONValueObjectAppend(json, "attrs", g_steal_pointer(&attributes)) < 0)
> + return -1;
> + }
> +
> + *buf = virJSONValueToString(json, false);
> + if (*buf == NULL)
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> +virMdevctlStart(const char *parent, const char *json_file, char **uuid)
> +{
> + int status;
> + g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);
> + if (!mdevctl)
> + return -1;
> +
> + g_autoptr(virCommand) cmd = virCommandNewArgList(mdevctl,
> + "start",
> + "-p",
> + parent,
> + "--jsonfile",
> + json_file,
> + NULL);
> + virCommandAddEnvPassCommon(cmd);
> + virCommandSetOutputBuffer(cmd, uuid);
> +
> + /* an auto-generated uuid is returned via stdout if no uuid is specified in
> + * the mdevctl args */
> + if (virCommandRun(cmd, &status) < 0 || status != 0)
> + return -1;
> +
> + /* remove newline */
> + *uuid = g_strstrip(*uuid);
> +
> + return 0;
> +}
> +
> virNodeDevicePtr
> nodeDeviceCreateXML(virConnectPtr conn,
> const char *xmlDesc,
> @@ -569,6 +674,78 @@ nodeDeviceCreateXML(virConnectPtr conn,
> _("no node device for '%s' with matching "
> "wwnn '%s' and wwpn '%s'"),
> def->name, wwnn, wwpn);
> + } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
Can this whole block be moved into a separate function? I think it would
look slightly better.
> + virNodeDeviceObjPtr parent_dev = NULL;
> + virNodeDeviceDefPtr parent_def = NULL;
> + virNodeDevCapsDefPtr parent_caps = NULL;
> + g_autofree char *uuid = NULL;
> + g_autofree char *parent_pci = NULL;
> +
> + if (def->parent == NULL) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
> + _("cannot create a mediated device without a parent"));
> + return NULL;
> + }
> +
> + if ((parent_dev = virNodeDeviceObjListFindByName(driver->devs, def->parent)) == NULL) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE,
> + _("could not find parent device '%s'"), def->parent);
> + return NULL;
> + }
> +
> + parent_def = virNodeDeviceObjGetDef(parent_dev);
> + for (parent_caps = parent_def->caps; parent_caps != NULL; parent_caps = parent_caps->next) {
> + if (parent_caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
> + virPCIDeviceAddress addr = {
> + .domain = parent_caps->data.pci_dev.domain,
> + .bus = parent_caps->data.pci_dev.bus,
> + .slot = parent_caps->data.pci_dev.slot,
> + .function = parent_caps->data.pci_dev.function
> + };
> +
> + parent_pci = virPCIDeviceAddressAsString(&addr);
> + break;
> + }
> + }
> +
> + virNodeDeviceObjEndAPI(&parent_dev);
> +
> + if (parent_pci == NULL) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE,
> + _("unable to find PCI address for parent device '%s'"), def->parent);
> + return NULL;
> + }
> +
> + /* Convert node device definition to a JSON string formatted for
> + * mdevctl */
> + g_autofree char *json = NULL;
> + if (virNodeDeviceDefToMdevctlConfig(def, &json) < 0) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
> + _("couldn't convert node device def to mdevctl JSON"));
> + return NULL;
> + }
> +
> + g_autofree char *tmp = g_build_filename(driver->stateDir, "mdev-json-XXXXXX", NULL);
> + gint tmpfd;
These variables should be declared upfront. We don't really like
in-block declarations. Also, we have VIR_AUTOCLOSE ;-) Or even better,
virFileWriteStr(). I know, it's not obvious, unless you know src/util/
by heart.
> +
> + if ((tmpfd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) {
> + virReportSystemError(errno, "%s", _("Failed to open temp file for write"));
> + return NULL;
> + }
> + if (safewrite(tmpfd, json, strlen(json)) != strlen(json) ||
> + VIR_CLOSE(tmpfd) < 0) {
> + virReportSystemError(errno, "%s", _("Failed to write temp file"));
> + return NULL;
> + }
> +
> + if (virMdevctlStart(parent_pci, tmp, &uuid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to start mediated device")); > + return NULL;
> + }
> +
> + if (uuid && uuid[0] != '\0')
> + device = nodeDeviceFindNewMediatedDevice(conn, uuid);
> } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Unsupported device type"));
>
Michal
On Mon, May 11, 2020 at 03:27:56PM +0200, Michal Privoznik wrote: > On 4/30/20 11:42 PM, Jonathon Jongsma wrote: > > With recent additions to the node device xml schema, an xml schema can > > now describe a mdev device sufficiently for libvirt to create and start > > the device using the mdevctl utility. > > > > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> > > --- > > libvirt.spec.in | 3 + > > m4/virt-external-programs.m4 | 3 + > > src/conf/virnodedeviceobj.c | 34 +++++ > > src/conf/virnodedeviceobj.h | 3 + > > src/libvirt_private.syms | 1 + > > src/node_device/node_device_driver.c | 177 +++++++++++++++++++++++++++ > > 6 files changed, 221 insertions(+) > > > > diff --git a/libvirt.spec.in b/libvirt.spec.in > > index 6abf97df85..411846a9fc 100644 > > --- a/libvirt.spec.in > > +++ b/libvirt.spec.in > > @@ -466,6 +466,9 @@ Requires: dbus > > # For uid creation during pre > > Requires(pre): shadow-utils > > +# For managing persistent mediated devices > > +Recommends: mdevctl > > + > > I wonder whether we could make this for nodedev driver RPM only. Because, if > somebody installs say client library + virsh only (for remote mgmt), then > there is no reason they should install mdevctl with that. Yeah, it should be mandatory against the nodedev driver RPM. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Apr 30, 2020 at 04:42:57PM -0500, Jonathon Jongsma wrote:
> With recent additions to the node device xml schema, an xml schema can
> now describe a mdev device sufficiently for libvirt to create and start
> the device using the mdevctl utility.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
...
> }
>
> +static int
> +virNodeDeviceDefToMdevctlConfig(virNodeDeviceDefPtr def, char **buf)
> +{
> + size_t i;
> + virNodeDevCapMdevPtr mdev = &def->caps->data.mdev;
> + g_autoptr(virJSONValue) json = virJSONValueNewObject();
> +
> + if (virJSONValueObjectAppendString(json, "mdev_type", mdev->type) < 0)
> + return -1;
> + if (virJSONValueObjectAppendString(json, "start", "manual") < 0)
> + return -1;
> + if (mdev->attributes) {
> + g_autoptr(virJSONValue) attributes = virJSONValueNewArray();
> + for (i = 0; i < mdev->nattributes; i++) {
> + virMediatedDeviceAttrPtr attr = mdev->attributes[i];
> + g_autoptr(virJSONValue) jsonattr = virJSONValueNewObject();
> +
> + if (virJSONValueObjectAppendString(jsonattr, attr->name, attr->value) < 0)
> + return -1;
> + if (virJSONValueArrayAppend(attributes, g_steal_pointer(&jsonattr)) < 0)
> + return -1;
> + }
> + if (virJSONValueObjectAppend(json, "attrs", g_steal_pointer(&attributes)) < 0)
> + return -1;
> + }
> +
> + *buf = virJSONValueToString(json, false);
> + if (*buf == NULL)
tiny nitpick - we test pointers simply as if(!ptr) (multiple occurrences)
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> +virMdevctlStart(const char *parent, const char *json_file, char **uuid)
> +{
> + int status;
> + g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);
insert an empty line here...
> + if (!mdevctl)
> + return -1;
> +
> + g_autoptr(virCommand) cmd = virCommandNewArgList(mdevctl,
> + "start",
> + "-p",
> + parent,
> + "--jsonfile",
> + json_file,
> + NULL);
So, I was wondering what if someone actually wants to specify the UUID for the
device for some reason. We would have to expose a new element in the XML
partially duplicating what the <name> already contains. On the other hand,
I think we're good in create since these are truly supposed to be transient
mdevs, so it's desirable to let someone else (or us internally for that matter)
figure out the UUID for such device.
Now, this is irrelevant to this patch because I'm already thinking ahead.
In order to define an mdev, you will need to expose an element mapping to the
UUID, you want to let apps create mdev profiles with expected UUID values and
activate them on demand.
> + virCommandAddEnvPassCommon(cmd);
> + virCommandSetOutputBuffer(cmd, uuid);
> +
> + /* an auto-generated uuid is returned via stdout if no uuid is specified in
> + * the mdevctl args */
> + if (virCommandRun(cmd, &status) < 0 || status != 0)
> + return -1;
> +
> + /* remove newline */
> + *uuid = g_strstrip(*uuid);
> +
> + return 0;
> +}
> +
> virNodeDevicePtr
> nodeDeviceCreateXML(virConnectPtr conn,
> const char *xmlDesc,
> @@ -569,6 +674,78 @@ nodeDeviceCreateXML(virConnectPtr conn,
> _("no node device for '%s' with matching "
> "wwnn '%s' and wwpn '%s'"),
> def->name, wwnn, wwpn);
> + } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
> + virNodeDeviceObjPtr parent_dev = NULL;
> + virNodeDeviceDefPtr parent_def = NULL;
> + virNodeDevCapsDefPtr parent_caps = NULL;
> + g_autofree char *uuid = NULL;
> + g_autofree char *parent_pci = NULL;
> +
> + if (def->parent == NULL) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
This IMO should be a VIR_ERR_XML_ERROR because it's a mandatory element to be
specified.
> + _("cannot create a mediated device without a parent"));
> + return NULL;
> + }
> +
> + if ((parent_dev = virNodeDeviceObjListFindByName(driver->devs, def->parent)) == NULL) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE,
> + _("could not find parent device '%s'"), def->parent);
> + return NULL;
> + }
> +
> + parent_def = virNodeDeviceObjGetDef(parent_dev);
> + for (parent_caps = parent_def->caps; parent_caps != NULL; parent_caps = parent_caps->next) {
> + if (parent_caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
> + virPCIDeviceAddress addr = {
> + .domain = parent_caps->data.pci_dev.domain,
> + .bus = parent_caps->data.pci_dev.bus,
> + .slot = parent_caps->data.pci_dev.slot,
> + .function = parent_caps->data.pci_dev.function
> + };
> +
> + parent_pci = virPCIDeviceAddressAsString(&addr);
> + break;
> + }
> + }
> +
> + virNodeDeviceObjEndAPI(&parent_dev);
> +
> + if (parent_pci == NULL) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE,
> + _("unable to find PCI address for parent device '%s'"), def->parent);
> + return NULL;
> + }
> +
> + /* Convert node device definition to a JSON string formatted for
> + * mdevctl */
> + g_autofree char *json = NULL;
> + if (virNodeDeviceDefToMdevctlConfig(def, &json) < 0) {
> + virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
I think ^this should be an INTERNAL_ERROR instead
> + _("couldn't convert node device def to mdevctl JSON"));
> + return NULL;
> + }
> +
> + g_autofree char *tmp = g_build_filename(driver->stateDir, "mdev-json-XXXXXX", NULL);
> + gint tmpfd;
> +
> + if ((tmpfd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) {
> + virReportSystemError(errno, "%s", _("Failed to open temp file for write"));
> + return NULL;
> + }
> + if (safewrite(tmpfd, json, strlen(json)) != strlen(json) ||
> + VIR_CLOSE(tmpfd) < 0) {
> + virReportSystemError(errno, "%s", _("Failed to write temp file"));
> + return NULL;
You should discard the temporary JSON file at some point once the below is
finished because it's littering the state dir.
> + }
> +
> + if (virMdevctlStart(parent_pci, tmp, &uuid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to start mediated device"));
> + return NULL;
> + }
> +
> + if (uuid && uuid[0] != '\0')
Is ^this check necessary? I mean, mdevctl either returns the UUID and retcode 0
or nothing and retcode 1 which we should have already checked in virCommandRun,
so since it was successful, we now know that UUID is filled in or did I miss
something in the code?
> + device = nodeDeviceFindNewMediatedDevice(conn, uuid);
> } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Unsupported device type"));
--
Erik Skultety
© 2016 - 2026 Red Hat, Inc.