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

[Michal Privoznik]

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