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@xxxxxxxxxxxxx>
> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
> ---
> 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
>
