Re: [PATCH v2 2/2] node_device: detecting mdev_types capability on CSS devices

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(a)linux.ibm.com&gt; > Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com&gt; > --- > 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 >