From eskultet at redhat.com Wed Nov 11 04:39:44 2020
Content-Type: multipart/mixed; boundary="===============8546553232324824493=="
MIME-Version: 1.0
From: Erik Skultety <eskultet at redhat.com>
To: devel at lists.libvirt.org
Subject: Re: [PATCH v2 2/2] node_device: detecting mdev_types capability on
 CSS devices
Date: Wed, 11 Nov 2020 10:39:36 +0100
Message-ID: <20201111093936.GB857537@nautilus>
In-Reply-To: 20201110180906.38759-3-fiuczy@linux.ibm.com

--===============8546553232324824493==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

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>
> Reviewed-by: Bjoern Walk <bwalk(a)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=3D"MDEVCap">MDEV capability</a></h3>
>      <p>
> -      A PCI device capable of creating mediated devices will include a n=
ested
> +      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 avail=
able
>        through sysfs. A detailed description of the XML format for the
>        <code>mdev_types</code> capability can be found
> -      <a href=3D"formatnode.html#MDEVCap">here</a>.
> +      <a href=3D"formatnode.html#MDEVCap">here for PCI</a> or
> +      <a href=3D"formatnode.html#MDEVCapCSS">here for CSS</a>.

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

>      </p>
>      <p>
>        The following example shows how we might represent an NVIDIA GPU d=
evice
> 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=3D"MDEVCapCSS">mdev_types</a></code></=
dt>
> +                  <dd>
> +                    <span class=3D"since">Since 6.10.0</span>
> +                    This device is capable of creating mediated devices.
> +                    The sub-elements are summarized in
> +                    <a href=3D"#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=3D"MDevTypesCap">mdev_types capability</a></h3>
>  =

>      <p>
> -      <a href=3D"#MDEVCap">PCI</a> devices can be capable of
> -      creating mediated devices.
> +      <a href=3D"#MDEVCap">PCI</a> and <a href=3D"#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 agr=
ee
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=3D"devno">
>        <ref name=3D"ccwDevnoRange"/>
>      </element>
> +    <optional>
> +      <ref name=3D"mdev_types"/>
> +    </optional>
>    </define>
>  =

>    <define name=3D"capvdpa">
> @@ -702,6 +705,7 @@
>            <element name=3D"deviceAPI">
>              <choice>
>                <value>vfio-pci</value>
> +              <value>vfio-ccw</value>
>              </choice>
>            </element>
>            <element name=3D"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 ct=
xt,
>  }
>  =

>  =

> +static int
> +virNodeDevCSSCapabilityParseXML(xmlXPathContextPtr ctxt,
> +                                xmlNodePtr node,
> +                                virNodeDevCapCCWPtr ccw_dev)
> +{
> +    g_autofree char *type =3D virXMLPropString(node, "type");
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt)
> +
> +    ctxt->node =3D node;
> +
> +    if (!type) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing capability ty=
pe"));
> +        return -1;
> +    }
> +
> +    if (STREQ(type, "mdev_types")) {
> +        if (virNodeDevCapMdevTypesParseXML(ctxt,
> +                                           &ccw_dev->mdev_types,
> +                                           &ccw_dev->nmdev_types) < 0)
> +            return -1;
> +        ccw_dev->flags |=3D 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 =3D NULL;
> +    int n =3D 0;
> +    size_t i =3D 0;
>      g_autofree char *cssid =3D NULL;
>      g_autofree char *ssid =3D NULL;
>      g_autofree char *devno =3D NULL;
> @@ -895,6 +929,14 @@ virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt,
>          return -1;
>      }
>  =

> +    if ((n =3D virXPathNodeSet("./capability", ctxt, &nodes)) < 0)
> +        return -1;
> +
> +    for (i =3D 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 =3D 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 de=
f,
>                  ncaps++;
>              }
>          }
> +
> +        if (caps->data.type =3D=3D VIR_NODE_DEV_CAP_CSS_DEV) {
> +            flags =3D 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 *sysfsPa=
th,
>      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 &=3D ~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 |=3D VIR_NODE_DEV_CAP_FLAG_CSS_MDEV;
> +
> +    return 0;
> +}
> +
>  #else
>  =

>  int
> @@ -2675,4 +2756,11 @@ int virNodeDeviceGetSCSITargetCaps(const char *sys=
fsPath 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                  =3D (1 << 3),
>  } virNodeDevPCICapFlags;
>  =

> +typedef enum {
> +    VIR_NODE_DEV_CAP_FLAG_CSS_MDEV                  =3D (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 =3D=3D VIR_NODE_DEV_CAP_MDEV_TYPES &&
> +                (cap->data.ccw_dev.flags & VIR_NODE_DEV_CAP_FLAG_CSS_MDE=
V))
> +                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_de=
vice_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) !=3D 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=3D'css'>
> +    <cssid>0x0</cssid>
> +    <ssid>0x0</ssid>
> +    <devno>0xfffe</devno>
> +    <capability type=3D'mdev_types'>
> +      <type id=3D'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 =3D=3D 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
> -- =

> 2.26.2
> =



--===============8546553232324824493==--