[PATCH v2 2/2] node_device: detecting mdev_types capability on CSS devices
Boris Fiuczynski
fiuczy at linux.ibm.com
Wed Nov 11 10:15:33 UTC 2020
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 at linux.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk at 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
>
>> </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"...
Agreed, see above.
>
>> + </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?
This list makes sense if someone arives here coming from
docs/drvnodedev.html.in and does not know which devices provides
mdev_types. There is another series in the mailing list [
https://www.redhat.com/archives/libvir-list/2020-October/msg01191.html ]
adding ap_matrix to the list as well. I have the mdev_types support
patch ready for this as well but the series needs to get accepted first.
Shalini is currently working on a v2 and could also append my patch to
her series if we are quick enough. :-)
>
> 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.
Thanks Erik.
>
> 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
>>
>
--
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
More information about the libvir-list
mailing list