Re: [PATCH 2/2] nodedev: Introduce <pci-express/> to PCI devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 06, 2014 at 12:54:18PM +0200, Michal Privoznik wrote:
This new element is there to represent PCI-Express capabilities
of a PCI devices, like link speed, number of lanes, etc.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
docs/formatnode.html.in                            |  19 ++++
docs/schemas/nodedev.rng                           |  26 +++++
src/conf/node_device_conf.c                        | 123 ++++++++++++++++++++-
src/conf/node_device_conf.h                        |  31 +++++-
src/node_device/node_device_udev.c                 |  31 ++++++
.../pci_8086_4238_pcie_wireless.xml                |  26 +++++
tests/nodedevxml2xmltest.c                         |   1 +
7 files changed, 254 insertions(+), 3 deletions(-)
create mode 100644 tests/nodedevschemadata/pci_8086_4238_pcie_wireless.xml

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index b424c96..46ec2bc 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -110,6 +110,21 @@
                have a list of <code>address</code> subelements, one
                for each VF on this PF.
              </dd>
+              <dt><code>pci-express</code></dt>
+              <dd>
+              This optional element contains information on PCI Express part of
+              the device. For example, it can contain a child element
+              <code>link</code> which addresses the PCI Express device's link.
+              While a device has it's own capabilities
+              (<code>validity='cap'</code>), the actual run time capabilities
+              are negotiated on the device initialization
+              (<code>validity='sta'</code>). The <code>link</code> element then
+              contains three attributes: <code>port</code> which says in which
+              port is the device plugged in, <code>speed</code> (in
+              GigaTransfers per second) and <code>width</code> for the number
+              of lanes used. Since the port can't be negotiated, it's not
+              exposed in <code>./pci-express/link/[@validity='sta']</code>.
+              </dd>
            </dl>
          </dd>
          <dt><code>usb_device</code></dt>
@@ -291,6 +306,10 @@
      &lt;address domain='0x0000' bus='0x02' slot='0x00' function='0x0'/&gt;
      &lt;address domain='0x0000' bus='0x02' slot='0x00' function='0x1'/&gt;
    &lt;/iommuGroup&gt;
+    &lt;pci-express&gt;
+      &lt;link validity='cap' port='1' speed='2.5' width='1'/&gt;
+      &lt;link validity='sta' speed='2.5' width='1'/&gt;
+    &lt;/pci-express&gt;
  &lt;/capability&gt;
&lt;/device&gt;
    </pre>
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index 81ab4d4..79e8fd2 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -158,6 +158,32 @@
      </element>
    </optional>

+    <optional>
+        <element name='pci-express'>
+            <zeroOrMore>
+                <element name='link'>
+                    <attribute name='validity'>
+                        <choice>
+                            <value>cap</value>
+                            <value>sta</value>
+                        </choice>
+                    </attribute>
+                    <optional>
+                        <attribute name='port'>
+                            <ref name='unsignedInt'/>
+                        </attribute>
+                    </optional>
+                    <optional>
+                        <attribute name='speed'>

If you don't want to name the values here, there should be at least
<text/> or <data type="string"/>.  Maybe the best would be to do
(not tested):

<data type="string">
  <param name="pattern">[0-9]+(.[0-9]+)?</param>
</data>


+                        </attribute>
+                    </optional>
+                    <attribute name='width'>
+                        <ref name='unsignedInt'/>
+                    </attribute>
+                </element>
+            </zeroOrMore>
+        </element>
+    </optional>
  </define>

  <define name='capusbdev'>
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index e65b5e4..70634cc 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -58,6 +58,9 @@ VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST,
              "80203",
              "80211")

+VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST,
+              "", "2.5", "5", "8")
+
static int
virNodeDevCapsDefParseString(const char *xpath,
                             xmlXPathContextPtr ctxt,
@@ -218,6 +221,35 @@ void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
    }
}

+static void
+virPCIELinkFormat(virBufferPtr buf,
+                  virPCIELinkPtr lnk,
+                  const char *attrib)
+{
+    virBufferAsprintf(buf, "<link validity='%s'", attrib);
+    if (lnk->port >= 0)
+        virBufferAsprintf(buf, " port='%d'", lnk->port);
+    if (lnk->speed)
+        virBufferAsprintf(buf, " speed='%s'",
+                          virPCIELinkSpeedTypeToString(lnk->speed));
+    virBufferAsprintf(buf, " width='%d'", lnk->width);
+    virBufferAddLit(buf, "/>\n");
+}
+
+static void
+virPCIEDeviceInfoFormat(virBufferPtr buf,
+                        virPCIEDeviceInfoPtr info)
+{
+    virBufferAddLit(buf, "<pci-express>\n");
+    virBufferAdjustIndent(buf, 2);
+
+    virPCIELinkFormat(buf, &info->link_cap, "cap");
+    virPCIELinkFormat(buf, &info->link_sta, "sta");
+
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</pci-express>\n");
+}
+
char *virNodeDeviceDefFormat(const virNodeDeviceDef *def)
{
    virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -346,6 +378,8 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def)
                virBufferAdjustIndent(&buf, -2);
                virBufferAddLit(&buf, "</iommuGroup>\n");
            }
+            if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCIE)
+                virPCIEDeviceInfoFormat(&buf, data->pci_dev.pci_express);
            break;
        case VIR_NODE_DEV_CAP_USB_DEV:
            virBufferAsprintf(&buf, "<bus>%d</bus>\n", data->usb_dev.bus);
@@ -1043,6 +1077,87 @@ virNodeDevCapPCIDevIommuGroupParseXML(xmlXPathContextPtr ctxt,
    return ret;
}

+static int
+virNodeDevCapPCIDevPCIExpressParseLinkXML(xmlXPathContextPtr ctxt,
+                                          xmlNodePtr linkNode,
+                                          virPCIELinkPtr lnk)
+{
+    xmlNodePtr origNode = ctxt->node;
+    int ret = -1, speed;
+    char *speedStr = NULL, *portStr = NULL;
+
+    ctxt->node = linkNode;
+
+    if (virXPathUInt("number(./@width)", ctxt, &lnk->width) < 0) {
+        virReportError(VIR_ERR_XML_DETAIL, "%s",
+                       _("mandatory attribute 'width' is missing or malformed"));
+        goto cleanup;
+    }
+
+    if ((speedStr = virXPathString("string(./@speed)", ctxt))) {
+        if ((speed = virPCIELinkSpeedTypeFromString(speedStr)) < 0) {
+            virReportError(VIR_ERR_XML_DETAIL,
+                           _("malformed 'speed' attribute: %s"),
+                           speedStr);
+            goto cleanup;
+        }
+        lnk->speed = speed;
+    }

It would be nice (maybe for some future code) if you reset lnk->speed
to 0 if the provided structure is not clean.  Even though you allocate
it currently with VIR_ALLOC.

+
+    if ((portStr = virXPathString("string(./@port)", ctxt))) {
+        if (virStrToLong_i(portStr, NULL, 10, &lnk->port) < 0) {
+            virReportError(VIR_ERR_XML_DETAIL,
+                           _("malformed 'port' attribute: %s"),
+                           portStr);
+            goto cleanup;
+        }
+    } else {
+        lnk->port = -1;
+    }

The same way you do it here with port.

+
+    ret = 0;
+ cleanup:
+    VIR_FREE(portStr);
+    VIR_FREE(speedStr);
+    ctxt->node = origNode;
+    return ret;
+}
+
+static int
+virNodeDevCapPCIDevPCIExpressParseXML(xmlXPathContextPtr ctxt,

How about virPCIEDeviceInfoParse()?  And s/Parse/LinkParse/ for the
one above.

Martin

Attachment: signature.asc
Description: Digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]