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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Thu, Jun 12, 2014 at 05:27:35PM +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>
[...]
> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> index 505f913..0a8b7fa 100644
> --- a/docs/schemas/nodedev.rng
> +++ b/docs/schemas/nodedev.rng
> @@ -168,6 +168,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'>

List the values here or use the string with pattern from my suggestion
in v1.

> +              </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.h b/src/conf/node_device_conf.h
> index d1311dd..b95ccf1 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -76,10 +76,38 @@ typedef enum {
> } virNodeDevSCSIHostCapFlags;
>
> typedef enum {
> -    VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION		= (1 << 0),
> -    VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION		= (1 << 1),
> +    VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION     = (1 << 0),
> +    VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION      = (1 << 1),
> +    VIR_NODE_DEV_CAP_FLAG_PCIE                      = (1 << 2),
> } virNodeDevPCICapFlags;
>
> +typedef enum {
> +    VIR_PCIE_LINK_SPEED_NA = 0,
> +    VIR_PCIE_LINK_SPEED_25,
> +    VIR_PCIE_LINK_SPEED_5,
> +    VIR_PCIE_LINK_SPEED_8,
> +    VIR_PCIE_LINK_SPEED_LAST
> +} virPCIELinkSpeed;
> +
> +VIR_ENUM_DECL(virPCIELinkSpeed)
> +
> +typedef struct _virPCIELink virPCIELink;
> +typedef virPCIELink *virPCIELinkPtr;
> +struct _virPCIELink {
> +    int port;
> +    virPCIELinkSpeed speed;
> +    unsigned int width;
> +};
> +
> +typedef struct _virPCIEDeviceInfo virPCIEDeviceInfo;
> +typedef virPCIEDeviceInfo *virPCIEDeviceInfoPtr;
> +struct _virPCIEDeviceInfo {
> +    /* Not all PCI Express devices has link. For example this 'Root Complex
> +     * Integrated Endpoint' and 'Root Complex Event Collector' doesn't have. */

Actually, exactly only these two do not have it.

[...]
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index a8e74e4..bb6a0b9 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -426,6 +426,8 @@ static int udevProcessPCI(struct udev_device *device,
>     const char *syspath = NULL;
>     union _virNodeDevCapData *data = &def->caps->data;
>     virPCIDeviceAddress addr;
> +    virPCIEDeviceInfoPtr pci_express = NULL;
> +    virPCIDevicePtr pciDev = NULL;
>     int tmpGroup, ret = -1;
>     char *p;
>     int rc;
> @@ -535,9 +537,41 @@ static int udevProcessPCI(struct udev_device *device,
>         data->pci_dev.iommuGroupNumber = tmpGroup;
>     }
>
> +    if (!(pciDev = virPCIDeviceNew(data->pci_dev.domain,
> +                                   data->pci_dev.bus,
> +                                   data->pci_dev.slot,
> +                                   data->pci_dev.function)))
> +        goto out;
> +
> +    if (virPCIDeviceIsPCIExpress(pciDev) > 0) {
> +        if (VIR_ALLOC(pci_express) < 0)
> +            goto out;
> +
> +        if (virPCIDeviceHasPCIExpressLink(pciDev) > 0) {
> +            if (VIR_ALLOC(pci_express->link_cap) < 0 ||
> +                VIR_ALLOC(pci_express->link_sta) < 0)
> +                goto out;
> +
> +            if (virPCIDeviceGetLinkCapSta(pciDev,
> +                                          &pci_express->link_cap->port,
> +                                          &pci_express->link_cap->speed,
> +                                          &pci_express->link_cap->width,
> +                                          &pci_express->link_sta->speed,
> +                                          &pci_express->link_sta->width) < 0)

You wouldn't have to pass all the params if you defined the structs in
virpci, but that's just a cosmetic thing.

ACK with the RelaxNG schema fixed,

Martin

Attachment: signature.asc
Description: Digital signature

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