Re: [PATCH 1/2] virpci: Introduce virPCIDeviceIsPCIExpress and friends

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

 



On 12.06.2014 10:56, Martin Kletzander wrote:
On Fri, Jun 06, 2014 at 12:54:17PM +0200, Michal Privoznik wrote:
These functions will handle PCIe devices and their link capabilities
to query some info about it.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
src/libvirt_private.syms |  3 ++
src/util/virpci.c        | 81
+++++++++++++++++++++++++++++++++++++++++++++++-
src/util/virpci.h        |  8 +++++
3 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d73a9f5..f72a3ad 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1690,6 +1690,8 @@ virPCIDeviceFree;
virPCIDeviceGetDriverPathAndName;
virPCIDeviceGetIOMMUGroupDev;
virPCIDeviceGetIOMMUGroupList;
+virPCIDeviceGetLinkCap;
+virPCIDeviceGetLinkSta;
virPCIDeviceGetManaged;
virPCIDeviceGetName;
virPCIDeviceGetRemoveSlot;
@@ -1698,6 +1700,7 @@ virPCIDeviceGetStubDriver;
virPCIDeviceGetUnbindFromStub;
virPCIDeviceGetUsedBy;
virPCIDeviceIsAssignable;
+virPCIDeviceIsPCIExpress;
virPCIDeviceListAdd;
virPCIDeviceListAddCopy;
virPCIDeviceListCount;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index e0f2344..8ad28b8 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -130,7 +130,13 @@ struct _virPCIDeviceList {

/* PCIe20 7.8.3  Device Capabilities Register (Offset 04h) */
#define PCI_EXP_DEVCAP          0x4     /* Device capabilities */
-#define PCI_EXP_DEVCAP_FLR     (1<<28) /* Function Level Reset */
+#define PCI_EXP_LNKCAP          0xc     /* Link Capabilities */
+#define PCI_EXP_LNKSTA          0x12    /* Link Status */
+#define PCI_EXP_DEVCAP_FLR     (1<<28)  /* Function Level Reset */
+#define PCI_EXP_LNKCAP_SPEED    0x0000f /* Maximum Link Speed */
+#define PCI_EXP_LNKCAP_WIDTH    0x003f0 /* Maximum Link Width */
+#define PCI_EXP_LNKSTA_SPEED    0x000f  /* Negotiated Link Speed */
+#define PCI_EXP_LNKSTA_WIDTH    0x03f0  /* Negotiated Link Width */


These two sets are essentially the same, just keep it as e.g.
PCI_EXP_LINK_(SPEED|WIDTH) with 0x(f|3f) respectively.  And keep the
order of the names alphabetical.

The values are copied from /usr/include/pci/header.h. One day we could drop all of these copies and include the file directly. For that, I'd rather keep it just as is in the foreign file. Until the time we do that, I'm sorting these alphabetically, okay.


/* Header type 1 BR12 3.2 PCI-to-PCI Bridge Configuration Space Header
Format */
#define PCI_PRIMARY_BUS         0x18    /* BR12 3.2.5.2 Primary bus
number */
@@ -2750,3 +2756,76 @@ virPCIGetVirtualFunctionInfo(const char
*vf_sysfs_device_path ATTRIBUTE_UNUSED,
    return -1;
}
#endif /* __linux__ */
+
+int
+virPCIDeviceIsPCIExpress(virPCIDevicePtr dev)
+{
+    int fd;
+    int ret = -1;
+
+    if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
+        return ret;
+
+    if (virPCIDeviceInit(dev, fd) < 0)
+        goto cleanup;
+
+    ret = dev->pcie_cap_pos != 0;
+
+ cleanup:
+    virPCIDeviceConfigClose(dev, fd);
+    return ret;
+}
+
+int
+virPCIDeviceGetLinkCap(virPCIDevicePtr dev,
+                       int *port,
+                       unsigned int *speed,
+                       unsigned int *width)
+{
+    uint32_t t;
+    int fd;
+    int ret = -1;
+
+    if ((fd = virPCIDeviceConfigOpen(dev, true) < 0))
+        return ret;
+
+    if (virPCIDeviceInit(dev, fd) < 0)
+        goto cleanup;
+
+    t = virPCIDeviceRead32(dev, fd, dev->pcie_cap_pos + PCI_EXP_LNKCAP);
+
+    *port = t >> 0x18;

Took me second to figure out you just want the highest byte.
s/0x18/24/ would make that much more clear, at least for me.

+    *speed = t & PCI_EXP_LNKCAP_SPEED;
+    *width = (t & PCI_EXP_LNKCAP_WIDTH) >> 4;

And if I wanted to be *really* digging deep into this and nitpicking a
lot, I'd say you can do:

port = Read8
speed = Read16 & SPEED
width = Read8


The truth is, I've took my inspiration from lscpi sources. But your suggestion is good also.

+    ret = 0;
+
+ cleanup:
+    virPCIDeviceConfigClose(dev, fd);
+    return ret;
+}
+
+int
+virPCIDeviceGetLinkSta(virPCIDevicePtr dev,
+                       unsigned int *speed,
+                       unsigned int *width)
+{
+    uint32_t t;
+    int fd;
+    int ret = -1;
+
+    if ((fd = virPCIDeviceConfigOpen(dev, true) < 0))
+        return ret;
+
+    if (virPCIDeviceInit(dev, fd) < 0)
+        goto cleanup;
+
+    t = virPCIDeviceRead32(dev, fd, dev->pcie_cap_pos + PCI_EXP_LNKSTA);
+
+    *speed = t & PCI_EXP_LNKSTA_SPEED;
+    *width = (t & PCI_EXP_LNKSTA_WIDTH) >> 4;
+    ret = 0;
+
+ cleanup:
+    virPCIDeviceConfigClose(dev, fd);
+    return ret;
+}

Both these functions do the same thing with 2 micro differences,
either merge it to one, so you avoid double open, init, etc.  Or
create one that at least takes a parameter saying whether the caller
wants CAPability or STAtus data.

I'll merge those two.


Other than that it looks good.

Martin

--
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]