PCI: Linux kernel SR-IOV support

drivers/pci/Makefile +6 -0

+static void sriov_release(struct pci_dev *dev)
+{
+	if (dev == dev->sriov->pdev)
+		mutex_destroy(&dev->sriov->lock);
+	else
+		pci_dev_put(dev->sriov->pdev);

drivers/pci/Kconfig config HT_IRQ · line 66

Quoted 4 lines · Yu Zhao

drivers/pci/Kconfig +4 -0

+config PCI_IOV
+	bool "PCI IOV support"
+	depends on PCI
+	select PCI_MSI

My understanding is that having 'select' of a config symbol that the user can choose is bad. I think we should probably make this 'depends on PCI_MSI'.

PCI MSI can also be disabled at runtime (and Fedora do by default). Since SR-IOV really does require MSI, we need to put in a runtime check to see if pci_msi_enabled() is false.

We don't depend on PCIEPORTBUS (a horribly named symbol). Should we? SR-IOV is only supported for PCI Express machines. I'm not sure of the right answer here, but I thought I should raise the question.

drivers/pci/Kconfig config HT_IRQ · line 74

Quoted 7 lines

drivers/pci/Kconfig +7 -0

+	help
+	  PCI-SIG I/O Virtualization (IOV) Specifications support.
+	  Single Root IOV: allows the Physical Function driver to enable
+	  the hardware capability, so the Virtual Function is accessible
+	  via the PCI Configuration Space using its own Bus, Device and
+	  Function Numbers. Each Virtual Function also has the PCI Memory
+	  Space to map the device specific register set.

I'm not convinced this is the most helpful we could be to the user who's configuring their own kernel. How about something like this? (Randy, I particularly look to you to make my prose less turgid).

help
IO Virtualisation is a PCI feature supported by some devices

which allows you to create virtual PCI devices and assign them to guest OSes. This option needs to be selected in the host or Dom0 kernel, but does not need to be selected in the guest or DomU kernel. If you don't know whether your hardware supports it, you can check by using lspci to look for the SR-IOV capability.

If you have no idea what any of that means, it is safe to answer 'N' here.

Quoted 9 lines · Matthew Wilcox

On Fri, Feb 20, 2009 at 02:54:42PM +0800, Yu Zhao wrote:
PCI MSI can also be disabled at runtime (and Fedora do by default). Since SR-IOV really does require MSI, we need to put in a runtime check to see if pci_msi_enabled() is false.

We don't depend on PCIEPORTBUS (a horribly named symbol). Should we? SR-IOV is only supported for PCI Express machines. I'm not sure of the right answer here, but I thought I should raise the question.

Quoted 7 lines

drivers/pci/Kconfig config HT_IRQ · line 74

Quoted 7 lines

drivers/pci/Kconfig +7 -0

+	help
+	  PCI-SIG I/O Virtualization (IOV) Specifications support.
+	  Single Root IOV: allows the Physical Function driver to enable
+	  the hardware capability, so the Virtual Function is accessible
+	  via the PCI Configuration Space using its own Bus, Device and
+	  Function Numbers. Each Virtual Function also has the PCI Memory
+	  Space to map the device specific register set.

Too spec. and implementation specific for users.

Quoted 13 lines

I'm not convinced this is the most helpful we could be to the user who's configuring their own kernel. How about something like this? (Randy, I particularly look to you to make my prose less turgid).

help
IO Virtualisation is a PCI feature supported by some devices z ;) which allows you to create virtual PCI devices and assign them to guest OSes. This option needs to be selected in the host or Dom0 kernel, but does not need to be selected in the guest or DomU kernel. If you don't know whether your hardware supports it, you can check by using lspci to look for the SR-IOV
capability.

Quoted 2 lines

If you have no idea what any of that means, it is safe to answer 'N' here.

That's certainly more readable and user-friendly. I don't know what else it needs. Looks good to me.

~Randy

drivers/pci/iov.c line 51

Quoted 9 lines · Yu Zhao

drivers/pci/iov.c new file +9 -0

+	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
+		if (pdev->sriov)
+			break;
+	if (list_empty(&dev->bus->devices) || !pdev->sriov)
+		pdev = NULL;
+	ctrl = 0;
+	if (!pdev && pci_ari_enabled(dev->bus))
+		ctrl |= PCI_SRIOV_CTRL_ARI;
+

I don't like this loop. At the end of a list_for_each_entry() loop, pdev will not be pointing at a pci_device, it'll be pointing to some offset from &dev->bus->devices. So checking pdev->sriov at this point is really, really bad. I would prefer to see something like this:

        ctrl = 0;
        list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
                if (pdev->sriov)
                        goto ari_enabled;
        }

        if (pci_ari_enabled(dev->bus))
                ctrl = PCI_SRIOV_CTRL_ARI;
 ari_enabled:
        pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);

Quoted 16 lines · Matthew Wilcox

drivers/pci/iov.c line 51

Quoted 9 lines · Yu Zhao

drivers/pci/iov.c new file +9 -0

+	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
+		if (pdev->sriov)
+			break;
+	if (list_empty(&dev->bus->devices) || !pdev->sriov)
+		pdev = NULL;
+	ctrl = 0;
+	if (!pdev && pci_ari_enabled(dev->bus))
+		ctrl |= PCI_SRIOV_CTRL_ARI;
+

I don't like this loop. At the end of a list_for_each_entry() loop, pdev will not be pointing at a pci_device, it'll be pointing to some offset from &dev->bus->devices. So checking pdev->sriov at this point is really, really bad. I would prefer to see something like this:

Quoted 10 lines

        ctrl = 0;
        list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
                if (pdev->sriov)
                        goto ari_enabled;
        }

        if (pci_ari_enabled(dev->bus))
                ctrl = PCI_SRIOV_CTRL_ARI;
 ari_enabled:
        pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);

No, please use bus_for_each_dev() instead, or bus_find_device(), don't walk the bus list by hand. I'm kind of surprised that even builds. Hm, in looking at the 2.6.29-rc kernels, I notice it will not even build at all, you are now forced to use those functions, which is good.

Quoted 28 lines · Greg KH

Quoted 16 lines · Matthew Wilcox

drivers/pci/iov.c line 51

Quoted 9 lines · Yu Zhao

drivers/pci/iov.c new file +9 -0

+	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
+		if (pdev->sriov)
+			break;
+	if (list_empty(&dev->bus->devices) || !pdev->sriov)
+		pdev = NULL;
+	ctrl = 0;
+	if (!pdev && pci_ari_enabled(dev->bus))
+		ctrl |= PCI_SRIOV_CTRL_ARI;
+

I don't like this loop. At the end of a list_for_each_entry() loop, pdev will not be pointing at a pci_device, it'll be pointing to some offset from &dev->bus->devices. So checking pdev->sriov at this point is really, really bad. I would prefer to see something like this:

Quoted 10 lines

        ctrl = 0;
        list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
                if (pdev->sriov)
                        goto ari_enabled;
        }

        if (pci_ari_enabled(dev->bus))
                ctrl = PCI_SRIOV_CTRL_ARI;
 ari_enabled:
        pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);

Quoted 4 lines

No, please use bus_for_each_dev() instead, or bus_find_device(), don't walk the bus list by hand. I'm kind of surprised that even builds. Hm, in looking at the 2.6.29-rc kernels, I notice it will not even build at all, you are now forced to use those functions, which is good.

The devices haven't been added at this time, so we can't use bus_for_each_dev(). I guess that's why the `bus->devices' exists, and actually pci_bus_add_devices() walks the bus list same way to retrieve the devices and add them.

drivers/pci/Kconfig config HT_IRQ · line 66

Quoted 4 lines · Yu Zhao

drivers/pci/Kconfig +4 -0

+config PCI_IOV
+	bool "PCI IOV support"
+	depends on PCI
+	select PCI_MSI

My understanding is that having 'select' of a config symbol that the user can choose is bad. I think we should probably make this 'depends on PCI_MSI'.

PCI MSI can also be disabled at runtime (and Fedora do by default). Since SR-IOV really does require MSI, we need to put in a runtime check to see if pci_msi_enabled() is false.

We don't depend on PCIEPORTBUS (a horribly named symbol). Should we? SR-IOV is only supported for PCI Express machines. I'm not sure of the right answer here, but I thought I should raise the question.

drivers/pci/Kconfig config HT_IRQ · line 67

Quoted 1 line

drivers/pci/Kconfig +1 -0

+	default n

You don't need this -- the default default is n ;-)

drivers/pci/Kconfig config HT_IRQ · line 74

Quoted 7 lines

drivers/pci/Kconfig +7 -0

+	help
+	  PCI-SIG I/O Virtualization (IOV) Specifications support.
+	  Single Root IOV: allows the Physical Function driver to enable
+	  the hardware capability, so the Virtual Function is accessible
+	  via the PCI Configuration Space using its own Bus, Device and
+	  Function Numbers. Each Virtual Function also has the PCI Memory
+	  Space to map the device specific register set.

I'm not convinced this is the most helpful we could be to the user who's configuring their own kernel. How about something like this? (Randy, I particularly look to you to make my prose less turgid).

help
IO Virtualisation is a PCI feature supported by some devices which allows you to create virtual PCI devices and assign them to guest OSes. This option needs to be selected in the host or Dom0 kernel, but does not need to be selected in the guest or DomU kernel. If you don't know whether your hardware supports it, you can check by using lspci to look for the SR-IOV capability.

If you have no idea what any of that means, it is safe to answer 'N' here.

drivers/pci/Makefile obj-$(CONFIG_DMAR) += dmar.o iova.o intel-iommu.o · line 33

Quoted 10 lines

drivers/pci/Makefile +2 -0

index 3d07ce2..ba99282 100644
@@ -29,6 +29,9 @@ obj-$(CONFIG_DMAR) += dmar.o iova.o intel-iommu.o
 
 obj-$(CONFIG_INTR_REMAP) += dmar.o intr_remapping.o
 
+# PCI IOV support
+obj-$(CONFIG_PCI_IOV) += iov.o

I see you're following the gerneal style in this file, but the comments really add no value. I should send a patch to take out the existing ones.

drivers/pci/Makefile obj-$(CONFIG_DMAR) += dmar.o iova.o intel-iommu.o

Quoted 9 lines

drivers/pci/Makefile +9 -0

+	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
+		if (pdev->sriov)
+			break;
+	if (list_empty(&dev->bus->devices) || !pdev->sriov)
+		pdev = NULL;
+	ctrl = 0;
+	if (!pdev && pci_ari_enabled(dev->bus))
+		ctrl |= PCI_SRIOV_CTRL_ARI;
+

I don't like this loop. At the end of a list_for_each_entry() loop, pdev will not be pointing at a pci_device, it'll be pointing to some offset from &dev->bus->devices. So checking pdev->sriov at this point is really, really bad. I would prefer to see something like this:

        ctrl = 0;
        list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
                if (pdev->sriov)
                        goto ari_enabled;
        }

        if (pci_ari_enabled(dev->bus))
                ctrl = PCI_SRIOV_CTRL_ARI;
 ari_enabled:
        pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);

drivers/pci/Makefile

Quoted 6 lines

drivers/pci/Makefile +6 -0

+	if (pdev)
+		iov->pdev = pci_dev_get(pdev);
+	else {
+		iov->pdev = dev;
+		mutex_init(&iov->lock);
+	}

Now I'm confused. Why don't we need to init the mutex if there's another device on the same bus which also has an iov capability?

Quoted 6 lines

drivers/pci/Makefile +6 -0

+static void sriov_release(struct pci_dev *dev)
+{
+	if (dev == dev->sriov->pdev)
+		mutex_destroy(&dev->sriov->lock);
+	else
+		pci_dev_put(dev->sriov->pdev);

drivers/pci/Makefile

Quoted 10 lines

drivers/pci/Makefile +9 -0

+
+	kfree(dev->sriov);
+	dev->sriov = NULL;
+}
 
+void pci_iov_release(struct pci_dev *dev)
+{
+	if (dev->sriov)
+		sriov_release(dev);
+}

This seems to be a bit of a design pattern with you, and I'm not quite sure why you do it like this instead of just doing:

void pci_iov_release(struct pci_dev *dev)

{
	if (!dev->sriov)
		return;
	[...]
}

Signed-off-by Yu Zhao <[email protected]>

drivers/pci/iov.c +2 -0

+static void sriov_restore_state(struct pci_dev *dev)
+{

drivers/pci/iov.c +2 -0

+}
+

drivers/pci/iov.c line 25

Quoted 9 lines · Yu Zhao

drivers/pci/iov.c +9 -0

+static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn)
+{
+	u16 bdf;
+
+	bdf = (dev->bus->number << 8) + dev->devfn +
+	      dev->sriov->offset + dev->sriov->stride * id;
+	*busnr = bdf >> 8;
+	*devfn = bdf & 0xff;
+}

I find the interface here a bit clunky -- a function returning void while having two OUT parameters. How about this variation on the theme (viewers are encouraged to come up with their own preferred implementations and interfaces):

static inline __pure u16 virtfn_bdf(struct pci_dev *dev, int id)

{
	return (dev->bus->number << 8) + dev->devfn + dev->sriov->offset +
		dev->sriov->stride * id;
}

#define VIRT_BUS(dev, id) (virtfn_bdf(dev, id) >> 8)
#define VIRT_DEVFN(dev, id) (virtfn_bdf(dev, id) & 0xff)

We rely on GCC to do CSE and not actually invoke virtfn_bdf more than once.

Quoted 21 lines · Matthew Wilcox

drivers/pci/iov.c line 25

Quoted 9 lines · Yu Zhao

drivers/pci/iov.c +9 -0

+static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn)
+{
+	u16 bdf;
+
+	bdf = (dev->bus->number << 8) + dev->devfn +
+	      dev->sriov->offset + dev->sriov->stride * id;
+	*busnr = bdf >> 8;
+	*devfn = bdf & 0xff;
+}

I find the interface here a bit clunky -- a function returning void while having two OUT parameters. How about this variation on the theme (viewers are encouraged to come up with their own preferred implementations and interfaces):

static inline __pure u16 virtfn_bdf(struct pci_dev *dev, int id)

{
	return (dev->bus->number << 8) + dev->devfn + dev->sriov->offset +
		dev->sriov->stride * id;
}

Quoted 5 lines

#define VIRT_BUS(dev, id) (virtfn_bdf(dev, id) >> 8)
#define VIRT_DEVFN(dev, id) (virtfn_bdf(dev, id) & 0xff)

We rely on GCC to do CSE and not actually invoke virtfn_bdf more than once.

Yes, that's a good idea. Will replace that function with macros.

Quoted 9 lines

Quoted 9 lines

static inline __pure u16 virtfn_bdf(struct pci_dev *dev, int id)

{
	return (dev->bus->number << 8) + dev->devfn + dev->sriov->offset +
		dev->sriov->stride * id;
}

#define VIRT_BUS(dev, id) (virtfn_bdf(dev, id) >> 8)
#define VIRT_DEVFN(dev, id) (virtfn_bdf(dev, id) & 0xff)

Quoted 4 lines

Quoted 2 lines

We rely on GCC to do CSE and not actually invoke virtfn_bdf more than once.

Yes, that's a good idea. Will replace that function with macros.

That's the opposite of most changes lately. I.e., functions (with typechecking) are preferred.

drivers/pci/iov.c +4 -0

+	virtfn->sysdata = dev->bus->sysdata;
+	virtfn->dev.parent = dev->dev.parent;
+	virtfn->dev.bus = dev->dev.bus;
+	virtfn->devfn = devfn;

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 251

Quoted 4 lines · Yu Zhao

drivers/pci/iov.c +4 -0

+	if (nres != iov->nres) {
+		dev_err(&dev->dev, "no enough MMIO for SR-IOV\n");
+		return -ENOMEM;
+	}

Randy, can you help us out with better wording here?

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 258

Quoted 1 line

drivers/pci/iov.c +1 -0

+		dev_err(&dev->dev, "no enough bus range for SR-IOV\n");

and here.

Quoted 6 lines

drivers/pci/iov.c +6 -0

+	if (iov->link != dev->devfn) {
+		rc = -ENODEV;
+		list_for_each_entry(link, &dev->bus->devices, bus_list) {
+			if (link->sriov && link->devfn == iov->link)
+				rc = sysfs_create_link(&iov->dev.kobj,
+						&link->dev.kobj, "dep_link");

Quoted 6 lines · Matthew Wilcox

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 251

Quoted 4 lines · Yu Zhao

drivers/pci/iov.c +4 -0

+	if (nres != iov->nres) {
+		dev_err(&dev->dev, "no enough MMIO for SR-IOV\n");
+		return -ENOMEM;
+	}

"not enough MMIO BARs for SR-IOV"
or
"not enough MMIO resources for SR-IOV"
or
"too few MMIO BARs for SR-IOV"
?

Quoted 5 lines

Randy, can you help us out with better wording here?

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 258

Quoted 1 line

drivers/pci/iov.c +1 -0

+		dev_err(&dev->dev, "no enough bus range for SR-IOV\n");

and here.

"SR-IOV: bus number too large"
or
"SR-IOV: bus number out of range"
or
"SR-IOV: cannot allocate valid bus number"
?

Quoted 6 lines

Quoted 6 lines

drivers/pci/iov.c +6 -0

+	if (iov->link != dev->devfn) {
+		rc = -ENODEV;
+		list_for_each_entry(link, &dev->bus->devices, bus_list) {
+			if (link->sriov && link->devfn == iov->link)
+				rc = sysfs_create_link(&iov->dev.kobj,
+						&link->dev.kobj, "dep_link");

Quoted 4 lines · Yu Zhao

drivers/pci/iov.c +4 -0

+	virtfn->sysdata = dev->bus->sysdata;
+	virtfn->dev.parent = dev->dev.parent;
+	virtfn->dev.bus = dev->dev.bus;
+	virtfn->devfn = devfn;

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 107

Quoted 10 lines

drivers/pci/iov.c +10 -0

+	virtfn->hdr_type = PCI_HEADER_TYPE_NORMAL;
+	virtfn->cfg_size = PCI_CFG_SPACE_EXP_SIZE;
+	virtfn->error_state = pci_channel_io_normal;
+	virtfn->current_state = PCI_UNKNOWN;
+	virtfn->is_pcie = 1;
+	virtfn->pcie_type = PCI_EXP_TYPE_ENDPOINT;
+	virtfn->dma_mask = 0xffffffff;
+	virtfn->vendor = dev->vendor;
+	virtfn->subsystem_vendor = dev->subsystem_vendor;
+	virtfn->class = dev->class;

There seems to be a certain amount of commonality between this and pci_scan_device(). Have you considered trying to make a common helper function, or does it not work out well?

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 134

Quoted 1 line

drivers/pci/iov.c +1 -0

+	pci_device_add(virtfn, virtfn->bus);

Greg is probably going to ding you here for adding the device, then creating the symlinks. I believe it's now best practice to create the symlinks first, so there's no window where userspace can get confused.

Quoted 10 lines · Yu Zhao

drivers/pci/iov.c +10 -0

+	virtfn->sysdata = dev->bus->sysdata;
+	virtfn->dev.parent = dev->dev.parent;
+	virtfn->dev.bus = dev->dev.bus;
+	virtfn->devfn = devfn;
+	virtfn->hdr_type = PCI_HEADER_TYPE_NORMAL;
+	virtfn->cfg_size = PCI_CFG_SPACE_EXP_SIZE;
+	virtfn->error_state = pci_channel_io_normal;
+	virtfn->current_state = PCI_UNKNOWN;
+	virtfn->is_pcie = 1;
+	virtfn->pcie_type = PCI_EXP_TYPE_ENDPOINT;

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 107

Quoted 4 lines

drivers/pci/iov.c +4 -0

+	virtfn->dma_mask = 0xffffffff;
+	virtfn->vendor = dev->vendor;
+	virtfn->subsystem_vendor = dev->subsystem_vendor;
+	virtfn->class = dev->class;

There seems to be a certain amount of commonality between this and pci_scan_device(). Have you considered trying to make a common helper function, or does it not work out well?

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 134

Quoted 1 line

drivers/pci/iov.c +1 -0

+	pci_device_add(virtfn, virtfn->bus);

Greg is probably going to ding you here for adding the device, then creating the symlinks. I believe it's now best practice to create the symlinks first, so there's no window where userspace can get confused.

Review

Quoted 1 line

(quoted diff) +1 -0

+	mutex_unlock(&iov->pdev->sriov->lock);

I question the existance of this mutex now. What's it protecting?

Aren't we going to be implicitly protected by virtue of the Physical Function device driver being the only one calling this function, and the driver will be calling it from the ->probe routine which is not called simultaneously for the same device.

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 142

Quoted 6 lines

drivers/pci/iov.c +6 -0

+	virtfn->physfn = pci_dev_get(dev);
+
+	rc = pci_bus_add_device(virtfn);
+	if (rc)
+		goto failed1;
+	sprintf(buf, "%d", id);

%u, perhaps? And maybe 'id' should always be unsigned? Just a thought.

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 148

Quoted 6 lines

drivers/pci/iov.c +6 -0

+	rc = sysfs_create_link(&iov->dev.kobj, &virtfn->dev.kobj, buf);
+	if (rc)
+		goto failed1;
+	rc = sysfs_create_link(&virtfn->dev.kobj, &dev->dev.kobj, "physfn");
+	if (rc)
+		goto failed2;

I'm glad to see these symlinks documented in later patches!

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 247

Quoted 7 lines

drivers/pci/iov.c +7 -0

+	nres = 0;
+	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
+		res = dev->resource + PCI_SRIOV_RESOURCES + i;
+		if (!res->parent)
+			continue;
+		nres++;
+	}

Can't this be written more simply as:

	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
		res = dev->resource + PCI_SRIOV_RESOURCES + i;
		if (res->parent)
			nres++;
	}

?

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 251

Quoted 4 lines

drivers/pci/iov.c +4 -0

+	if (nres != iov->nres) {
+		dev_err(&dev->dev, "no enough MMIO for SR-IOV\n");
+		return -ENOMEM;
+	}

Randy, can you help us out with better wording here?

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 258

Quoted 1 line

drivers/pci/iov.c +1 -0

+		dev_err(&dev->dev, "no enough bus range for SR-IOV\n");

and here.

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 275

Quoted 6 lines

drivers/pci/iov.c +6 -0

+	if (iov->link != dev->devfn) {
+		rc = -ENODEV;
+		list_for_each_entry(link, &dev->bus->devices, bus_list) {
+			if (link->sriov && link->devfn == iov->link)
+				rc = sysfs_create_link(&iov->dev.kobj,
+						&link->dev.kobj, "dep_link");

I skipped to the end and read patch 7/7 and I still don't understand what dep_link is for. Can you explain please? In particular, how is it different from physfn?

Quoted 5 lines

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 134

Quoted 1 line

drivers/pci/iov.c +1 -0

+	pci_device_add(virtfn, virtfn->bus);

Greg is probably going to ding you here for adding the device, then creating the symlinks. I believe it's now best practice to create the symlinks first, so there's no window where userspace can get confused.

Yes, but unfortunately we can't create links before adding a device. I double checked device_add(), there is no place for those links to be created before it sends uevent. So for now, we have to trigger another uevent for those links.

Quoted 10 lines · Yu Zhao

Quoted 5 lines

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 134

Quoted 1 line

drivers/pci/iov.c +1 -0

+	pci_device_add(virtfn, virtfn->bus);

Greg is probably going to ding you here for adding the device, then creating the symlinks. I believe it's now best practice to create the symlinks first, so there's no window where userspace can get confused.

Yes, but unfortunately we can't create links before adding a device. I double checked device_add(), there is no place for those links to be created before it sends uevent. So for now, we have to trigger another uevent for those links.

What exactly are you trying to do with a symlink here that you need to do it this way? I vaguely remember you mentioning this in the past, but I thought you had dropped the symlinks after our conversation about this very problem.

Quoted 12 lines · Greg KH

Quoted 10 lines · Yu Zhao

Quoted 5 lines

drivers/pci/iov.c static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8 *devfn) · line 134

Quoted 1 line

drivers/pci/iov.c +1 -0

+	pci_device_add(virtfn, virtfn->bus);

Greg is probably going to ding you here for adding the device, then creating the symlinks. I believe it's now best practice to create the symlinks first, so there's no window where userspace can get confused.

Yes, but unfortunately we can't create links before adding a device. I double checked device_add(), there is no place for those links to be created before it sends uevent. So for now, we have to trigger another uevent for those links.

Quoted 4 lines

What exactly are you trying to do with a symlink here that you need to do it this way? I vaguely remember you mentioning this in the past, but I thought you had dropped the symlinks after our conversation about this very problem.

I'd like to create some symlinks to reflect the relationship between Physical Function and its associated Virtual Functions. The Physical Function is like a master device that controls the allocation of its Virtual Functions and owns the device physical resource. The Virtual Functions are like slave devices of the Physical Function. For example, if 01:00.0 is a Physical Function and 02:00.0 is a Virtual Function associated with 01:00.0. Then the symlinks (virtfnN and physfn) would look like:

  $ ls -l /sys/bus/pci/devices/0000:01:00.0/
  ...
  ...  virtfn0 -> ../0000:02:00.0
  ...  virtfn1 -> ../0000:02:00.1
  ...  virtfn2 -> ../0000:02:00.2
  ...

  $ ls -l /sys/bus/pci/devices/0000:02:00.0/
  ...
  ... physfn -> ../0000:01:00.0
  ...

This is very useful for userspace applications, both KVM and Xen need to know this kind of relationship so they can request the permission from a Physical Function before using its associated Virtual Functions.

drivers/pci/iov.c +15 -0

+	if (!(status & PCI_SRIOV_STATUS_VFM))
+		return 0;
+
+	schedule_work(&iov->mtask);
+
+	return 1;
+}
 
+/**
+ * pci_sriov_migration - notify SR-IOV core of Virtual Function Migration
+ * @dev: the PCI device
+ *
+ * Returns IRQ_HANDLED if the IRQ is handled, or IRQ_NONE if not.
+ *
+ * Physical Function driver is responsible to register IRQ handler using
+ * VF Migration Interrupt Message Number, and call this function when the

drivers/pci/iov.c static void sriov_release_dev(struct device *dev) · line 220

Quoted 12 lines · Yu Zhao

drivers/pci/iov.c +12 -0

+static int sriov_migration(struct pci_dev *dev)
+{
+	u16 status;
+	struct pci_sriov *iov = dev->sriov;
+
+	if (!iov->nr_virtfn)
+		return 0;
+
+	if (!(iov->cap & PCI_SRIOV_CAP_VFM))
+		return 0;
+
+	pci_read_config_word(iov->self, iov->pos + PCI_SRIOV_STATUS, &status);

You passed in dev here, you don't need to use iov->self, right?

Quoted 16 lines

drivers/pci/iov.c +15 -0

+	if (!(status & PCI_SRIOV_STATUS_VFM))
+		return 0;
+
+	schedule_work(&iov->mtask);
+
+	return 1;
+}
 
+/**
+ * pci_sriov_migration - notify SR-IOV core of Virtual Function Migration
+ * @dev: the PCI device
+ *
+ * Returns IRQ_HANDLED if the IRQ is handled, or IRQ_NONE if not.
+ *
+ * Physical Function driver is responsible to register IRQ handler using
+ * VF Migration Interrupt Message Number, and call this function when the

drivers/pci/iov.c void pci_disable_sriov(struct pci_dev *dev) · line 711

Quoted 10 lines

drivers/pci/iov.c +10 -0

+ * interrupt is generated by the hardware.
+ */
+irqreturn_t pci_sriov_migration(struct pci_dev *dev)
+{
+	if (!dev->sriov)
+		return IRQ_NONE;
+
+	return sriov_migration(dev) ? IRQ_HANDLED : IRQ_NONE;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_migration);

OK, I think I get it -- you've basically written an interrupt handler for the driver to call from its interrupt handler. Am I right in thinking that the reason the driver needs to do the interrupt handler here is because we don't currently have an interface that looks like:

int pci_get_msix_interrupt(struct pci_dev *dev, unsigned vector);

? If so, we should probably add it; I want it for my MSI-X rewrite anyway.

Signed-off-by Yu Zhao <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-pci |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

Documentation/ABI/testing/sysfs-bus-pci +27 -0

index ceddcff..84dc100 100644
@@ -9,3 +9,30 @@ Description:
 		that some devices may have malformatted data.  If the
 		underlying VPD has a writable section then the
 		corresponding section of this file will be writable.
+
+What:		/sys/bus/pci/devices/.../virtfn/N
+Date:		February 2009
+Contact:	Yu Zhao <[email protected]>
+Description:
+		This symbol link appears when hardware supports SR-IOV
+		capability and Physical Function driver has enabled it.
+		The symbol link points to the PCI device sysfs entry of
+		Virtual Function whose index is N (0...MaxVFs-1).
+
+What:		/sys/bus/pci/devices/.../virtfn/dep_link
+Date:		February 2009
+Contact:	Yu Zhao <[email protected]>
+Description:
+		This symbol link appears when hardware supports SR-IOV
+		capability and Physical Function driver has enabled it,
+		and this device has vendor specific dependencies with
+		others. The symbol link points to the PCI device sysfs
+		entry of Physical Function this device depends on.
+
+What:		/sys/bus/pci/devices/.../physfn
+Date:		February 2009
+Contact:	Yu Zhao <[email protected]>
+Description:
+		This symbol link appears when a device is Virtual Function.
+		The symbol link points to the PCI device sysfs entry of
+		Physical Function this device associates with.

Signature · 6 lines

1.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Randy, can you wordsmith this one?

I think I'm starting to understand the difference between physfn and dep_link, but an example would definitely help. It may or may not be appropriate to put it in.

Quoted 5 lines · Yu Zhao

Signed-off-by Yu Zhao <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-pci |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

Quoted 5 lines

Documentation/ABI/testing/sysfs-bus-pci +0 -0

index ceddcff..84dc100 100644

Documentation/ABI/testing/sysfs-bus-pci Description: · line 17

Quoted 9 lines

Documentation/ABI/testing/sysfs-bus-pci +6 -0

@@ -9,3 +9,30 @@ Description:
 		that some devices may have malformatted data.  If the
 		underlying VPD has a writable section then the
 		corresponding section of this file will be writable.
+
+What:		/sys/bus/pci/devices/.../virtfn/N
+Date:		February 2009
+Contact:	Yu Zhao <[email protected]>
+Description:
+		This symbol link appears when hardware supports SR-IOV

Documentation/ABI/testing/sysfs-bus-pci Description: · line 18

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-pci +1 -0

+		capability and Physical Function driver has enabled it.

Documentation/ABI/testing/sysfs-bus-pci Description: · line 19

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-pci +1 -0

+		The symbol link points to the PCI device sysfs entry of

Documentation/ABI/testing/sysfs-bus-pci Description: · line 20

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-pci +1 -0

+		Virtual Function whose index is N (0...MaxVFs-1).

Documentation/ABI/testing/sysfs-bus-pci Description: · line 26

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-pci +6 -0

+
+What:		/sys/bus/pci/devices/.../virtfn/dep_link
+Date:		February 2009
+Contact:	Yu Zhao <[email protected]>
+Description:
+		This symbol link appears when hardware supports SR-IOV

Documentation/ABI/testing/sysfs-bus-pci Description: · line 27

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-pci +1 -0

+		capability and Physical Function driver has enabled it,

Documentation/ABI/testing/sysfs-bus-pci Description: · line 29

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-pci +2 -0

+		and this device has vendor specific dependencies with
+		others. The symbol link points to the PCI device sysfs

Documentation/ABI/testing/sysfs-bus-pci Description: · line 36

Quoted 7 lines

Documentation/ABI/testing/sysfs-bus-pci +7 -0

+		entry of Physical Function this device depends on.
+
+What:		/sys/bus/pci/devices/.../physfn
+Date:		February 2009
+Contact:	Yu Zhao <[email protected]>
+Description:
+		This symbol link appears when a device is Virtual Function.

Documentation/ABI/testing/sysfs-bus-pci Description: · line 37

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-pci +1 -0

+		The symbol link points to the PCI device sysfs entry of

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-pci +1 -0

+		Physical Function this device associates with.

Signed-off-by Yu Zhao <[email protected]>

---
 Documentation/DocBook/kernel-api.tmpl |    1 +
 Documentation/PCI/pci-iov-howto.txt   |   99 +++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/PCI/pci-iov-howto.txt

Documentation/DocBook/kernel-api.tmpl +1 -0

index 5818ff7..506e611 100644
@@ -251,6 +251,7 @@ X!Edrivers/pci/hotplug.c
 -->
 !Edrivers/pci/probe.c
 !Edrivers/pci/rom.c
+!Edrivers/pci/iov.c
      </sect1>
      <sect1><title>PCI Hotplug Support Library</title>
 !Edrivers/pci/hotplug/pci_hotplug_core.c

Documentation/PCI/pci-iov-howto.txt new file +99 -0

new file mode 100644
index 0000000..fc73ef5
@@ -0,0 +1,99 @@
+		PCI Express I/O Virtualization Howto
+		Copyright (C) 2009 Intel Corporation
+		    Yu Zhao <[email protected]>
+
+
+1. Overview
+
+1.1 What is SR-IOV
+
+Single Root I/O Virtualization (SR-IOV) is a PCI Express Extended
+capability which makes one physical device appear as multiple virtual
+devices. The physical device is referred to as Physical Function (PF)
+while the virtual devices are referred to as Virtual Functions (VF).
+Allocation of the VF can be dynamically controlled by the PF via
+registers encapsulated in the capability. By default, this feature is
+not enabled and the PF behaves as traditional PCIe device. Once it's
+turned on, each VF's PCI configuration space can be accessed by its own
+Bus, Device and Function Number (Routing ID). And each VF also has PCI
+Memory Space, which is used to map its register set. VF device driver
+operates on the register set so it can be functional and appear as a
+real existing PCI device.
+
+2. User Guide
+
+2.1 How can I enable SR-IOV capability
+
+The device driver (PF driver) will control the enabling and disabling
+of the capability via API provided by SR-IOV core. If the hardware
+has SR-IOV capability, loading its PF driver would enable it and all
+VFs associated with the PF.
+
+2.2 How can I use the Virtual Functions
+
+The VF is treated as hot-plugged PCI devices in the kernel, so they
+should be able to work in the same way as real PCI devices. The VF
+requires device driver that is same as a normal PCI device's.
+
+3. Developer Guide
+
+3.1 SR-IOV API
+
+To enable SR-IOV capability:
+	int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
+	'nr_virtfn' is number of VFs to be enabled.
+
+To disable SR-IOV capability:
+	void pci_disable_sriov(struct pci_dev *dev);
+
+To notify SR-IOV core of Virtual Function Migration:
+	irqreturn_t pci_sriov_migration(struct pci_dev *dev);
+
+3.2 Usage example
+
+Following piece of code illustrates the usage of the SR-IOV API.
+
+static int __devinit dev_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	pci_enable_sriov(dev, NR_VIRTFN);
+
+	...
+
+	return 0;
+}
+
+static void __devexit dev_remove(struct pci_dev *dev)
+{
+	pci_disable_sriov(dev);
+
+	...
+}
+
+static int dev_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	...
+
+	return 0;
+}
+
+static int dev_resume(struct pci_dev *dev)
+{
+	...
+
+	return 0;
+}
+
+static void dev_shutdown(struct pci_dev *dev)
+{
+	...
+}
+
+static struct pci_driver dev_driver = {
+	.name =		"SR-IOV Physical Function driver",
+	.id_table =	dev_id_table,
+	.probe =	dev_probe,
+	.remove =	__devexit_p(dev_remove),
+	.suspend =	dev_suspend,
+	.resume =	dev_resume,
+	.shutdown =	dev_shutdown,
+};

Signature · 6 lines

1.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greetings,

Following patches are intended to support SR-IOV capability in the Linux kernel. With these patches, people can turn a PCI device with the capability into multiple ones from software perspective, which will benefit KVM and achieve other purposes such as QoS, security, and etc.

Quoted 8 lines · Yu Zhao

Greetings,

Following patches are intended to support SR-IOV capability in the Linux kernel. With these patches, people can turn a PCI device with the capability into multiple ones from software perspective, which will benefit KVM and achieve other purposes such as QoS, security, and etc.

Do those patches allow using a VF on the host (in other words, does the kernel emulate config space accesses)?

Do those patches allow using a VF on the host (in other words, does the kernel emulate config space accesses)?

Quoted 2 lines · Avi Kivity

Do those patches allow using a VF on the host (in other words, does the kernel emulate config space accesses)?

SR-IOV hardware handles config space accesses to virtual functions. No kernel changes needed for that aspect of it.

Quoted 4 lines · Matthew Wilcox

Quoted 2 lines · Avi Kivity

Do those patches allow using a VF on the host (in other words, does the kernel emulate config space accesses)?

Quoted 2 lines

SR-IOV hardware handles config space accesses to virtual functions. No kernel changes needed for that aspect of it.

Patches 2 and 3 of the patchset that enables SR/IOV in kvm [1] suggest that at the config space is only partially implemented.

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/29034

Quoted 8 lines · Avi Kivity

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · Avi Kivity

Do those patches allow using a VF on the host (in other words, does the kernel emulate config space accesses)?

SR-IOV hardware handles config space accesses to virtual functions. No kernel changes needed for that aspect of it.

Quoted 4 lines

Patches 2 and 3 of the patchset that enables SR/IOV in kvm [1] suggest that at the config space is only partially implemented.

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/29034

I believe Red Hat are now members of the PCI SIG, you really should download the SR-IOV spec that Yu Zhao keeps pointing at. It's going to be very hard to have a sensible discussion if you haven't read it.

Quoted 4 lines · Matthew Wilcox

Quoted 2 lines · Avi Kivity

Do those patches allow using a VF on the host (in other words, does the kernel emulate config space accesses)?

Quoted 2 lines

SR-IOV hardware handles config space accesses to virtual functions. No kernel changes needed for that aspect of it.

Patches 2 and 3 of the patchset that enables SR/IOV in kvm [1] suggest that at the config space is only partially implemented.

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/29034

Quoted 5 lines · On Sunday 08 March 2009 22:30:16 Avi Kivity wrote:

Quoted 4 lines · Matthew Wilcox

Quoted 2 lines · Avi Kivity

Do those patches allow using a VF on the host (in other words, does the kernel emulate config space accesses)?

Quoted 7 lines

Quoted 2 lines

SR-IOV hardware handles config space accesses to virtual functions. No kernel changes needed for that aspect of it.

Patches 2 and 3 of the patchset that enables SR/IOV in kvm [1] suggest that at the config space is only partially implemented.

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/29034

Hi Avi

For kernel side, patch 2 is not necessary. Because kernel would read VID/DID directly from pci_dev rather than configuration space, which have been set properly already.

And very sorry, for the patch 3. We haven't known exactly what's happened. I think the problem is caused by guest driver, but didn't confirm(and I have some misunderstandings with ZhaoYu for I thought we are agree on the reason, but after confirm with him, he didn't agree). I am doing more investigations to find the real cause.

Quoted 11 lines

Quoted 5 lines

Patches 2 and 3 of the patchset that enables SR/IOV in kvm [1] suggest that at the config space is only partially implemented.

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/29034

Hi Avi

For kernel side, patch 2 is not necessary. Because kernel would read VID/DID directly from pci_dev rather than configuration space, which have been set properly already.

Quoted 6 lines

And very sorry, for the patch 3. We haven't known exactly what's happened. I think the problem is caused by guest driver, but didn't confirm(and I have some misunderstandings with ZhaoYu for I thought we are agree on the reason, but after confirm with him, he didn't agree). I am doing more investigations to find the real cause.

Found the reason of patch 3.

After insert guest driver module(vf driver), the driver would do a RMW to the command register to enable Bus Master bit(bit 2). And before that, MMIO bit have been set in the register. But without the patch 3, guest driver won't see the MMIO bit(bit 1), then just set 0x4 to the command register, with the side effect to unmap MMIO in QEmu. So patch 3 is needed(and what I thought before is right).

Unset the bit only affect the QEmu, which would unmap the mapping for MMIO. Kernel side don't need this, so it's OK.

Following patches are intended to support SR-IOV capability in the Linux kernel. With these patches, people can turn a PCI device with the capability into multiple ones from software perspective, which will benefit KVM and achieve other purposes such as QoS, security, and etc.

Physical Function driver patches for Intel 82576 NIC are available:
http://patchwork.kernel.org/patch/8063/
http://patchwork.kernel.org/patch/8064/
http://patchwork.kernel.org/patch/8065/
http://patchwork.kernel.org/patch/8066/

Quoted 5 lines

Physical Function driver patches for Intel 82576 NIC are available:
http://patchwork.kernel.org/patch/8063/
http://patchwork.kernel.org/patch/8064/
http://patchwork.kernel.org/patch/8065/
http://patchwork.kernel.org/patch/8066/

I need to review this driver; I haven't done that yet. Has anyone else?

Quoted 7 lines · Matthew Wilcox

Quoted 5 lines

Physical Function driver patches for Intel 82576 NIC are available:
http://patchwork.kernel.org/patch/8063/
http://patchwork.kernel.org/patch/8064/
http://patchwork.kernel.org/patch/8065/
http://patchwork.kernel.org/patch/8066/

I need to review this driver; I haven't done that yet. Has anyone else?

The driver was rejected by the upstream developers, who said it would never be accepted.