[libvirt] [PATCH v2 0/2] nodedev: Expose PCI header type information
v2: Removed <multifunction/> reporting. v1: https://www.redhat.com/archives/libvir-list/2016-March/msg00645.html Martin Kletzander (2): nodedev: Indent PCI express for future fix nodedev: Expose PCI header type docs/schemas/nodedev.rng | 12 +++++++ src/conf/node_device_conf.c | 24 ++++++++++++++ src/conf/node_device_conf.h | 1 + src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 37 ++++++++++++---------- src/util/virpci.c | 33 +++++++++++++++++++ src/util/virpci.h | 12 +++++++ .../pci_0000_00_02_0_header_type.xml | 16 ++++++++++ .../pci_0000_00_1c_0_header_type.xml | 20 ++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 144 insertions(+), 16 deletions(-) create mode 100644 tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml create mode 100644 tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml -- 2.7.3
Best viewed with '-w' as this is just an adjustment for future patch to be readable without that. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/node_device/node_device_udev.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 100b44d20186..aaee0e503d7b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -505,28 +505,30 @@ static int udevProcessPCI(struct udev_device *device, goto out; /* We need to be root to read PCI device configs */ - if (priv->privileged && 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) + if (priv->privileged) { + if (virPCIDeviceIsPCIExpress(pciDev) > 0) { + if (VIR_ALLOC(pci_express) < 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) - goto out; + if (virPCIDeviceHasPCIExpressLink(pciDev) > 0) { + if (VIR_ALLOC(pci_express->link_cap) < 0 || + VIR_ALLOC(pci_express->link_sta) < 0) + goto out; - pci_express->link_sta->port = -1; /* PCIe can't negotiate port. Yet :) */ + 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) + goto out; + + pci_express->link_sta->port = -1; /* PCIe can't negotiate port. Yet :) */ + } + data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCIE; + data->pci_dev.pci_express = pci_express; + pci_express = NULL; } - data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCIE; - data->pci_dev.pci_express = pci_express; - pci_express = NULL; } ret = 0; -- 2.7.3
If we expose this information, which is one byte in every PCI config file, we let all mgmt apps know whether the device itself is an endpoint or not so it's easier for them to decide whether such device can be passed through into a VM (endpoint) or not (*-bridge). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1317531 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/nodedev.rng | 12 ++++++++ src/conf/node_device_conf.c | 24 ++++++++++++++++ src/conf/node_device_conf.h | 1 + src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 3 ++ src/util/virpci.c | 33 ++++++++++++++++++++++ src/util/virpci.h | 12 ++++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++++ .../pci_0000_00_1c_0_header_type.xml | 20 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 126 insertions(+) create mode 100644 tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml create mode 100644 tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 744dccdf5fa9..33637546e48f 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,18 @@ </optional> <optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute> + </element> + </optional> + + <optional> <element name='pci-express'> <zeroOrMore> <element name='link'> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 0c9c348e3793..d173cc826ec9 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -402,6 +402,12 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) if (data->pci_dev.numa_node >= 0) virBufferAsprintf(&buf, "<numa node='%d'/>\n", data->pci_dev.numa_node); + + if (data->pci_dev.hdrType >= 0) { + virBufferAsprintf(&buf, "<header type='%s'/>\n", + virPCIHeaderTypeToString(data->pci_dev.hdrType)); + } + if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCIE) virPCIEDeviceInfoFormat(&buf, data->pci_dev.pci_express); break; @@ -1272,6 +1278,7 @@ virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt, xmlNodePtr orignode, iommuGroupNode, pciExpress; int ret = -1; virPCIEDeviceInfoPtr pci_express = NULL; + char *tmp = NULL; orignode = ctxt->node; ctxt->node = node; @@ -1329,6 +1336,22 @@ virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt, _("invalid NUMA node ID supplied for '%s'")) < 0) goto out; + data->pci_dev.hdrType = -1; + + + + if ((tmp = virXPathString("string(./header[1]/@type)", ctxt))) { + int hdrType = virPCIHeaderTypeFromString(tmp); + + if (hdrType < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown PCI header type '%s'"), tmp); + goto out; + } + + data->pci_dev.hdrType = hdrType; + } + if ((pciExpress = virXPathNode("./pci-express[1]", ctxt))) { if (VIR_ALLOC(pci_express) < 0) goto out; @@ -1343,6 +1366,7 @@ virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt, ret = 0; out: + VIR_FREE(tmp); virPCIEDeviceInfoFree(pci_express); ctxt->node = orignode; return ret; diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index d0071867ccb7..be6dd5eb4ec1 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -119,6 +119,7 @@ typedef struct _virNodeDevCapData { unsigned int iommuGroupNumber; int numa_node; virPCIEDeviceInfoPtr pci_express; + int hdrType; /* enum virPCIHeaderType or -1 */ } pci_dev; struct { unsigned int bus; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55c3047d3db6..d2fcb868356f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2004,11 +2004,14 @@ virPCIDeviceSetUsedBy; virPCIDeviceUnbind; virPCIDeviceWaitForCleanup; virPCIEDeviceInfoFree; +virPCIGetHeaderType; virPCIGetNetName; virPCIGetPhysicalFunction; virPCIGetVirtualFunctionIndex; virPCIGetVirtualFunctionInfo; virPCIGetVirtualFunctions; +virPCIHeaderTypeFromString; +virPCIHeaderTypeToString; virPCIIsVirtualFunction; virPCIStubDriverTypeFromString; virPCIStubDriverTypeToString; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index aaee0e503d7b..6bff5bac7366 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -506,6 +506,9 @@ static int udevProcessPCI(struct udev_device *device, /* We need to be root to read PCI device configs */ if (priv->privileged) { + if (virPCIGetHeaderType(pciDev, &data->pci_dev.hdrType) < 0) + goto out; + if (virPCIDeviceIsPCIExpress(pciDev) > 0) { if (VIR_ALLOC(pci_express) < 0) goto out; diff --git a/src/util/virpci.c b/src/util/virpci.c index 1854318dd114..f7921f86d6de 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -62,6 +62,12 @@ VIR_ENUM_IMPL(virPCIStubDriver, VIR_PCI_STUB_DRIVER_LAST, "vfio-pci", /* VFIO */ ); +VIR_ENUM_IMPL(virPCIHeader, VIR_PCI_HEADER_LAST, + "endpoint", + "pci-bridge", + "cardbus-bridge", +); + struct _virPCIDevice { virPCIDeviceAddress address; @@ -2883,6 +2889,33 @@ virPCIDeviceGetLinkCapSta(virPCIDevicePtr dev, } +int virPCIGetHeaderType(virPCIDevicePtr dev, int *hdrType) +{ + int fd; + uint8_t type; + + *hdrType = -1; + + if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0) + return -1; + + type = virPCIDeviceRead8(dev, fd, PCI_HEADER_TYPE); + + virPCIDeviceConfigClose(dev, fd); + + type &= PCI_HEADER_TYPE_MASK; + if (type >= VIR_PCI_HEADER_LAST) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown PCI header type '%d'"), type); + return -1; + } + + *hdrType = type; + + return 0; +} + + void virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev) { diff --git a/src/util/virpci.h b/src/util/virpci.h index 55329c8a03d3..82f45ec4175f 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -62,6 +62,16 @@ typedef enum { VIR_ENUM_DECL(virPCIELinkSpeed) +typedef enum { + VIR_PCI_HEADER_ENDPOINT = 0, + VIR_PCI_HEADER_PCI_BRIDGE, + VIR_PCI_HEADER_CARDBUS_BRIDGE, + + VIR_PCI_HEADER_LAST +} virPCIHeaderType; + +VIR_ENUM_DECL(virPCIHeader) + typedef struct _virPCIELink virPCIELink; typedef virPCIELink *virPCIELinkPtr; struct _virPCIELink { @@ -223,6 +233,8 @@ int virPCIDeviceGetLinkCapSta(virPCIDevicePtr dev, unsigned int *sta_speed, unsigned int *sta_width); +int virPCIGetHeaderType(virPCIDevicePtr dev, int *hdrType); + void virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev); #endif /* __VIR_PCI_H__ */ diff --git a/tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml b/tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml new file mode 100644 index 000000000000..340d73ec4976 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml @@ -0,0 +1,16 @@ +<device> + <name>pci_0000_00_02_0</name> + <parent>computer</parent> + <capability type='pci'> + <domain>0</domain> + <bus>0</bus> + <slot>2</slot> + <function>0</function> + <product id='0x0416'>4th Gen Core Processor Integrated Graphics Controller</product> + <vendor id='0x8086'>Intel Corporation</vendor> + <iommuGroup number='1'> + <address domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </iommuGroup> + <header type='endpoint'/> + </capability> +</device> diff --git a/tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml b/tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml new file mode 100644 index 000000000000..52574b90b217 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml @@ -0,0 +1,20 @@ +<device> + <name>pci_0000_00_1c_0</name> + <parent>computer</parent> + <capability type='pci'> + <domain>0</domain> + <bus>0</bus> + <slot>28</slot> + <function>0</function> + <product id='0x8c10'>8 Series/C220 Series Chipset Family PCI Express Root Port #1</product> + <vendor id='0x8086'>Intel Corporation</vendor> + <iommuGroup number='8'> + <address domain='0x0000' bus='0x00' slot='0x1c' function='0x0'/> + </iommuGroup> + <header type='pci-bridge'/> + <pci-express> + <link validity='cap' port='1' speed='5' width='1'/> + <link validity='sta' speed='2.5' width='1'/> + </pci-express> + </capability> +</device> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index 0089b5dadffc..f519bce6345f 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -91,6 +91,8 @@ mymain(void) DO_TEST("usb_device_1d6b_1_0000_00_1d_0"); DO_TEST("pci_8086_4238_pcie_wireless"); DO_TEST("pci_8086_0c0c_snd_hda_intel"); + DO_TEST("pci_0000_00_02_0_header_type"); + DO_TEST("pci_0000_00_1c_0_header_type"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.7.3
On Thu, Mar 17, 2016 at 04:51:06PM +0100, Martin Kletzander wrote:
If we expose this information, which is one byte in every PCI config file, we let all mgmt apps know whether the device itself is an endpoint or not so it's easier for them to decide whether such device can be passed through into a VM (endpoint) or not (*-bridge).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1317531
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/nodedev.rng | 12 ++++++++ src/conf/node_device_conf.c | 24 ++++++++++++++++ src/conf/node_device_conf.h | 1 + src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 3 ++ src/util/virpci.c | 33 ++++++++++++++++++++++ src/util/virpci.h | 12 ++++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++++ .../pci_0000_00_1c_0_header_type.xml | 20 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 126 insertions(+) create mode 100644 tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml create mode 100644 tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 744dccdf5fa9..33637546e48f 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,18 @@ </optional>
<optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute> + </element> + </optional>
As before, I think we should be using a capability to express this not creating a new way to express capabilities without calling them capabilities. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Mar 17, 2016 at 04:05:18PM +0000, Daniel P. Berrange wrote:
On Thu, Mar 17, 2016 at 04:51:06PM +0100, Martin Kletzander wrote:
If we expose this information, which is one byte in every PCI config file, we let all mgmt apps know whether the device itself is an endpoint or not so it's easier for them to decide whether such device can be passed through into a VM (endpoint) or not (*-bridge).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1317531
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/nodedev.rng | 12 ++++++++ src/conf/node_device_conf.c | 24 ++++++++++++++++ src/conf/node_device_conf.h | 1 + src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 3 ++ src/util/virpci.c | 33 ++++++++++++++++++++++ src/util/virpci.h | 12 ++++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++++ .../pci_0000_00_1c_0_header_type.xml | 20 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 126 insertions(+) create mode 100644 tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml create mode 100644 tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 744dccdf5fa9..33637546e48f 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,18 @@ </optional>
<optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute> + </element> + </optional>
As before, I think we should be using a capability to express this not creating a new way to express capabilities without calling them capabilities.
Oh and I should say I don't think we need to express 'endpoint' as a capability. It would suffice to justhave <capability type="pci"> ... <capability type="pci-bridge"/> ... </capability> or <capability type="pci"> ... <capability type="cardbus-bridge"/> ... </capability> Any device without one of those two would implicitly be an end point Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Mar 17, 2016 at 04:07:25PM +0000, Daniel P. Berrange wrote:
On Thu, Mar 17, 2016 at 04:05:18PM +0000, Daniel P. Berrange wrote:
On Thu, Mar 17, 2016 at 04:51:06PM +0100, Martin Kletzander wrote:
If we expose this information, which is one byte in every PCI config file, we let all mgmt apps know whether the device itself is an endpoint or not so it's easier for them to decide whether such device can be passed through into a VM (endpoint) or not (*-bridge).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1317531
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/nodedev.rng | 12 ++++++++ src/conf/node_device_conf.c | 24 ++++++++++++++++ src/conf/node_device_conf.h | 1 + src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 3 ++ src/util/virpci.c | 33 ++++++++++++++++++++++ src/util/virpci.h | 12 ++++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++++ .../pci_0000_00_1c_0_header_type.xml | 20 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 126 insertions(+) create mode 100644 tests/nodedevschemadata/pci_0000_00_02_0_header_type.xml create mode 100644 tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 744dccdf5fa9..33637546e48f 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,18 @@ </optional>
<optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute> + </element> + </optional>
As before, I think we should be using a capability to express this not creating a new way to express capabilities without calling them capabilities.
Oh and I should say I don't think we need to express 'endpoint' as a capability. It would suffice to justhave
<capability type="pci"> ... <capability type="pci-bridge"/> ... </capability>
or
<capability type="pci"> ... <capability type="cardbus-bridge"/> ... </capability>
Any device without one of those two would implicitly be an end point
Oh, so you meant capability *inside* the PCI one. I misunderstood that, that makes sense. I'll send a v3 for that. Martin
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Martin Kletzander