On 11/11/20 10:34 AM, Erik Skultety wrote:

On Tue, Nov 10, 2020 at 07:09:05PM +0100, Boris Fiuczynski wrote:

...

To prevent copying the mdev_types description multiple times it is refactored into a new paragraph for easy reuse.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/formatnode.html.in | 70 ++++++++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 6928bdd69c..bd3112c5a8 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -159,35 +159,10 @@ </dd> <dt><code><a id="MDEVCap">mdev_types</a></code></dt> <dd> - This device is capable of creating mediated devices, and - the capability will contain a list of <code>type</code> - elements, which list all mdev types supported on the - physical device. <span class="since">Since 3.4.0</span> - Each <code>type</code> element has a single <code>id</code> - attribute that holds an official vendor-supplied identifier - for the type. It supports the following sub-elements: - <dl> - <dt><code>name</code></dt> - <dd> - The <code>name</code> element holds a vendor-supplied - code name for the given mediated device type. This is - an optional element. - </dd> - <dt><code>deviceAPI</code></dt> - <dd> - The value of this element describes how an instance of - the given type will be presented to the guest by the - VFIO framework. - </dd> - <dt><code>availableInstances</code></dt> - <dd> - This element reports the current state of resource - allocation. In other words, how many instances of the - given type can still be successfully created on the - physical device. - </dd> - </dl> - </dd> + This device is capable of creating mediated devices. + The sub-elements are summarized in + <a href="#MDevTypesCap">mdev_types capability</a>. + </dd> </dl> </dd>

@@ -445,6 +420,43 @@ </dd> </dl>

+ <h3><a id="MDevTypesCap">mdev_types capability</a></h3> + + <p> + <a href="#MDEVCap">PCI</a> devices can be capable of + creating mediated devices. + If they are capable the attribute <code>type</code> of the + element <code>capability</code> is <code>mdev_types</code>.

I'd slightly adjust the wording because the first I read it I got confused, not just the rewrite, also by the original (probably mine) text. How about:

"If the indeed are capable, then the parent <code>capability</code> element

If they indeed...

for <code>mdev_types</code> type will contain a list of ...."

Reviewed-by: Erik Skultety <eskultet@redhat.com>

Yes, I agree that it is an easier read.

...

+ This capability will contain a list of <code>type</code> + elements, which list all mdev types supported on the + physical device. <span class="since">Since 3.4.0</span> + Each <code>type</code> element has a single <code>id</code> + attribute that holds an official vendor-supplied identifier + for the type. It supports the following sub-elements: + <dl> + <dt><code>name</code></dt> + <dd> + The <code>name</code> element holds a vendor-supplied + code name for the given mediated device type. This is + an optional element. + </dd> + <dt><code>deviceAPI</code></dt> + <dd> + The value of this element describes how an instance of + the given type will be presented to the guest by the + VFIO framework. + </dd> + <dt><code>availableInstances</code></dt> + <dd> + This element reports the current state of resource + allocation. In other words, how many instances of the + given type can still be successfully created on the + physical device. + </dd> + </dl> + </p> + + <h2><a id="nodeExample">Examples</a></h2>

<p>The following are some example node device XML outputs:</p> -- 2.26.2

-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Tue, Nov 10, 2020 at 07:09:06PM +0100, Boris Fiuczynski wrote:

Add detection of mdev_types capability to channel subsystem devices.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- docs/drvnodedev.html.in | 5 +- docs/formatnode.html.in | 19 +++- docs/schemas/nodedev.rng | 4 + src/conf/node_device_conf.c | 92 ++++++++++++++++++- src/conf/node_device_conf.h | 11 +++ src/conf/virnodedeviceobj.c | 7 +- src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 3 + .../css_0_0_fffe_mdev_types.xml | 17 ++++ tests/nodedevxml2xmltest.c | 1 + 10 files changed, 153 insertions(+), 7 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_fffe_mdev_types.xml

diff --git a/docs/drvnodedev.html.in b/docs/drvnodedev.html.in index 0823c1888d..d5191d6d93 100644 --- a/docs/drvnodedev.html.in +++ b/docs/drvnodedev.html.in @@ -139,12 +139,13 @@

<h3><a id="MDEVCap">MDEV capability</a></h3> <p> - A PCI device capable of creating mediated devices will include a nested + A device capable of creating mediated devices will include a nested capability <code>mdev_types</code> which enumerates all supported mdev types on the physical device, along with the type attributes available through sysfs. A detailed description of the XML format for the <code>mdev_types</code> capability can be found - <a href="formatnode.html#MDEVCap">here</a>. + <a href="formatnode.html#MDEVCap">here for PCI</a> or + <a href="formatnode.html#MDEVCapCSS">here for CSS</a>.

Both PCI and CSS mdev_types are just a pointer to the mdev_types XML element. How about just fixing the href to MDEVTypesCap?

</p> <p> The following example shows how we might represent an NVIDIA GPU device diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index bd3112c5a8..d9abba0efa 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -405,6 +405,21 @@ <dd>The subchannel-set identifier.</dd> <dt><code>devno</code></dt> <dd>The device number.</dd> + <dt><code>capability</code></dt> + <dd> + This optional element can occur multiple times. If it + exists, it has a mandatory <code>type</code> attribute + which will be set to: + <dl> + <dt><code><a id="MDEVCapCSS">mdev_types</a></code></dt> + <dd> + <span class="since">Since 6.10.0</span> + This device is capable of creating mediated devices. + The sub-elements are summarized in + <a href="#MDevTypesCap">mdev_types capability</a>.

I think we should stay consistent and make this "MDEVTypesCap"...

+ </dd> + </dl> + </dd> </dl> </dd> <dt><code>vdpa</code></dt> @@ -423,8 +438,8 @@ <h3><a id="MDevTypesCap">mdev_types capability</a></h3>

<p> - <a href="#MDEVCap">PCI</a> devices can be capable of - creating mediated devices. + <a href="#MDEVCap">PCI</a> and <a href="#MDEVCapCSS">CSS</a>

Here I'm wondering why we've closed the circle and point to a generic text which will point us back here, shouldn't we point to the whole PCI/CSS definitions instead? These are just stylistic nitpicks, let me know your opinion whether you agree with the proposed ideas and I'll do the changes and merge. Erik

+ devices can be capable of creating mediated devices. If they are capable the attribute <code>type</code> of the element <code>capability</code> is <code>mdev_types</code>. This capability will contain a list of <code>type</code> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 231afa0218..b3e986659e 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -654,6 +654,9 @@ <element name="devno"> <ref name="ccwDevnoRange"/> </element> + <optional> + <ref name="mdev_types"/> + </optional> </define>

<define name="capvdpa"> @@ -702,6 +705,7 @@ <element name="deviceAPI"> <choice> <value>vfio-pci</value> + <value>vfio-ccw</value> </choice> </element> <element name="availableInstances"> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a57505a27e..4e2837c1cd 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -552,6 +552,10 @@ virNodeDeviceCapCCWDefFormat(virBufferPtr buf, data->ccw_dev.ssid); virBufferAsprintf(buf, "<devno>0x%04x</devno>\n", data->ccw_dev.devno); + if (data->ccw_dev.flags & VIR_NODE_DEV_CAP_FLAG_CSS_MDEV) + virNodeDeviceCapMdevTypesFormat(buf, + data->ccw_dev.mdev_types, + data->ccw_dev.nmdev_types); }

@@ -843,6 +847,33 @@ virNodeDevCapMdevTypesParseXML(xmlXPathContextPtr ctxt, }

+static int +virNodeDevCSSCapabilityParseXML(xmlXPathContextPtr ctxt, + xmlNodePtr node, + virNodeDevCapCCWPtr ccw_dev) +{ + g_autofree char *type = virXMLPropString(node, "type"); + VIR_XPATH_NODE_AUTORESTORE(ctxt) + + ctxt->node = node; + + if (!type) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing capability type")); + return -1; + } + + if (STREQ(type, "mdev_types")) { + if (virNodeDevCapMdevTypesParseXML(ctxt, + &ccw_dev->mdev_types, + &ccw_dev->nmdev_types) < 0) + return -1; + ccw_dev->flags |= VIR_NODE_DEV_CAP_FLAG_CSS_MDEV; + } + + return 0; +} + + static int virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt, virNodeDeviceDefPtr def, @@ -850,6 +881,9 @@ virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt, virNodeDevCapCCWPtr ccw_dev) { VIR_XPATH_NODE_AUTORESTORE(ctxt) + g_autofree xmlNodePtr *nodes = NULL; + int n = 0; + size_t i = 0; g_autofree char *cssid = NULL; g_autofree char *ssid = NULL; g_autofree char *devno = NULL; @@ -895,6 +929,14 @@ virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt, return -1; }

+ if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) + return -1; + + for (i = 0; i < n; i++) { + if (virNodeDevCSSCapabilityParseXML(ctxt, nodes[i], ccw_dev) < 0) + return -1; + } + return 0; }

@@ -2253,12 +2295,16 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) virMediatedDeviceAttrFree(data->mdev.attributes[i]); VIR_FREE(data->mdev.attributes); break; + case VIR_NODE_DEV_CAP_CSS_DEV: + for (i = 0; i < data->ccw_dev.nmdev_types; i++) + virMediatedDeviceTypeFree(data->ccw_dev.mdev_types[i]); + VIR_FREE(data->ccw_dev.mdev_types); + break; case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_DRM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_CCW_DEV: - case VIR_NODE_DEV_CAP_CSS_DEV: case VIR_NODE_DEV_CAP_VDPA: case VIR_NODE_DEV_CAP_LAST: /* This case is here to shutup the compiler */ @@ -2297,6 +2343,11 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def) &cap->data.pci_dev) < 0) return -1; break; + case VIR_NODE_DEV_CAP_CSS_DEV: + if (virNodeDeviceGetCSSDynamicCaps(def->sysfs_path, + &cap->data.ccw_dev) < 0) + return -1; + break;

/* all types that (supposedly) don't require any updates * relative to what's in the cache. @@ -2313,7 +2364,6 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def) case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_CCW_DEV: - case VIR_NODE_DEV_CAP_CSS_DEV: case VIR_NODE_DEV_CAP_VDPA: case VIR_NODE_DEV_CAP_LAST: break; @@ -2388,6 +2438,15 @@ virNodeDeviceCapsListExport(virNodeDeviceDefPtr def, ncaps++; } } + + if (caps->data.type == VIR_NODE_DEV_CAP_CSS_DEV) { + flags = caps->data.ccw_dev.flags; + + if (flags & VIR_NODE_DEV_CAP_FLAG_CSS_MDEV) { + MAYBE_ADD_CAP(VIR_NODE_DEV_CAP_MDEV_TYPES); + ncaps++; + } + } }

#undef MAYBE_ADD_CAP @@ -2653,6 +2712,28 @@ virNodeDeviceGetPCIDynamicCaps(const char *sysfsPath, return 0; }

+ +/* virNodeDeviceGetCSSDynamicCaps() get info that is stored in sysfs + * about devices related to this device, i.e. things that can change + * without this device itself changing. These must be refreshed + * anytime full XML of the device is requested, because they can + * change with no corresponding notification from the kernel/udev. + */ +int +virNodeDeviceGetCSSDynamicCaps(const char *sysfsPath, + virNodeDevCapCCWPtr ccw_dev) +{ + ccw_dev->flags &= ~VIR_NODE_DEV_CAP_FLAG_CSS_MDEV; + if (virNodeDeviceGetMdevTypesCaps(sysfsPath, + &ccw_dev->mdev_types, + &ccw_dev->nmdev_types) < 0) + return -1; + if (ccw_dev->nmdev_types > 0) + ccw_dev->flags |= VIR_NODE_DEV_CAP_FLAG_CSS_MDEV; + + return 0; +} + #else

int @@ -2675,4 +2756,11 @@ int virNodeDeviceGetSCSITargetCaps(const char *sysfsPath G_GNUC_UNUSED, return -1; }

+int +virNodeDeviceGetCSSDynamicCaps(const char *sysfsPath G_GNUC_UNUSED, + virNodeDevCapCCWPtr ccw_dev G_GNUC_UNUSED) +{ + return -1; +} + #endif /* __linux__ */ diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 3057c728a0..262524fc87 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -102,6 +102,10 @@ typedef enum { VIR_NODE_DEV_CAP_FLAG_PCI_MDEV = (1 << 3), } virNodeDevPCICapFlags;

+typedef enum { + VIR_NODE_DEV_CAP_FLAG_CSS_MDEV = (1 << 0), +} virNodeDevCCWCapFlags; + typedef enum { /* Keep in sync with VIR_ENUM_IMPL in node_device_conf.c */ VIR_NODE_DEV_DRM_PRIMARY, @@ -274,6 +278,9 @@ struct _virNodeDevCapCCW { unsigned int cssid; unsigned int ssid; unsigned int devno; + unsigned int flags; /* enum virNodeDevCCWCapFlags */ + virMediatedDeviceTypePtr *mdev_types; + size_t nmdev_types; };

typedef struct _virNodeDevCapVDPA virNodeDevCapVDPA; @@ -391,6 +398,10 @@ int virNodeDeviceGetPCIDynamicCaps(const char *sysfsPath, virNodeDevCapPCIDevPtr pci_dev);

+int +virNodeDeviceGetCSSDynamicCaps(const char *sysfsPath, + virNodeDevCapCCWPtr ccw_dev); + int virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def);

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 037a938e88..9e46d22a2f 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -696,6 +696,12 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, return true; break;

+ case VIR_NODE_DEV_CAP_CSS_DEV: + if (type == VIR_NODE_DEV_CAP_MDEV_TYPES && + (cap->data.ccw_dev.flags & VIR_NODE_DEV_CAP_FLAG_CSS_MDEV)) + return true; + break; + case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_USB_DEV: case VIR_NODE_DEV_CAP_USB_INTERFACE: @@ -710,7 +716,6 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_CCW_DEV: - case VIR_NODE_DEV_CAP_CSS_DEV: case VIR_NODE_DEV_CAP_VDPA: case VIR_NODE_DEV_CAP_LAST: break; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ddddc8f2d1..491bd5bd87 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -824,6 +824,7 @@ virNodeDeviceDefFree; virNodeDeviceDefParseFile; virNodeDeviceDefParseNode; virNodeDeviceDefParseString; +virNodeDeviceGetCSSDynamicCaps; virNodeDeviceGetPCIDynamicCaps; virNodeDeviceGetSCSIHostCaps; virNodeDeviceGetSCSITargetCaps; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b315894965..65f312d8f4 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1139,6 +1139,9 @@ udevProcessCSS(struct udev_device *device, if (udevGenerateDeviceName(device, def, NULL) != 0) return -1;

+ if (virNodeDeviceGetCSSDynamicCaps(def->sysfs_path, &def->caps->data.ccw_dev) < 0) + return -1; + return 0; }

diff --git a/tests/nodedevschemadata/css_0_0_fffe_mdev_types.xml b/tests/nodedevschemadata/css_0_0_fffe_mdev_types.xml new file mode 100644 index 0000000000..5058b6434e --- /dev/null +++ b/tests/nodedevschemadata/css_0_0_fffe_mdev_types.xml @@ -0,0 +1,17 @@ +<device> + <name>css_0_0_fffe</name> + <path>/sys/devices/css0/0.0.fffe</path> + <parent>computer</parent> + <capability type='css'> + <cssid>0x0</cssid> + <ssid>0x0</ssid> + <devno>0xfffe</devno> + <capability type='mdev_types'> + <type id='vfio_ccw-io'> + <name>I/O subchannel (Non-QDIO)</name> + <deviceAPI>vfio-ccw</deviceAPI> + <availableInstances>1</availableInstances> + </type> + </capability> + </capability> +</device> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index 3cb23b1df4..a009ecb343 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -124,6 +124,7 @@ mymain(void) DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b"); DO_TEST("ccw_0_0_ffff"); DO_TEST("css_0_0_ffff"); + DO_TEST("css_0_0_fffe_mdev_types");

return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.26.2

On Wed, Nov 11, 2020 at 11:15:33AM +0100, Boris Fiuczynski wrote:

On 11/11/20 10:39 AM, Erik Skultety wrote:

...

On Tue, Nov 10, 2020 at 07:09:06PM +0100, Boris Fiuczynski wrote:

...

Add detection of mdev_types capability to channel subsystem devices.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- docs/drvnodedev.html.in | 5 +- docs/formatnode.html.in | 19 +++- docs/schemas/nodedev.rng | 4 + src/conf/node_device_conf.c | 92 ++++++++++++++++++- src/conf/node_device_conf.h | 11 +++ src/conf/virnodedeviceobj.c | 7 +- src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 3 + .../css_0_0_fffe_mdev_types.xml | 17 ++++ tests/nodedevxml2xmltest.c | 1 + 10 files changed, 153 insertions(+), 7 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_fffe_mdev_types.xml

diff --git a/docs/drvnodedev.html.in b/docs/drvnodedev.html.in index 0823c1888d..d5191d6d93 100644 --- a/docs/drvnodedev.html.in +++ b/docs/drvnodedev.html.in @@ -139,12 +139,13 @@ <h3><a id="MDEVCap">MDEV capability</a></h3> <p> - A PCI device capable of creating mediated devices will include a nested + A device capable of creating mediated devices will include a nested capability <code>mdev_types</code> which enumerates all supported mdev types on the physical device, along with the type attributes available through sysfs. A detailed description of the XML format for the <code>mdev_types</code> capability can be found - <a href="formatnode.html#MDEVCap">here</a>. + <a href="formatnode.html#MDEVCap">here for PCI</a> or + <a href="formatnode.html#MDEVCapCSS">here for CSS</a>.

Both PCI and CSS mdev_types are just a pointer to the mdev_types XML element. How about just fixing the href to MDEVTypesCap?

That is fine since from there one gets the list of devices as well. But than let us cleanly rename the ids in docs/formatnode.html.in as follows:

MDEVCap => MDEVTypesCapPCI MDEVCapCSS => MDEVTypesCapCSS MDevTypesCap => MDEVTypesCap

Okay, makes sense, but that should come to a trivial standalone patch. I'll let you quickly respin with that along with the other nits, I'll approve and you can then apply what's necessary on Shalini's series :). Erik