pci/iov: VFs are never multifunction

Per the SR-IOV spec rev 1.1:

3.4.1.9 Header Type (Offset 0Eh)

"... For VFs, this register must be RO Zero."

Unfortunately some devices get this wrong, ex. Emulex OneConnect 10Gb NIC. When they do it makes us handle ACS testing and therefore IOMMU groups as if they were actual multifunction devices and require ACS capabilities to make sure there's no peer-to-peer between functions. VFs are never traditional multifunction devices, so simply clear this bit before we get any further into setup.

Signed-off-by Alex Williamson <[email protected]>

---
 drivers/pci/iov.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/iov.c +1 -0

index 1fe2d6f..e2fbb67 100644
@@ -84,6 +84,7 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
        virtfn->dev.parent = dev->dev.parent;
        virtfn->physfn = pci_dev_get(dev);
        virtfn->is_virtfn = 1;
+       virtfn->multifunction = 0;
 
        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
                res = dev->resource + PCI_IOV_RESOURCES + i;

Quoted 4 lines · Alex Williamson

Per the SR-IOV spec rev 1.1:

3.4.1.9 Header Type (Offset 0Eh)

Quoted 8 lines

"... For VFs, this register must be RO Zero."

Unfortunately some devices get this wrong, ex. Emulex OneConnect 10Gb NIC. When they do it makes us handle ACS testing and therefore IOMMU groups as if they were actual multifunction devices and require ACS capabilities to make sure there's no peer-to-peer between functions. VFs are never traditional multifunction devices, so simply clear this bit before we get any further into setup.

This seems reasonable. Do you have "lspci -vvxxxx" output for this device? I'd like to save it for future reference.

Quoted 18 lines

Signed-off-by Alex Williamson <[email protected]>

---
 drivers/pci/iov.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/iov.c +1 -0

index 1fe2d6f..e2fbb67 100644
@@ -84,6 +84,7 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
        virtfn->dev.parent = dev->dev.parent;
        virtfn->physfn = pci_dev_get(dev);
        virtfn->is_virtfn = 1;
+       virtfn->multifunction = 0;
 
        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
                res = dev->resource + PCI_IOV_RESOURCES + i;

Quoted 15 lines · Bjorn Helgaas

Quoted 12 lines · Alex Williamson

Per the SR-IOV spec rev 1.1:

3.4.1.9 Header Type (Offset 0Eh)

"... For VFs, this register must be RO Zero."

Unfortunately some devices get this wrong, ex. Emulex OneConnect 10Gb NIC. When they do it makes us handle ACS testing and therefore IOMMU groups as if they were actual multifunction devices and require ACS capabilities to make sure there's no peer-to-peer between functions. VFs are never traditional multifunction devices, so simply clear this bit before we get any further into setup.

Quoted 2 lines

This seems reasonable. Do you have "lspci -vvxxxx" output for this device? I'd like to save it for future reference.

Sure, here's a VF:

09:04.0 Ethernet controller: Emulex Corporation OneConnect 10Gb NIC (be3) (rev 01)
Subsystem: Emulex Corporation Device e722

Quoted 16 lines · Alex Williamson

Quoted 15 lines · Bjorn Helgaas

Quoted 12 lines · Alex Williamson

Per the SR-IOV spec rev 1.1:

3.4.1.9 Header Type (Offset 0Eh)

"... For VFs, this register must be RO Zero."

Unfortunately some devices get this wrong, ex. Emulex OneConnect 10Gb NIC. When they do it makes us handle ACS testing and therefore IOMMU groups as if they were actual multifunction devices and require ACS capabilities to make sure there's no peer-to-peer between functions. VFs are never traditional multifunction devices, so simply clear this bit before we get any further into setup.

Quoted 7 lines

Quoted 2 lines

This seems reasonable. Do you have "lspci -vvxxxx" output for this device? I'd like to save it for future reference.

Sure, here's a VF:

09:04.0 Ethernet controller: Emulex Corporation OneConnect 10Gb NIC (be3) (rev 01)
Subsystem: Emulex Corporation Device e722

Thanks! I put this in
https://bugzilla.kernel.org/show_bug.cgi?id=68431, and I'll add a reference to the changelog.

But I wonder if we can make this slightly more generic by doing something like this in pci_setup_device():

  dev->multifunction = (PCI_FUNC(dev->devfn) == 0) && (hdr_type & 0x80);

That's basically what lspci does in pci_generic_scan_bus(), and section 3.2.2.3.4 of the PCI 3.0 spec sort of implies that we should only look at the bit 7 of the header type for function 0:

If a single function device is detected (i.e., bit 7 in the Header Type register of function 0 is 0), no more functions for that Device Number will be checked. If a multi-function device is detected (i.e., bit 7 in the Header Type register of function 0 is 1), then all remaining Function Numbers will be checked.

Quoted 39 lines · Bjorn Helgaas

Quoted 16 lines · Alex Williamson

Quoted 15 lines · Bjorn Helgaas

Quoted 12 lines · Alex Williamson

Per the SR-IOV spec rev 1.1:

3.4.1.9 Header Type (Offset 0Eh)

"... For VFs, this register must be RO Zero."

Unfortunately some devices get this wrong, ex. Emulex OneConnect 10Gb NIC. When they do it makes us handle ACS testing and therefore IOMMU groups as if they were actual multifunction devices and require ACS capabilities to make sure there's no peer-to-peer between functions. VFs are never traditional multifunction devices, so simply clear this bit before we get any further into setup.

Quoted 7 lines

Quoted 2 lines

This seems reasonable. Do you have "lspci -vvxxxx" output for this device? I'd like to save it for future reference.

Sure, here's a VF:

09:04.0 Ethernet controller: Emulex Corporation OneConnect 10Gb NIC (be3) (rev 01)
Subsystem: Emulex Corporation Device e722

Thanks! I put this in
https://bugzilla.kernel.org/show_bug.cgi?id=68431, and I'll add a reference to the changelog.

But I wonder if we can make this slightly more generic by doing something like this in pci_setup_device():

  dev->multifunction = (PCI_FUNC(dev->devfn) == 0) && (hdr_type & 0x80);

That's basically what lspci does in pci_generic_scan_bus(), and section 3.2.2.3.4 of the PCI 3.0 spec sort of implies that we should only look at the bit 7 of the header type for function 0:

Quoted 5 lines

If a single function device is detected (i.e., bit 7 in the Header Type register of function 0 is 0), no more functions for that Device Number will be checked. If a multi-function device is detected (i.e., bit 7 in the Header Type register of function 0 is 1), then all remaining Function Numbers will be checked.

We could do that and rely only on pci_scan_slot() to set multifunction=1 for the other functions, but that doesn't completely solve this problem. VFs can occupy function zero and the example device would still set multifunction with that test. Thanks,

From that path, it *looks* like it doesn't really matter whether

[+to Nishank]

Quoted 46 lines · Alex Williamson

Quoted 39 lines · Bjorn Helgaas

Quoted 16 lines · Alex Williamson

Quoted 15 lines · Bjorn Helgaas

Quoted 12 lines · Alex Williamson

Per the SR-IOV spec rev 1.1:

3.4.1.9 Header Type (Offset 0Eh)

"... For VFs, this register must be RO Zero."

Unfortunately some devices get this wrong, ex. Emulex OneConnect 10Gb NIC. When they do it makes us handle ACS testing and therefore IOMMU groups as if they were actual multifunction devices and require ACS capabilities to make sure there's no peer-to-peer between functions. VFs are never traditional multifunction devices, so simply clear this bit before we get any further into setup.

Quoted 7 lines

Quoted 2 lines

This seems reasonable. Do you have "lspci -vvxxxx" output for this device? I'd like to save it for future reference.

Sure, here's a VF:

09:04.0 Ethernet controller: Emulex Corporation OneConnect 10Gb NIC (be3) (rev 01)
Subsystem: Emulex Corporation Device e722

Thanks! I put this in
https://bugzilla.kernel.org/show_bug.cgi?id=68431, and I'll add a reference to the changelog.

But I wonder if we can make this slightly more generic by doing something like this in pci_setup_device():

  dev->multifunction = (PCI_FUNC(dev->devfn) == 0) && (hdr_type & 0x80);

That's basically what lspci does in pci_generic_scan_bus(), and section 3.2.2.3.4 of the PCI 3.0 spec sort of implies that we should only look at the bit 7 of the header type for function 0:

Quoted 5 lines

If a single function device is detected (i.e., bit 7 in the Header Type register of function 0 is 0), no more functions for that Device Number will be checked. If a multi-function device is detected (i.e., bit 7 in the Header Type register of function 0 is 1), then all remaining Function Numbers will be checked.

Quoted 4 lines

We could do that and rely only on pci_scan_slot() to set multifunction=1 for the other functions, but that doesn't completely solve this problem. VFs can occupy function zero and the example device would still set multifunction with that test. Thanks,

Duh, it would help if I actually paid attention to your lspci output...

The reason I'm thinking about this is that virtfn_add() is only used when we enable SR-IOV. If we clear dev->multifunction there, we only end up with the correct value if we start with SR-IOV disabled, and then enable it.

If SR-IOV were enabled by the firmware before Linux boots, we wouldn't go through the virtfn_add() path, and dev->multifunction might still be wrong.

I'm pretty sure Nishank said there were Cisco boxes that enable SR-IOV in the firmware, but I don't know how that works. It looks like we would disable SR-IOV during enumeration in the path below:

pci_scan_slot
pci_scan_single_device
pci_device_add
pci_init_capabilities
pci_iov_init
sriov_init
pci_read_config_word(dev, pos + PCI_SRIOV_CTRL, &ctrl) if (ctrl & PCI_SRIOV_CTRL_VFE)
pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, 0)

From that path, it *looks* like it doesn't really matter whether SR-IOV is enabled at handoff, because we disable it anyway.

So I'm not sure if I misunderstood Nishank or what. I think it would be cool if we could enumerate previously-enabled VFs, but maybe there are other issues that would make that impossible.

Per the SR-IOV spec rev 1.1:

3.4.1.9 Header Type (Offset 0Eh)

"... For VFs, this register must be RO Zero."

Unfortunately some devices get this wrong, ex. Emulex OneConnect 10Gb NIC. When they do it makes us handle ACS testing and therefore IOMMU groups as if they were actual multifunction devices and require ACS capabilities to make sure there's no peer-to-peer between functions. VFs are never traditional multifunction devices, so simply clear this bit before we get any further into setup.

Signed-off-by Alex Williamson <[email protected]>

---
 drivers/pci/iov.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/iov.c +1 -0

index 1fe2d6f..e2fbb67 100644
@@ -84,6 +84,7 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
        virtfn->dev.parent = dev->dev.parent;
        virtfn->physfn = pci_dev_get(dev);
        virtfn->is_virtfn = 1;
+       virtfn->multifunction = 0;
 
        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
                res = dev->resource + PCI_IOV_RESOURCES + i;