5:08 a.m.
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(a)redhat.com>
> ---
> 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