Re: [PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
On Tue, Mar 10, 2009 at 09:37:53AM +0800, Yu Zhao wrote: On Tue, Mar 10, 2009 at 03:39:01AM +0800, Greg KH wrote: On Mon, Mar 09, 2009 at 04:25:05PM +0800, Yu Zhao wrote: + 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. 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/:01:00.0/ ... ... virtfn0 - ../:02:00.0 ... virtfn1 - ../:02:00.1 ... virtfn2 - ../:02:00.2 ... $ ls -l /sys/bus/pci/devices/:02:00.0/ ... ... physfn - ../: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. Ok, but then make sure you never rely on a udev rule or notifier to see these symlinks when the device is added to the kernel, as there will be a nice race condition there :) thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
On Sat, Mar 07, 2009 at 04:37:18AM +0800, Matthew Wilcox wrote:
On Fri, Feb 20, 2009 at 02:54:45PM +0800, Yu Zhao wrote:
+ 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;
+ virtfn-dma_mask = 0x;
+ 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?
It's doable. Will enhance the pci_setup_device and use it to setup the VF.
+ 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.
+ 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.
The PF driver patches I listed before support dynamical enabling/disabling
of the SR-IOV through sysfs interface. So we have to protect the VF bus
allocation as I explained before.
+ 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.
Yes, will replace %d to %u.
+ 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!
+ 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++;
}
Yes, will do
?
+ 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?
+ dev_err(dev-dev, no enough bus range for SR-IOV\n);
and here.
+ 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?
It's defined by spec as:
3.3.8. Function Dependency Link (12h)
The programming model for a Device may have vendor specific dependencies
between sets of Functions. The Function Dependency Link field is used to
describe these dependencies. This field describes dependencies between PFs.
VF dependencies are the same as the dependencies of their associated PFs.
If a PF is independent from other PFs of a Device, this field shall
contain its own Function Number. If a PF is dependent on other PFs of a
Device, this field shall contain the Function Number of the next PF in
the same Function Dependency List. The last PF in a Function Dependency
List shall contain the Function Number of the first PF in the Function
Dependency List. If PF p and PF q are in the same Function Dependency
List, than any SI that is assigned VF p,n shall also be assigned to VF q,n.
Thanks,
Yu
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
Thanks a lot, Randy!
On Sat, Mar 07, 2009 at 05:48:33AM +0800, Randy Dunlap wrote:
Matthew Wilcox wrote:
On Fri, Feb 20, 2009 at 02:54:45PM +0800, Yu Zhao wrote:
+ 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
?
Randy, can you help us out with better wording here?
+ 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
?
+ 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);
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
On Mon, Mar 09, 2009 at 04:25:05PM +0800, Yu Zhao wrote: + 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. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
On Tue, Mar 10, 2009 at 03:39:01AM +0800, Greg KH wrote: On Mon, Mar 09, 2009 at 04:25:05PM +0800, Yu Zhao wrote: + 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. 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/:01:00.0/ ... ... virtfn0 - ../:02:00.0 ... virtfn1 - ../:02:00.1 ... virtfn2 - ../:02:00.2 ... $ ls -l /sys/bus/pci/devices/:02:00.0/ ... ... physfn - ../: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. Thanks, Yu -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
On Fri, Feb 20, 2009 at 02:54:45PM +0800, Yu Zhao wrote:
+ 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;
+ virtfn-dma_mask = 0x;
+ 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?
+ 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.
+ 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.
+ 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.
+ 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!
+ 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++;
}
?
+ 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?
+ dev_err(dev-dev, no enough bus range for SR-IOV\n);
and here.
+ 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?
--
Matthew Wilcox Intel Open Source Technology Centre
Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
Matthew Wilcox wrote:
On Fri, Feb 20, 2009 at 02:54:45PM +0800, Yu Zhao wrote:
+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
?
Randy, can you help us out with better wording here?
+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
?
+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);
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
On Fri, Mar 06, 2009 at 01:37:18PM -0700, Matthew Wilcox wrote: On Fri, Feb 20, 2009 at 02:54:45PM +0800, Yu Zhao wrote: + 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; + virtfn-dma_mask = 0x; + 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? + 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. If the uevent gets sent before the symlinks are created, it's a bug. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 4/7] PCI: add SR-IOV API for Physical Function driver
Signed-off-by: Yu Zhao yu.z...@intel.com
---
drivers/pci/iov.c | 348 +++
drivers/pci/pci.h |3 +
include/linux/pci.h | 14 ++
3 files changed, 365 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 0b80437..8096fc9 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -13,6 +13,8 @@
#include linux/delay.h
#include pci.h
+#define VIRTFN_ID_LEN 8
+
static inline void virtfn_bdf(struct pci_dev *dev, int id, u8 *busnr, u8
*devfn)
{
@@ -24,6 +26,319 @@ static inline void virtfn_bdf(struct pci_dev *dev, int id,
u8 *busnr, u8 *devfn)
*devfn = bdf 0xff;
}
+static struct pci_bus *virtfn_add_bus(struct pci_bus *bus, int busnr)
+{
+ int rc;
+ struct pci_bus *child;
+
+ if (bus-number == busnr)
+ return bus;
+
+ child = pci_find_bus(pci_domain_nr(bus), busnr);
+ if (child)
+ return child;
+
+ child = pci_add_new_bus(bus, NULL, busnr);
+ if (!child)
+ return NULL;
+
+ child-subordinate = busnr;
+ child-dev.parent = bus-bridge;
+ rc = pci_bus_add_child(child);
+ if (rc) {
+ pci_remove_bus(child);
+ return NULL;
+ }
+
+ return child;
+}
+
+static void virtfn_remove_bus(struct pci_bus *bus, int busnr)
+{
+ struct pci_bus *child;
+
+ if (bus-number == busnr)
+ return;
+
+ child = pci_find_bus(pci_domain_nr(bus), busnr);
+ BUG_ON(!child);
+
+ if (list_empty(child-devices))
+ pci_remove_bus(child);
+}
+
+static int virtfn_add(struct pci_dev *dev, int id, int reset)
+{
+ int i;
+ int rc;
+ u64 size;
+ u8 busnr, devfn;
+ char buf[VIRTFN_ID_LEN];
+ struct pci_dev *virtfn;
+ struct resource *res;
+ struct pci_sriov *iov = dev-sriov;
+
+ virtfn = alloc_pci_dev();
+ if (!virtfn)
+ return -ENOMEM;
+
+ virtfn_bdf(dev, id, busnr, devfn);
+ mutex_lock(iov-pdev-sriov-lock);
+ virtfn-bus = virtfn_add_bus(dev-bus, busnr);
+ if (!virtfn-bus) {
+ kfree(virtfn);
+ mutex_unlock(iov-pdev-sriov-lock);
+ return -ENOMEM;
+ }
+
+ 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;
+ virtfn-dma_mask = 0x;
+ virtfn-vendor = dev-vendor;
+ virtfn-subsystem_vendor = dev-subsystem_vendor;
+ virtfn-class = dev-class;
+ pci_read_config_word(dev, iov-pos + PCI_SRIOV_VF_DID, virtfn-device);
+ pci_read_config_byte(virtfn, PCI_REVISION_ID, virtfn-revision);
+ pci_read_config_word(virtfn, PCI_SUBSYSTEM_ID,
+virtfn-subsystem_device);
+
+ dev_set_name(virtfn-dev, %04x:%02x:%02x.%d,
+pci_domain_nr(virtfn-bus), busnr,
+PCI_SLOT(devfn), PCI_FUNC(devfn));
+
+ for (i = 0; i PCI_SRIOV_NUM_BARS; i++) {
+ res = dev-resource + PCI_SRIOV_RESOURCES + i;
+ if (!res-parent)
+ continue;
+ virtfn-resource[i].name = pci_name(virtfn);
+ virtfn-resource[i].flags = res-flags;
+ size = resource_size(res);
+ do_div(size, iov-total);
+ virtfn-resource[i].start = res-start + size * id;
+ virtfn-resource[i].end = virtfn-resource[i].start + size - 1;
+ rc = request_resource(res, virtfn-resource[i]);
+ BUG_ON(rc);
+ }
+
+ if (reset)
+ pci_execute_reset_function(virtfn);
+
+ pci_device_add(virtfn, virtfn-bus);
+ mutex_unlock(iov-pdev-sriov-lock);
+
+ virtfn-physfn = pci_dev_get(dev);
+
+ rc = pci_bus_add_device(virtfn);
+ if (rc)
+ goto failed1;
+ sprintf(buf, %d, id);
+ 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;
+
+ kobject_uevent(virtfn-dev.kobj, KOBJ_CHANGE);
+
+ return 0;
+
+failed2:
+ sysfs_remove_link(iov-dev.kobj, buf);
+failed1:
+ pci_dev_put(dev);
+ mutex_lock(iov-pdev-sriov-lock);
+ pci_remove_bus_device(virtfn);
+ virtfn_remove_bus(dev-bus, busnr);
+ mutex_unlock(iov-pdev-sriov-lock);
+
+ return rc;
+}
+
+static void virtfn_remove(struct pci_dev *dev, int id, int reset)
+{
+ u8 busnr, devfn;
+ char buf[VIRTFN_ID_LEN];
+ struct
