[libvirt] [PATCH 0/2] nodedev: Expose PCI header type information
I can squash those two patches together if it's desirable. Martin Kletzander (2): nodedev: Indent PCI express for future fix nodedev: Expose PCI header type docs/schemas/nodedev.rng | 17 ++++++++++ src/conf/node_device_conf.c | 37 ++++++++++++++++++++ src/conf/node_device_conf.h | 2 ++ src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 39 +++++++++++++--------- src/util/virpci.c | 38 +++++++++++++++++++++ src/util/virpci.h | 12 +++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++ .../pci_0000_00_1c_0_header_type.xml | 22 ++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 172 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 | 17 ++++++++++ src/conf/node_device_conf.c | 37 +++++++++++++++++++++ src/conf/node_device_conf.h | 2 ++ src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 5 +++ src/util/virpci.c | 38 ++++++++++++++++++++++ src/util/virpci.h | 12 +++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++ .../pci_0000_00_1c_0_header_type.xml | 22 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 154 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..7aec2adf48e9 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,23 @@ </optional> <optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute> + <optional> + <element name='multifunction'> + <empty/> + </element> + </optional> + </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 c04739f90121..20aa14474223 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -402,6 +402,23 @@ 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'", + virPCIHeaderTypeToString(data->pci_dev.hdrType)); + if (!data->pci_dev.multiFunc) { + virBufferAddLit(&buf, "/>\n"); + } else { + virBufferAddLit(&buf, ">\n"); + + virBufferAdjustIndent(&buf, 2); + virBufferAddLit(&buf, "<multifunction/>\n"); + virBufferAdjustIndent(&buf, -2); + + virBufferAddLit(&buf, "</header>\n"); + } + } + if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCIE) virPCIEDeviceInfoFormat(&buf, data->pci_dev.pci_express); break; @@ -1272,6 +1289,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 +1347,24 @@ 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; + data->pci_dev.multiFunc = virXPathNode("./header[1]/multifunction", + ctxt); + } + if ((pciExpress = virXPathNode("./pci-express[1]", ctxt))) { if (VIR_ALLOC(pci_express) < 0) goto out; @@ -1343,6 +1379,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..a42b3de9e108 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -119,6 +119,8 @@ typedef struct _virNodeDevCapData { unsigned int iommuGroupNumber; int numa_node; virPCIEDeviceInfoPtr pci_express; + int hdrType; /* enum virPCIHeaderType or -1 */ + bool multiFunc; } 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..cdd317c9ffc8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -506,6 +506,11 @@ 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, + &data->pci_dev.multiFunc) < 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..b6a0fa345117 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,38 @@ virPCIDeviceGetLinkCapSta(virPCIDevicePtr dev, } +int virPCIGetHeaderType(virPCIDevicePtr dev, int *hdrType, bool *multiFunc) +{ + int fd; + uint8_t type; + bool multi = false; + + *hdrType = -1; + *multiFunc = 0; + + if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0) + return -1; + + type = virPCIDeviceRead8(dev, fd, PCI_HEADER_TYPE); + + virPCIDeviceConfigClose(dev, fd); + + multi = type & PCI_HEADER_TYPE_MULTI; + 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; + *multiFunc = multi; + + return 0; +} + + void virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev) { diff --git a/src/util/virpci.h b/src/util/virpci.h index 55329c8a03d3..cf371e15784d 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, bool *multiFunc); + 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..3a30dae7fec4 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml @@ -0,0 +1,22 @@ +<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'> + <multifunction/> + </header> + <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 Tue, Mar 15, 2016 at 05:23:30PM +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 | 17 ++++++++++ src/conf/node_device_conf.c | 37 +++++++++++++++++++++ src/conf/node_device_conf.h | 2 ++ src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 5 +++ src/util/virpci.c | 38 ++++++++++++++++++++++ src/util/virpci.h | 12 +++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++ .../pci_0000_00_1c_0_header_type.xml | 22 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 154 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..7aec2adf48e9 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,23 @@ </optional>
<optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute>
We already have nested <capability> elements in the node device data, and reporting this header type just looks like another set of capabilities to me. ie <capability type='endpoint'/>
+ <optional> + <element name='multifunction'>
Your commit message mentioned nothing about adding a <multifunction/> element. It looks tangential to the goal of adding info about the device type, so should be a separate patch. In any case we already have a <capability type='virt_functions'> <address domain='0x0000' bus='0x05' slot='0x10' function='0x0'/> <address domain='0x0000' bus='0x05' slot='0x10' function='0x4'/> <address domain='0x0000' bus='0x05' slot='0x11' function='0x0'/> <address domain='0x0000' bus='0x05' slot='0x11' function='0x4'/> </capability> which is reported against physical functions. What's missing is that we omit the capability entirely if no functions are currently enabled. We also don't report the total number of functions that are possible. IMHO we should address it those problems rather than addign a new element. 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 Tue, Mar 15, 2016 at 04:37:55PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 15, 2016 at 05:23:30PM +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 | 17 ++++++++++ src/conf/node_device_conf.c | 37 +++++++++++++++++++++ src/conf/node_device_conf.h | 2 ++ src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 5 +++ src/util/virpci.c | 38 ++++++++++++++++++++++ src/util/virpci.h | 12 +++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++ .../pci_0000_00_1c_0_header_type.xml | 22 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 154 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..7aec2adf48e9 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,23 @@ </optional>
<optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute>
We already have nested <capability> elements in the node device data, and reporting this header type just looks like another set of capabilities to me. ie <capability type='endpoint'/>
But the devices that this is valid for are exactly only PCI ones. And all PCI ones. That's why I included it in capability type='pci'. It's not capability by itself, it's just information extracted from one Byte in PCI config.
+ <optional> + <element name='multifunction'>
Your commit message mentioned nothing about adding a <multifunction/> element. It looks tangential to the goal of adding info about the device type, so should be a separate patch.
OK, I should've explained it more clearly. This information is taken from the first bit of the PCI config header type field. That's what I meant in the commit message as exposing all the information we can get from that one Byte as it's part of that. That's also why that element is included *inside* the header element as it's part of that information.
In any case we already have a
<capability type='virt_functions'> <address domain='0x0000' bus='0x05' slot='0x10' function='0x0'/> <address domain='0x0000' bus='0x05' slot='0x10' function='0x4'/> <address domain='0x0000' bus='0x05' slot='0x11' function='0x0'/> <address domain='0x0000' bus='0x05' slot='0x11' function='0x4'/> </capability>
which is reported against physical functions. What's missing is that we omit the capability entirely if no functions are currently enabled. We also don't report the total number of functions that are possible. IMHO we should address it those problems rather than addign a new element.
The 'multifunction' element I added has nothing to do with virtual functions. It merely indicates that the physical device is supposed to be multifunction; as in there is another device with the same address, but different value of 'function' part of that address. I think explaining all this in the commit message would be enough as I believe the patch is still correct. Let me know if that's fine or if I misunderstood, thanks.
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 Wed, Mar 16, 2016 at 10:46:10AM +0100, Martin Kletzander wrote:
On Tue, Mar 15, 2016 at 04:37:55PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 15, 2016 at 05:23:30PM +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 | 17 ++++++++++ src/conf/node_device_conf.c | 37 +++++++++++++++++++++ src/conf/node_device_conf.h | 2 ++ src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 5 +++ src/util/virpci.c | 38 ++++++++++++++++++++++ src/util/virpci.h | 12 +++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++ .../pci_0000_00_1c_0_header_type.xml | 22 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 154 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..7aec2adf48e9 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,23 @@ </optional>
<optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute>
We already have nested <capability> elements in the node device data, and reporting this header type just looks like another set of capabilities to me. ie <capability type='endpoint'/>
But the devices that this is valid for are exactly only PCI ones. And all PCI ones. That's why I included it in capability type='pci'. It's not capability by itself, it's just information extracted from one Byte in PCI config.
We have nested capabilities already. This would be a nested capability below the main PCI capability, which scopes it to just PCI devices.
In any case we already have a
<capability type='virt_functions'> <address domain='0x0000' bus='0x05' slot='0x10' function='0x0'/> <address domain='0x0000' bus='0x05' slot='0x10' function='0x4'/> <address domain='0x0000' bus='0x05' slot='0x11' function='0x0'/> <address domain='0x0000' bus='0x05' slot='0x11' function='0x4'/> </capability>
which is reported against physical functions. What's missing is that we omit the capability entirely if no functions are currently enabled. We also don't report the total number of functions that are possible. IMHO we should address it those problems rather than addign a new element.
The 'multifunction' element I added has nothing to do with virtual functions. It merely indicates that the physical device is supposed to be multifunction; as in there is another device with the same address, but different value of 'function' part of that address.
That doesn't really seem to add any value then since an application can already see there are multiple functions in the same slot via the address info we provide 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 10:00:36AM +0000, Daniel P. Berrange wrote:
On Wed, Mar 16, 2016 at 10:46:10AM +0100, Martin Kletzander wrote:
On Tue, Mar 15, 2016 at 04:37:55PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 15, 2016 at 05:23:30PM +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 | 17 ++++++++++ src/conf/node_device_conf.c | 37 +++++++++++++++++++++ src/conf/node_device_conf.h | 2 ++ src/libvirt_private.syms | 3 ++ src/node_device/node_device_udev.c | 5 +++ src/util/virpci.c | 38 ++++++++++++++++++++++ src/util/virpci.h | 12 +++++++ .../pci_0000_00_02_0_header_type.xml | 16 +++++++++ .../pci_0000_00_1c_0_header_type.xml | 22 +++++++++++++ tests/nodedevxml2xmltest.c | 2 ++ 10 files changed, 154 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..7aec2adf48e9 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -169,6 +169,23 @@ </optional>
<optional> + <element name='header'> + <attribute name='type'> + <choice> + <value>endpoint</value> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute>
We already have nested <capability> elements in the node device data, and reporting this header type just looks like another set of capabilities to me. ie <capability type='endpoint'/>
But the devices that this is valid for are exactly only PCI ones. And all PCI ones. That's why I included it in capability type='pci'. It's not capability by itself, it's just information extracted from one Byte in PCI config.
We have nested capabilities already. This would be a nested capability below the main PCI capability, which scopes it to just PCI devices.
In any case we already have a
<capability type='virt_functions'> <address domain='0x0000' bus='0x05' slot='0x10' function='0x0'/> <address domain='0x0000' bus='0x05' slot='0x10' function='0x4'/> <address domain='0x0000' bus='0x05' slot='0x11' function='0x0'/> <address domain='0x0000' bus='0x05' slot='0x11' function='0x4'/> </capability>
which is reported against physical functions. What's missing is that we omit the capability entirely if no functions are currently enabled. We also don't report the total number of functions that are possible. IMHO we should address it those problems rather than addign a new element.
The 'multifunction' element I added has nothing to do with virtual functions. It merely indicates that the physical device is supposed to be multifunction; as in there is another device with the same address, but different value of 'function' part of that address.
That doesn't really seem to add any value then since an application can already see there are multiple functions in the same slot via the address info we provide
If that's always the case, then yes. And I don't know about any occasion when that wouldn't be true, so I'll remove that in v2.
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 :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Daniel P. Berrange -
Martin Kletzander