[libvirt PATCH v2 02/10] nodedev: add support for mdev attributes

Jonathon Jongsma posted 10 patches 5 years, 8 months ago
There is a newer version of this series
[libvirt PATCH v2 02/10] nodedev: add support for mdev attributes
Posted by Jonathon Jongsma 5 years, 8 months ago
Mediated devices support arbitrary vendor-specific attributes that can
be attached to a mediated device. These attributes are ordered, and are
written to sysfs in order after a device is created. This patch adds
support for these attributes to the mdev data types and XML schema.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 docs/formatnode.html.in     |  7 +++++
 docs/schemas/nodedev.rng    |  6 ++++
 src/conf/node_device_conf.c | 59 +++++++++++++++++++++++++++++++++++--
 src/conf/node_device_conf.h |  2 ++
 src/libvirt_private.syms    |  2 ++
 src/util/virmdev.c          | 12 ++++++++
 src/util/virmdev.h          | 11 +++++++
 7 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index 76eae928de..a46b73254b 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -393,6 +393,13 @@
                 which holds the IOMMU group number the mediated device belongs
                   to.
               </dd>
+              <dt><code>attr</code></dt>
+              <dd>
+                This optional element can occur multiple times. It represents a
+                vendor-specific attribute that is used to configure this
+                mediated device. It has two required attributes:
+                <code>name</code> and <code>value</code>.
+              </dd>
             </dl>
           </dd>
           <dt><code>ccw</code></dt>
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index fe6ffa0b53..a1ce09af54 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -634,6 +634,12 @@
         <ref name='unsignedInt'/>
       </attribute>
     </element>
+    <zeroOrMore>
+      <element name="attr">
+        <attribute name="name"/>
+        <attribute name="value"/>
+      </element>
+    </zeroOrMore>
   </define>
 
   <define name='capccwdev'>
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index bccdbd0af8..045d146433 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -500,6 +500,21 @@ virNodeDeviceCapStorageDefFormat(virBufferPtr buf,
         virBufferAddLit(buf, "<capability type='hotpluggable'/>\n");
 }
 
+static void
+virNodeDeviceCapMdevDefFormat(virBufferPtr buf,
+                              const virNodeDevCapData *data)
+{
+    size_t i;
+
+    virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.type);
+    virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n",
+                      data->mdev.iommuGroupNumber);
+    for (i = 0; i < data->mdev.nattributes; i++) {
+        virMediatedDeviceAttrPtr attr = data->mdev.attributes[i];
+        virBufferAsprintf(buf, "<attr name='%s' value='%s'/>\n",
+                          attr->name, attr->value);
+    }
+}
 
 char *
 virNodeDeviceDefFormat(const virNodeDeviceDef *def)
@@ -583,9 +598,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
             virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type));
             break;
         case VIR_NODE_DEV_CAP_MDEV:
-            virBufferEscapeString(&buf, "<type id='%s'/>\n", data->mdev.type);
-            virBufferAsprintf(&buf, "<iommuGroup number='%u'/>\n",
-                              data->mdev.iommuGroupNumber);
+            virNodeDeviceCapMdevDefFormat(&buf, data);
             break;
         case VIR_NODE_DEV_CAP_CCW_DEV:
             virBufferAsprintf(&buf, "<cssid>0x%x</cssid>\n",
@@ -1757,6 +1770,35 @@ virNodeDevCapSystemParseXML(xmlXPathContextPtr ctxt,
     return ret;
 }
 
+static int
+virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt,
+                                   xmlNodePtr node,
+                                   virNodeDevCapMdevPtr mdev)
+{
+    xmlNodePtr orig_node = node;
+    ctxt->node = node;
+    int ret = -1;
+    g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew();
+
+    attr->name = virXPathString("string(./@name)", ctxt);
+    attr->value = virXPathString("string(./@value)", ctxt);
+    if (!attr->name || !attr->value) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("mdev attribute missing name or value"));
+        goto cleanup;
+    }
+
+    if (VIR_APPEND_ELEMENT(mdev->attributes,
+                           mdev->nattributes,
+                           attr) < 0)
+      goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    ctxt->node = orig_node;
+    return ret;
+}
 
 static int
 virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
@@ -1766,6 +1808,9 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
 {
     VIR_XPATH_NODE_AUTORESTORE(ctxt);
     int ret = -1;
+    int nattrs = 0;
+    xmlNodePtr *attrs;
+    size_t i;
 
     ctxt->node = node;
 
@@ -1783,6 +1828,11 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
                                       "'%s'")) < 0)
         goto out;
 
+    if ((nattrs = virXPathNodeSet("./attr", ctxt, &attrs)) < 0)
+        goto out;
+    for (i = 0; i < nattrs; i++)
+        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], mdev);
+
     ret = 0;
  out:
     return ret;
@@ -2172,6 +2222,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
         break;
     case VIR_NODE_DEV_CAP_MDEV:
         VIR_FREE(data->mdev.type);
+        for (i = 0; i < data->mdev.nattributes; i++)
+            virMediatedDeviceAttrFree(data->mdev.attributes[i]);
+        VIR_FREE(data->mdev.attributes);
         break;
     case VIR_NODE_DEV_CAP_MDEV_TYPES:
     case VIR_NODE_DEV_CAP_DRM:
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 9e4b0847fb..e3e1e788d4 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -141,6 +141,8 @@ typedef virNodeDevCapMdev *virNodeDevCapMdevPtr;
 struct _virNodeDevCapMdev {
     char *type;
     unsigned int iommuGroupNumber;
+    virMediatedDeviceAttrPtr *attributes;
+    size_t nattributes;
 };
 
 typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a6af44fe1c..b8e6f058c3 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2486,6 +2486,8 @@ virMacMapRemove;
 virMacMapWriteFile;
 
 # util/virmdev.h
+virMediatedDeviceAttrFree;
+virMediatedDeviceAttrNew;
 virMediatedDeviceFree;
 virMediatedDeviceGetIOMMUGroupDev;
 virMediatedDeviceGetIOMMUGroupNum;
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 51a88a91d7..b8023dd991 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -512,3 +512,15 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath,
 
     return 0;
 }
+
+virMediatedDeviceAttrPtr virMediatedDeviceAttrNew(void)
+{
+    return g_new0(virMediatedDeviceAttr, 1);
+}
+
+void virMediatedDeviceAttrFree(virMediatedDeviceAttrPtr attr)
+{
+    g_free(attr->name);
+    g_free(attr->value);
+    g_free(attr);
+}
diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index 51f7f608a2..eb167ccb48 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -37,6 +37,17 @@ typedef struct _virMediatedDevice virMediatedDevice;
 typedef virMediatedDevice *virMediatedDevicePtr;
 typedef struct _virMediatedDeviceList virMediatedDeviceList;
 typedef virMediatedDeviceList *virMediatedDeviceListPtr;
+typedef struct _virMediatedDeviceAttr virMediatedDeviceAttr;
+typedef virMediatedDeviceAttr *virMediatedDeviceAttrPtr;
+
+struct _virMediatedDeviceAttr {
+    char *name;
+    char *value;
+};
+
+virMediatedDeviceAttrPtr virMediatedDeviceAttrNew(void);
+void virMediatedDeviceAttrFree(virMediatedDeviceAttrPtr attr);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceAttr, virMediatedDeviceAttrFree);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceList, virObjectUnref);
 
-- 
2.21.3

Re: [libvirt PATCH v2 02/10] nodedev: add support for mdev attributes
Posted by Michal Privoznik 5 years, 8 months ago
On 6/9/20 11:43 PM, Jonathon Jongsma wrote:
> Mediated devices support arbitrary vendor-specific attributes that can
> be attached to a mediated device. These attributes are ordered, and are
> written to sysfs in order after a device is created. This patch adds
> support for these attributes to the mdev data types and XML schema.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>   docs/formatnode.html.in     |  7 +++++
>   docs/schemas/nodedev.rng    |  6 ++++
>   src/conf/node_device_conf.c | 59 +++++++++++++++++++++++++++++++++++--
>   src/conf/node_device_conf.h |  2 ++
>   src/libvirt_private.syms    |  2 ++
>   src/util/virmdev.c          | 12 ++++++++
>   src/util/virmdev.h          | 11 +++++++
>   7 files changed, 96 insertions(+), 3 deletions(-)
> 


> +static int
> +virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt,
> +                                   xmlNodePtr node,
> +                                   virNodeDevCapMdevPtr mdev)
> +{
> +    xmlNodePtr orig_node = node;
> +    ctxt->node = node;

Alternatively, VIR_XPATH_NODE_AUTORESTORE() can be used.

> +    int ret = -1;
> +    g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew();
> +
> +    attr->name = virXPathString("string(./@name)", ctxt);
> +    attr->value = virXPathString("string(./@value)", ctxt);
> +    if (!attr->name || !attr->value) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("mdev attribute missing name or value"));
> +        goto cleanup;
> +    }
> +
> +    if (VIR_APPEND_ELEMENT(mdev->attributes,
> +                           mdev->nattributes,
> +                           attr) < 0)
> +      goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    ctxt->node = orig_node;
> +    return ret;
> +}
>   
>   static int
>   virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
> @@ -1766,6 +1808,9 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>   {
>       VIR_XPATH_NODE_AUTORESTORE(ctxt);
>       int ret = -1;
> +    int nattrs = 0;
> +    xmlNodePtr *attrs;

g_autofree becasue ..

> +    size_t i;
>   
>       ctxt->node = node;
>   
> @@ -1783,6 +1828,11 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>                                         "'%s'")) < 0)
>           goto out;
>   
> +    if ((nattrs = virXPathNodeSet("./attr", ctxt, &attrs)) < 0)
> +        goto out;

.. this allocates @attrs array.

> +    for (i = 0; i < nattrs; i++)
> +        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], mdev);
> +
>       ret = 0;
>    out:
>       return ret;


Just squash this in:

diff --git i/src/conf/node_device_conf.c w/src/conf/node_device_conf.c
index 045d146433..7b2a8c5545 100644
--- i/src/conf/node_device_conf.c
+++ w/src/conf/node_device_conf.c
@@ -1775,29 +1775,21 @@ 
virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt,
                                     xmlNodePtr node,
                                     virNodeDevCapMdevPtr mdev)
  {
-    xmlNodePtr orig_node = node;
-    ctxt->node = node;
-    int ret = -1;
+    VIR_XPATH_NODE_AUTORESTORE(ctxt);
      g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew();

+    ctxt->node = node;
      attr->name = virXPathString("string(./@name)", ctxt);
      attr->value = virXPathString("string(./@value)", ctxt);
      if (!attr->name || !attr->value) {
          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                         _("mdev attribute missing name or value"));
-        goto cleanup;
+        return -1;
      }

-    if (VIR_APPEND_ELEMENT(mdev->attributes,
-                           mdev->nattributes,
-                           attr) < 0)
-      goto cleanup;
-
-    ret = 0;
-
- cleanup:
-    ctxt->node = orig_node;
-    return ret;
+    return VIR_APPEND_ELEMENT(mdev->attributes,
+                              mdev->nattributes,
+                              attr);
  }

  static int
@@ -1809,7 +1801,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
      VIR_XPATH_NODE_AUTORESTORE(ctxt);
      int ret = -1;
      int nattrs = 0;
-    xmlNodePtr *attrs;
+    g_autofree xmlNodePtr *attrs = NULL;
      size_t i;

      ctxt->node = node;



Michal

Re: [libvirt PATCH v2 02/10] nodedev: add support for mdev attributes
Posted by Erik Skultety 5 years, 8 months ago
On Tue, Jun 09, 2020 at 04:43:42PM -0500, Jonathon Jongsma wrote:
> Mediated devices support arbitrary vendor-specific attributes that can
> be attached to a mediated device. These attributes are ordered, and are
> written to sysfs in order after a device is created. This patch adds
> support for these attributes to the mdev data types and XML schema.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  docs/formatnode.html.in     |  7 +++++
>  docs/schemas/nodedev.rng    |  6 ++++
>  src/conf/node_device_conf.c | 59 +++++++++++++++++++++++++++++++++++--
>  src/conf/node_device_conf.h |  2 ++
>  src/libvirt_private.syms    |  2 ++
>  src/util/virmdev.c          | 12 ++++++++
>  src/util/virmdev.h          | 11 +++++++
>  7 files changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
> index 76eae928de..a46b73254b 100644
> --- a/docs/formatnode.html.in
> +++ b/docs/formatnode.html.in
> @@ -393,6 +393,13 @@
>                  which holds the IOMMU group number the mediated device belongs
>                    to.
>                </dd>
> +              <dt><code>attr</code></dt>
> +              <dd>
> +                This optional element can occur multiple times. It represents a
> +                vendor-specific attribute that is used to configure this
> +                mediated device. It has two required attributes:
> +                <code>name</code> and <code>value</code>.
> +              </dd>
>              </dl>
>            </dd>
>            <dt><code>ccw</code></dt>
> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> index fe6ffa0b53..a1ce09af54 100644
> --- a/docs/schemas/nodedev.rng
> +++ b/docs/schemas/nodedev.rng
> @@ -634,6 +634,12 @@
>          <ref name='unsignedInt'/>
>        </attribute>
>      </element>
> +    <zeroOrMore>
> +      <element name="attr">
> +        <attribute name="name"/>
> +        <attribute name="value"/>
> +      </element>
> +    </zeroOrMore>

Only contextually related, but this patch should be preceded by one that makes
iommugroup an optional element (this change would have to go to the parser as
well). Since before this series, mdev XMLs were either not internally parsed
at all or it would have come from inside libvirt, I'm saying this because even
though we wouldn't break backwards compatibility, because we'd be relaxing the
RNG and parser (which is ok), but I'd still like to see that change to take
effect before this series is fully applied.

With Michal's comments:
Reviewed-by: Erik Skultety <eskultet@redhat.com>