Re: [libvirt] [PATCH v2 07/10] nodedev: Introduce the mdev capability to a PCI parent device

[Pavel Hrdina]

On Thu, Apr 20, 2017 at 03:05:57PM +0200, Erik Skultety wrote:
> The parent device needs to report the generic stuff about the supported
> mediated devices types, like device API, available instances, type name,
> etc. Therefore this patch introduces a new nested capability element of
> type 'mdev' with the resulting XML of the following format:
> 
> 
> ...
>   
>   ...
> 
>   
> optional, raw, unstructured resource allocation data
> 
> vfio-pci
> NUM
>   
>   ...
>   
>   ...
>   
> 
>   
> ...
> 

You've mentioned it in the cover letter and asked me privately to share
my opinion about the mdev capability.  I agree that we should split the
capability into one for parent device and one for child device.  Having
one capability for two different devices where the capability means
something else based on the device doesn't seem to be a good idea especially
if we have an API that allows you list devices whit some specific capability.

The internal representation of the capability is one structure where both
parent and child shares only *type*.  The suggested names mdev-parent and
mdev-child seems as good names if the "parent" and "child" naming is generally
used for mdev devices.

> 
> Signed-off-by: Erik Skultety 
> ---
>  docs/schemas/nodedev.rng  |  24 
>  src/conf/node_device_conf.c   | 103 +
>  src/conf/node_device_conf.h   |  17 +++
>  src/libvirt_private.syms  |   1 +
>  src/node_device/node_device_udev.c| 133 
> ++
>  tests/nodedevschemadata/pci__02_10_7_mdev.xml |  27 +
>  6 files changed, 305 insertions(+)
>  create mode 100644 tests/nodedevschemadata/pci__02_10_7_mdev.xml
> 
> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> index 0f90a73c8..4b5dca777 100644
> --- a/docs/schemas/nodedev.rng
> +++ b/docs/schemas/nodedev.rng
> @@ -205,6 +205,30 @@
>  
>  
>  
> +  
> +
> +  mdev
> +
> +
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  vfio-pci
> +
> +  
> +  
> +
> +  
> +
> +  
> +   
> +
> +
>
>  
>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index fdddc97eb..fe4f1bc60 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -87,6 +87,26 @@ virNodeDevCapsDefParseString(const char *xpath,
>  }
>  
>  
> +static void
> +virNodeDevCapMdevClear(virNodeDevCapMdevPtr mdev)
> +{
> +VIR_FREE(mdev->type);
> +VIR_FREE(mdev->name);
> +VIR_FREE(mdev->device_api);
> +}

This won't be required if we split the mdev capability.

> +void
> +virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev)
> +{
> +if (!mdev)
> +return;
> +
> +virNodeDevCapMdevClear(mdev);
> +VIR_FREE(mdev);
> +}
> +
> +
>  void
>  virNodeDeviceDefFree(virNodeDeviceDefPtr def)
>  {
> @@ -264,6 +284,27 @@ virNodeDeviceCapPCIDefFormat(virBufferPtr buf,
>  virBufferAsprintf(buf, "\n",
>virPCIHeaderTypeToString(data->pci_dev.hdrType));
>  }
> +if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_MDEV) {
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +for (i = 0; i < data->pci_dev.nmdevs; i++) {
> +virNodeDevCapMdevPtr mdev = data->pci_dev.mdevs[i];
> +virBufferEscapeString(buf, "\n", mdev->type);
> +virBufferAdjustIndent(buf, 2);
> +if (mdev->name)
> +virBufferAsprintf(buf, "%s\n",
> +  mdev->name);

This will fit on one line.

> +virBufferAsprintf(buf, "%s\n",
> +  mdev->device_api);
> +virBufferAsprintf(buf,
> +  
> "%u\n",
> +  mdev->available_instances);
> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +}
> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +}
>  if (data->pci_dev.nIommuGroupDevices) {
>  virBufferAsprintf(buf, "\n",
>data->pci_dev.iommuGroupNumber);
> @@ -1358,6 +1399,62 @@ virNodeDevPCICapSRIOVParseXML(xmlXPathContextPtr ctxt,
>  
>  
>  static int
> +virNodeDevPCICapMediatedDevParseXML(xmlXPathContextPtr ctxt,
> +virNodeDevCapPCIDevPtr pci_dev)
> +{
> +int ret = -1;
> +xmlNodePtr orignode = NULL;
> +xmlNodePtr *nodes = NULL;
> +int nmdevs = virXPathNodeSet("./type", ctxt, );
> +virNodeDevCapMdevPtr mdev = NULL;
> +size_t i;
> +
> +orignode = ctxt->node;
> +for (i = 0; i < nmdevs; i++) {
> +  

Re: [libvirt] [PATCH v2 07/10] nodedev: Introduce the mdev capability to a PCI parent device

[Erik Skultety]

[...]

> +static int
> +udevPCIGetMdevCaps(struct udev_device *device,
> +   virNodeDevCapPCIDevPtr pcidata)
> +{
> +int ret = -1;
> +int direrr = -1;
> +DIR *dir = NULL;
> +struct dirent *entry;
> +char *path = NULL;
> +char *tmppath = NULL;
> +virNodeDevCapMdevPtr mdev = NULL;
> +virNodeDevCapMdevPtr *mdevs = NULL;
> +size_t nmdevs = 0;
> +size_t i;
> +
> +if (virAsprintf(, "%s/mdev_supported_types",
> +udev_device_get_syspath(device)) < 0)
> +return -1;
> +
> +if ((direrr = virDirOpenIfExists(, path)) < 0)
> +goto cleanup;
> +
> +if (direrr == 0) {
> +ret = 0;
> +goto cleanup;
> +}
> +
> +if (VIR_ALLOC(mdevs) < 0)
> +goto cleanup;
> +
> +/* since udev doesn't provide means to list other than top-level
> + * attributes, we need to scan the subdirectories ourselves
> + */
> +while ((direrr = virDirRead(dir, , path)) > 0) {
> +if (VIR_ALLOC(mdev) < 0)
> +goto cleanup;
> +
> +if (virAsprintf(, "%s/%s", path, entry->d_name) < 0)
> +goto cleanup;
> +
> +if (udevGetMdevCaps(device, tmppath, mdev) < 0)
> +goto cleanup;
> +
> +if (VIR_APPEND_ELEMENT(mdevs, nmdevs, mdev) < 0)
> +goto cleanup;
> +
> +VIR_FREE(tmppath);
> +}
> +
> +if (direrr < 0)
> +goto cleanup;
> +
> +VIR_STEAL_PTR(pcidata->mdevs, mdevs);
> +pcidata->nmdevs = nmdevs;
> +nmdevs = 0;
> +ret = 0;
> + cleanup:

Oops, I forgot something in here. Consider this bit squashed in (otherwise the
parent device's output would lack some tiny bits)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index a04009110..349d51f69 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -427,6 +427,7 @@ udevPCIGetMdevCaps(struct udev_device *device,

 VIR_STEAL_PTR(pcidata->mdevs, mdevs);
 pcidata->nmdevs = nmdevs;
+pcidata->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_MDEV;
 nmdevs = 0;
 ret = 0;
  cleanup:

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 07/10] nodedev: Introduce the mdev capability to a PCI parent device

[Erik Skultety]

The parent device needs to report the generic stuff about the supported
mediated devices types, like device API, available instances, type name,
etc. Therefore this patch introduces a new nested capability element of
type 'mdev' with the resulting XML of the following format:


...
  
  ...

  
optional, raw, unstructured resource allocation data

vfio-pci
NUM
  
  ...
  
  ...
  

  
...


Signed-off-by: Erik Skultety 
---
 docs/schemas/nodedev.rng  |  24 
 src/conf/node_device_conf.c   | 103 +
 src/conf/node_device_conf.h   |  17 +++
 src/libvirt_private.syms  |   1 +
 src/node_device/node_device_udev.c| 133 ++
 tests/nodedevschemadata/pci__02_10_7_mdev.xml |  27 +
 6 files changed, 305 insertions(+)
 create mode 100644 tests/nodedevschemadata/pci__02_10_7_mdev.xml

diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index 0f90a73c8..4b5dca777 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -205,6 +205,30 @@
 
 
 
+  
+
+  mdev
+
+
+  
+
+  
+  
+
+  
+  
+
+  vfio-pci
+
+  
+  
+
+  
+
+  
+   
+
+
   
 
   
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index fdddc97eb..fe4f1bc60 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -87,6 +87,26 @@ virNodeDevCapsDefParseString(const char *xpath,
 }
 
 
+static void
+virNodeDevCapMdevClear(virNodeDevCapMdevPtr mdev)
+{
+VIR_FREE(mdev->type);
+VIR_FREE(mdev->name);
+VIR_FREE(mdev->device_api);
+}
+
+
+void
+virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev)
+{
+if (!mdev)
+return;
+
+virNodeDevCapMdevClear(mdev);
+VIR_FREE(mdev);
+}
+
+
 void
 virNodeDeviceDefFree(virNodeDeviceDefPtr def)
 {
@@ -264,6 +284,27 @@ virNodeDeviceCapPCIDefFormat(virBufferPtr buf,
 virBufferAsprintf(buf, "\n",
   virPCIHeaderTypeToString(data->pci_dev.hdrType));
 }
+if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_MDEV) {
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+for (i = 0; i < data->pci_dev.nmdevs; i++) {
+virNodeDevCapMdevPtr mdev = data->pci_dev.mdevs[i];
+virBufferEscapeString(buf, "\n", mdev->type);
+virBufferAdjustIndent(buf, 2);
+if (mdev->name)
+virBufferAsprintf(buf, "%s\n",
+  mdev->name);
+virBufferAsprintf(buf, "%s\n",
+  mdev->device_api);
+virBufferAsprintf(buf,
+  "%u\n",
+  mdev->available_instances);
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+}
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+}
 if (data->pci_dev.nIommuGroupDevices) {
 virBufferAsprintf(buf, "\n",
   data->pci_dev.iommuGroupNumber);
@@ -1358,6 +1399,62 @@ virNodeDevPCICapSRIOVParseXML(xmlXPathContextPtr ctxt,
 
 
 static int
+virNodeDevPCICapMediatedDevParseXML(xmlXPathContextPtr ctxt,
+virNodeDevCapPCIDevPtr pci_dev)
+{
+int ret = -1;
+xmlNodePtr orignode = NULL;
+xmlNodePtr *nodes = NULL;
+int nmdevs = virXPathNodeSet("./type", ctxt, );
+virNodeDevCapMdevPtr mdev = NULL;
+size_t i;
+
+orignode = ctxt->node;
+for (i = 0; i < nmdevs; i++) {
+ctxt->node = nodes[i];
+
+if (VIR_ALLOC(mdev) < 0)
+goto cleanup;
+
+if (!(mdev->type = virXPathString("string(./@id[1])", ctxt))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing 'id' attribute for mediated device's "
+ " element"));
+goto cleanup;
+}
+
+if (!(mdev->device_api = virXPathString("string(./deviceAPI[1])", 
ctxt))) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("missing device API for mediated device type 
'%s'"),
+   mdev->type);
+goto cleanup;
+}
+
+if (virXPathUInt("number(./availableInstances)", ctxt,
+ >available_instances) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("missing number of available instances for "
+ "mediated device type '%s'"),
+   mdev->type);
+goto cleanup;
+}
+
+mdev->name = virXPathString("string(./name)", ctxt);
+
+if