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 | 145 ++++++++++++++++++
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, 301 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 6143459618..fc5ac1d27e 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -853,6 +853,151 @@ virMdevctlStop(virNodeDeviceDefPtr def)
}
+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, 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",
+ _("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",
+ _("Child list 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) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to append child device to list"));
+ 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 02baf56dab..2a162513c0 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -119,6 +119,10 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
virCommandPtr
nodeDeviceGetMdevctlStopCommand(const char *uuid);
+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 1d3cb00400..93f4e3252c 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -143,6 +143,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)
{
@@ -263,13 +310,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)
@@ -279,6 +326,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");
@@ -287,6 +337,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
On Wed, Feb 03, 2021 at 11:38:49AM -0600, Jonathon Jongsma wrote:
> 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 | 145 ++++++++++++++++++
> 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, 301 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 6143459618..fc5ac1d27e 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -853,6 +853,151 @@ virMdevctlStop(virNodeDeviceDefPtr def)
> }
>
>
> +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, attrs;
^This was the other "One declaration per line" place I could not remember
during v3 review.
...
> +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",
> + _("JSON response contains no devices"));
I'd make it even more specific - "mdevctl JSON response..."
> + 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",
> + _("Child list is not an array"));
"Parent device's JSON object data is not an array" - or something along those
lines
> + 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"));
"Unable to parse mdev child device"
> + goto error;
> + }
> +
> + if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to append child device to list"));
APPEND_ELEMENT will either report an "Out of bounds" error or abort on
allocation failure, so please drop ^this virReportError.
> + 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 02baf56dab..2a162513c0 100644
> --- a/src/node_device/node_device_driver.h
> +++ b/src/node_device/node_device_driver.h
> @@ -119,6 +119,10 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> virCommandPtr
> nodeDeviceGetMdevctlStopCommand(const char *uuid);
>
> +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"
> + },
I'd still like to know why there are 2 assign_adapter and 2 assign_domain
attributes here, I'm simply confused what the outcome should be.
> + {
> + "assign_domain": "0xab"
> + },
> + {
> + "assign_control_domain": "0xab"
> + },
> + {
> + "assign_domain": "4"
> + },
> + {
> + "assign_control_domain": "4"
> + }
> + ]
> + }
> + }
> + ]
> + }
> +]
> +
...
> + <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'/>
Here too I'd like to hear an opinion (since v3) on naming the attributes in
such way that they correspond to the respective elements: ap-adapter,
ap-domain, etc. This naming is very unintuitive if not documented properly;
unless there's an internal reason why they have to be named "assign_control",
etc.
Erik
On Mon, 15 Feb 2021 18:22:08 +0100
Erik Skultety <eskultet@redhat.com> wrote:
> On Wed, Feb 03, 2021 at 11:38:49AM -0600, Jonathon Jongsma wrote:
> > 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"
> > + },
>
> I'd still like to know why there are 2 assign_adapter and 2
> assign_domain attributes here, I'm simply confused what the outcome
> should be.
As far as I can recall, i was just trying to use some real-world-ish
mdevctl output to test the parsing and handling of mdev attributes. In this
case, I believe that I simply copied the example output from the mdevctl
documentation. See:
https://github.com/mdevctl/mdevctl#advanced-usage-attributes-and-json
>
> > + {
> > + "assign_domain": "0xab"
> > + },
> > + {
> > + "assign_control_domain": "0xab"
> > + },
> > + {
> > + "assign_domain": "4"
> > + },
> > + {
> > + "assign_control_domain": "4"
> > + }
> > + ]
> > + }
> > + }
> > + ]
> > + }
> > +]
> > +
>
>
> ...
>
>
> > + <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'/>
>
> Here too I'd like to hear an opinion (since v3) on naming the
> attributes in such way that they correspond to the respective
> elements: ap-adapter, ap-domain, etc. This naming is very
> unintuitive if not documented properly; unless there's an internal
> reason why they have to be named "assign_control", etc.
These are the names of the attributes that are used to configure these
devices in sysfs:
https://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.ljdd/ljdd_c_vfio_bind_ap.html
The idea here is that <attr> is a direct mapping to and from the mdev sysfs
attributes, since that is how these devices are configured. An
attribute name means nothing to libvirt, it is just an opaque name that
we pass to mdevctl. If we were to deviate from this strict mapping and
add "friendlier" names in libvirt, we would need to maintain a custom
per-device mapping from mdev sysfs attribute name to libvirt
friendly-name. That seems unmaintainable.
Thanks,
Jonathon
On Tue, 16 Feb 2021 16:00:40 -0600
Jonathon Jongsma <jjongsma@redhat.com> wrote:
> On Mon, 15 Feb 2021 18:22:08 +0100
> Erik Skultety <eskultet@redhat.com> wrote:
>
> > On Wed, Feb 03, 2021 at 11:38:49AM -0600, Jonathon Jongsma wrote:
> > > 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"
> > > + },
> >
> > I'd still like to know why there are 2 assign_adapter and 2
> > assign_domain attributes here, I'm simply confused what the outcome
> > should be.
>
> As far as I can recall, i was just trying to use some real-world-ish
> mdevctl output to test the parsing and handling of mdev attributes. In this
> case, I believe that I simply copied the example output from the mdevctl
> documentation. See:
>
> https://github.com/mdevctl/mdevctl#advanced-usage-attributes-and-json
>
>
> >
> > > + {
> > > + "assign_domain": "0xab"
> > > + },
> > > + {
> > > + "assign_control_domain": "0xab"
> > > + },
> > > + {
> > > + "assign_domain": "4"
> > > + },
> > > + {
> > > + "assign_control_domain": "4"
> > > + }
> > > + ]
> > > + }
> > > + }
> > > + ]
> > > + }
> > > +]
> > > +
> >
> >
> > ...
> >
> >
> > > + <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'/>
> >
> > Here too I'd like to hear an opinion (since v3) on naming the
> > attributes in such way that they correspond to the respective
> > elements: ap-adapter, ap-domain, etc. This naming is very
> > unintuitive if not documented properly; unless there's an internal
> > reason why they have to be named "assign_control", etc.
>
> These are the names of the attributes that are used to configure these
> devices in sysfs:
> https://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.ljdd/ljdd_c_vfio_bind_ap.html
>
> The idea here is that <attr> is a direct mapping to and from the mdev sysfs
> attributes, since that is how these devices are configured. An
> attribute name means nothing to libvirt, it is just an opaque name that
> we pass to mdevctl. If we were to deviate from this strict mapping and
> add "friendlier" names in libvirt, we would need to maintain a custom
> per-device mapping from mdev sysfs attribute name to libvirt
> friendly-name. That seems unmaintainable.
Yep, and the names don't mean anything to mdevctl either, as you say
they're just sysfs attributes, mdevctl looks for the named attribute
and writes the value to it. Note that ordering of the attributes might
be important, which is why mdevctl stores them in an array and provides
some utility to re-index attributes. I can't tell if the above example
necessarily imposes that ordering from the xml. Thanks,
Alex
On Tue, 16 Feb 2021 15:20:28 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Tue, 16 Feb 2021 16:00:40 -0600
> Jonathon Jongsma <jjongsma@redhat.com> wrote:
>
> > On Mon, 15 Feb 2021 18:22:08 +0100
> > Erik Skultety <eskultet@redhat.com> wrote:
> >
> > > On Wed, Feb 03, 2021 at 11:38:49AM -0600, Jonathon Jongsma wrote:
> > >
> > > > 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"
> > > > + },
> > >
> > > I'd still like to know why there are 2 assign_adapter and 2
> > > assign_domain attributes here, I'm simply confused what the
> > > outcome should be.
> >
> > As far as I can recall, i was just trying to use some real-world-ish
> > mdevctl output to test the parsing and handling of mdev attributes.
> > In this case, I believe that I simply copied the example output
> > from the mdevctl documentation. See:
> >
> > https://github.com/mdevctl/mdevctl#advanced-usage-attributes-and-json
> >
> >
> > >
> > > > + {
> > > > + "assign_domain": "0xab"
> > > > + },
> > > > + {
> > > > + "assign_control_domain": "0xab"
> > > > + },
> > > > + {
> > > > + "assign_domain": "4"
> > > > + },
> > > > + {
> > > > + "assign_control_domain": "4"
> > > > + }
> > > > + ]
> > > > + }
> > > > + }
> > > > + ]
> > > > + }
> > > > +]
> > > > +
> > >
> > >
> > > ...
> > >
> > >
> > > > + <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'/>
> > >
> > > Here too I'd like to hear an opinion (since v3) on naming the
> > > attributes in such way that they correspond to the respective
> > > elements: ap-adapter, ap-domain, etc. This naming is very
> > > unintuitive if not documented properly; unless there's an internal
> > > reason why they have to be named "assign_control", etc.
> >
> > These are the names of the attributes that are used to configure
> > these devices in sysfs:
> > https://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.ljdd/ljdd_c_vfio_bind_ap.html
> >
> > The idea here is that <attr> is a direct mapping to and from the
> > mdev sysfs attributes, since that is how these devices are
> > configured. An attribute name means nothing to libvirt, it is just
> > an opaque name that we pass to mdevctl. If we were to deviate from
> > this strict mapping and add "friendlier" names in libvirt, we would
> > need to maintain a custom per-device mapping from mdev sysfs
> > attribute name to libvirt friendly-name. That seems unmaintainable.
> >
>
> Yep, and the names don't mean anything to mdevctl either, as you say
> they're just sysfs attributes, mdevctl looks for the named attribute
> and writes the value to it. Note that ordering of the attributes
> might be important, which is why mdevctl stores them in an array and
> provides some utility to re-index attributes. I can't tell if the
> above example necessarily imposes that ordering from the xml. Thanks,
>
Yes, the order of xml attributes is significant, they should be passed
to mdevctl in the same order as they are defined in the xml. Though I
don't think I've documented that anywhere...
Jonathon
On Wed, Feb 17, 2021 at 08:55:12AM -0600, Jonathon Jongsma wrote:
> On Tue, 16 Feb 2021 15:20:28 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
>
> > On Tue, 16 Feb 2021 16:00:40 -0600
> > Jonathon Jongsma <jjongsma@redhat.com> wrote:
> >
> > > On Mon, 15 Feb 2021 18:22:08 +0100
> > > Erik Skultety <eskultet@redhat.com> wrote:
> > >
> > > > On Wed, Feb 03, 2021 at 11:38:49AM -0600, Jonathon Jongsma wrote:
> > > >
> > > > > 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"
> > > > > + },
> > > >
> > > > I'd still like to know why there are 2 assign_adapter and 2
> > > > assign_domain attributes here, I'm simply confused what the
> > > > outcome should be.
> > >
> > > As far as I can recall, i was just trying to use some real-world-ish
> > > mdevctl output to test the parsing and handling of mdev attributes.
> > > In this case, I believe that I simply copied the example output
> > > from the mdevctl documentation. See:
> > >
> > > https://github.com/mdevctl/mdevctl#advanced-usage-attributes-and-json
> > >
> > >
> > > >
> > > > > + {
> > > > > + "assign_domain": "0xab"
> > > > > + },
> > > > > + {
> > > > > + "assign_control_domain": "0xab"
> > > > > + },
> > > > > + {
> > > > > + "assign_domain": "4"
> > > > > + },
> > > > > + {
> > > > > + "assign_control_domain": "4"
> > > > > + }
> > > > > + ]
> > > > > + }
> > > > > + }
> > > > > + ]
> > > > > + }
> > > > > +]
> > > > > +
> > > >
> > > >
> > > > ...
> > > >
> > > >
> > > > > + <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'/>
> > > >
> > > > Here too I'd like to hear an opinion (since v3) on naming the
> > > > attributes in such way that they correspond to the respective
> > > > elements: ap-adapter, ap-domain, etc. This naming is very
> > > > unintuitive if not documented properly; unless there's an internal
> > > > reason why they have to be named "assign_control", etc.
> > >
> > > These are the names of the attributes that are used to configure
> > > these devices in sysfs:
> > > https://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.ljdd/ljdd_c_vfio_bind_ap.html
> > >
> > > The idea here is that <attr> is a direct mapping to and from the
> > > mdev sysfs attributes, since that is how these devices are
> > > configured. An attribute name means nothing to libvirt, it is just
> > > an opaque name that we pass to mdevctl. If we were to deviate from
> > > this strict mapping and add "friendlier" names in libvirt, we would
> > > need to maintain a custom per-device mapping from mdev sysfs
> > > attribute name to libvirt friendly-name. That seems unmaintainable.
> > >
> >
> > Yep, and the names don't mean anything to mdevctl either, as you say
> > they're just sysfs attributes, mdevctl looks for the named attribute
> > and writes the value to it. Note that ordering of the attributes
> > might be important, which is why mdevctl stores them in an array and
> > provides some utility to re-index attributes. I can't tell if the
> > above example necessarily imposes that ordering from the xml. Thanks,
> >
>
> Yes, the order of xml attributes is significant, they should be passed
> to mdevctl in the same order as they are defined in the xml. Though I
> don't think I've documented that anywhere...
Okay, those are fair arguments, so we need to document the usage in libvirt
properly.
Erik
© 2016 - 2026 Red Hat, Inc.