| Message ID | 20180601020921.30957-1-tiwei.bie@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] virtio_pci: support enabling VFs | expand |
On Fri, Jun 01, 2018 at 10:09:21AM +0800, Tiwei Bie wrote: > There is a new feature bit allocated in virtio spec to > support SR-IOV (Single Root I/O Virtualization): > > https://github.com/oasis-tcs/virtio-spec/issues/11 > > This patch enables the support for this feature bit in > virtio driver. > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > Acked-by: Michael S. Tsirkin <mst@redhat.com> Hi Stefan, I'm sorry, I just noticed that I forgot to include you in the To/Cc list. I carried your "Acked-by" for the previous version [1] to this version (v2). Because from my understanding, this is an ACK for this feature. And the change in this version isn't big. The 1st change is to disable VFs when unbinding the driver. And the 2nd one is to drop the use of pci_sriov_configure_simple. So I guess the "ACK" is still valid. Please let me know if I'm wrong. Thanks a lot! Hi Michael, I also carried your "Acked-by" for the previous version [2] to this one (v2). Please let me know if I'm wrong. Thanks a lot! [1] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00182.html [2] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00183.html Best regards, Tiwei Bie > --- > v2: > - Disable VFs when unbinding the driver (Alex, MST); > - Don't use pci_sriov_configure_simple (Alex); > > drivers/virtio/virtio_pci_common.c | 30 ++++++++++++++++++++++++++++++ > drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++ > include/uapi/linux/virtio_config.h | 7 ++++++- > 3 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index 48d4d1cf1cb6..1d4467b2dc31 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -577,6 +577,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); > struct device *dev = get_device(&vp_dev->vdev.dev); > > + pci_disable_sriov(pci_dev); > + > unregister_virtio_device(&vp_dev->vdev); > > if (vp_dev->ioaddr) > @@ -588,6 +590,33 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > put_device(dev); > } > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs) > +{ > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); > + struct virtio_device *vdev = &vp_dev->vdev; > + int ret; > + > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK)) > + return -EBUSY; > + > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV)) > + return -EINVAL; > + > + if (pci_vfs_assigned(pci_dev)) > + return -EPERM; > + > + if (num_vfs == 0) { > + pci_disable_sriov(pci_dev); > + return 0; > + } > + > + ret = pci_enable_sriov(pci_dev, num_vfs); > + if (ret < 0) > + return ret; > + > + return num_vfs; > +} > + > static struct pci_driver virtio_pci_driver = { > .name = "virtio-pci", > .id_table = virtio_pci_id_table, > @@ -596,6 +625,7 @@ static struct pci_driver virtio_pci_driver = { > #ifdef CONFIG_PM_SLEEP > .driver.pm = &virtio_pci_pm_ops, > #endif > + .sriov_configure = virtio_pci_sriov_configure, > }; > > module_pci_driver(virtio_pci_driver); > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index 2555d80f6eec..07571daccfec 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -153,14 +153,28 @@ static u64 vp_get_features(struct virtio_device *vdev) > return features; > } > > +static void vp_transport_features(struct virtio_device *vdev, u64 features) > +{ > + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + struct pci_dev *pci_dev = vp_dev->pci_dev; > + > + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) && > + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV)) > + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV); > +} > + > /* virtio config->finalize_features() implementation */ > static int vp_finalize_features(struct virtio_device *vdev) > { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + u64 features = vdev->features; > > /* Give virtio_ring a chance to accept features. */ > vring_transport_features(vdev); > > + /* Give virtio_pci a chance to accept features. */ > + vp_transport_features(vdev, features); > + > if (!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) { > dev_err(&vdev->dev, "virtio: device uses modern interface " > "but does not have VIRTIO_F_VERSION_1\n"); > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > index 308e2096291f..b7c1f4e7d59e 100644 > --- a/include/uapi/linux/virtio_config.h > +++ b/include/uapi/linux/virtio_config.h > @@ -49,7 +49,7 @@ > * transport being used (eg. virtio_ring), the rest are per-device feature > * bits. */ > #define VIRTIO_TRANSPORT_F_START 28 > -#define VIRTIO_TRANSPORT_F_END 34 > +#define VIRTIO_TRANSPORT_F_END 38 > > #ifndef VIRTIO_CONFIG_NO_LEGACY > /* Do we get callbacks when the ring is completely used, even if we've > @@ -71,4 +71,9 @@ > * this is for compatibility with legacy systems. > */ > #define VIRTIO_F_IOMMU_PLATFORM 33 > + > +/* > + * Does the device support Single Root I/O Virtualization? > + */ > +#define VIRTIO_F_SR_IOV 37 > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */ > -- > 2.17.0 >
On Fri, Jun 01, 2018 at 10:09:21AM +0800, Tiwei Bie wrote: > There is a new feature bit allocated in virtio spec to > support SR-IOV (Single Root I/O Virtualization): > > https://github.com/oasis-tcs/virtio-spec/issues/11 > > This patch enables the support for this feature bit in > virtio driver. > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- Generally you aren't supposed to carry forward acks when you make major changes. In this case I'm fine with the patch, so never mind. > v2: > - Disable VFs when unbinding the driver (Alex, MST); > - Don't use pci_sriov_configure_simple (Alex); > > drivers/virtio/virtio_pci_common.c | 30 ++++++++++++++++++++++++++++++ > drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++ > include/uapi/linux/virtio_config.h | 7 ++++++- > 3 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index 48d4d1cf1cb6..1d4467b2dc31 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -577,6 +577,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); > struct device *dev = get_device(&vp_dev->vdev.dev); > > + pci_disable_sriov(pci_dev); > + > unregister_virtio_device(&vp_dev->vdev); > > if (vp_dev->ioaddr) > @@ -588,6 +590,33 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > put_device(dev); > } > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs) > +{ > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); > + struct virtio_device *vdev = &vp_dev->vdev; > + int ret; > + > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK)) > + return -EBUSY; > + > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV)) > + return -EINVAL; > + > + if (pci_vfs_assigned(pci_dev)) > + return -EPERM; Not a comment on this patch - existing code has the same race - but generally 1. this seems racy since assigning vfs does not seem to take device lock 2. does this work correctly for kvm at all? pci_set_dev_assigned seems to be only called by xen. Can you look at addressing this pls? > + > + if (num_vfs == 0) { > + pci_disable_sriov(pci_dev); > + return 0; > + } > + > + ret = pci_enable_sriov(pci_dev, num_vfs); > + if (ret < 0) > + return ret; > + > + return num_vfs; > +} > + > static struct pci_driver virtio_pci_driver = { > .name = "virtio-pci", > .id_table = virtio_pci_id_table, > @@ -596,6 +625,7 @@ static struct pci_driver virtio_pci_driver = { > #ifdef CONFIG_PM_SLEEP > .driver.pm = &virtio_pci_pm_ops, > #endif > + .sriov_configure = virtio_pci_sriov_configure, > }; > > module_pci_driver(virtio_pci_driver); > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index 2555d80f6eec..07571daccfec 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -153,14 +153,28 @@ static u64 vp_get_features(struct virtio_device *vdev) > return features; > } > > +static void vp_transport_features(struct virtio_device *vdev, u64 features) > +{ > + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + struct pci_dev *pci_dev = vp_dev->pci_dev; > + > + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) && > + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV)) > + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV); > +} > + > /* virtio config->finalize_features() implementation */ > static int vp_finalize_features(struct virtio_device *vdev) > { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + u64 features = vdev->features; > > /* Give virtio_ring a chance to accept features. */ > vring_transport_features(vdev); > > + /* Give virtio_pci a chance to accept features. */ > + vp_transport_features(vdev, features); > + > if (!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) { > dev_err(&vdev->dev, "virtio: device uses modern interface " > "but does not have VIRTIO_F_VERSION_1\n"); > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > index 308e2096291f..b7c1f4e7d59e 100644 > --- a/include/uapi/linux/virtio_config.h > +++ b/include/uapi/linux/virtio_config.h > @@ -49,7 +49,7 @@ > * transport being used (eg. virtio_ring), the rest are per-device feature > * bits. */ > #define VIRTIO_TRANSPORT_F_START 28 > -#define VIRTIO_TRANSPORT_F_END 34 > +#define VIRTIO_TRANSPORT_F_END 38 > > #ifndef VIRTIO_CONFIG_NO_LEGACY > /* Do we get callbacks when the ring is completely used, even if we've > @@ -71,4 +71,9 @@ > * this is for compatibility with legacy systems. > */ > #define VIRTIO_F_IOMMU_PLATFORM 33 > + > +/* > + * Does the device support Single Root I/O Virtualization? > + */ > +#define VIRTIO_F_SR_IOV 37 > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */ > -- > 2.17.0
On Fri, Jun 01, 2018 at 06:47:01AM +0300, Michael S. Tsirkin wrote: > On Fri, Jun 01, 2018 at 10:09:21AM +0800, Tiwei Bie wrote: > > There is a new feature bit allocated in virtio spec to > > support SR-IOV (Single Root I/O Virtualization): > > > > https://github.com/oasis-tcs/virtio-spec/issues/11 > > > > This patch enables the support for this feature bit in > > virtio driver. > > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> > > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > Generally you aren't supposed to carry forward acks > when you make major changes. Got it! Thank you very much! > > In this case I'm fine with the patch, so never mind. Thank you so much! Anyway, it's my fault. I'll send a new version dropping all acks. > [...] > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs) > > +{ > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); > > + struct virtio_device *vdev = &vp_dev->vdev; > > + int ret; > > + > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK)) > > + return -EBUSY; > > + > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV)) > > + return -EINVAL; > > + > > + if (pci_vfs_assigned(pci_dev)) > > + return -EPERM; > > Not a comment on this patch - existing code has the > same race - but generally > > 1. this seems racy since assigning vfs does not seem > to take device lock > 2. does this work correctly for kvm at all? > pci_set_dev_assigned seems to be only called by xen. > > Can you look at addressing this pls? Sure! I'll do it! Best regards, Tiwei Bie
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index 48d4d1cf1cb6..1d4467b2dc31 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -577,6 +577,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); struct device *dev = get_device(&vp_dev->vdev.dev); + pci_disable_sriov(pci_dev); + unregister_virtio_device(&vp_dev->vdev); if (vp_dev->ioaddr) @@ -588,6 +590,33 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) put_device(dev); } +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs) +{ + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); + struct virtio_device *vdev = &vp_dev->vdev; + int ret; + + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK)) + return -EBUSY; + + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV)) + return -EINVAL; + + if (pci_vfs_assigned(pci_dev)) + return -EPERM; + + if (num_vfs == 0) { + pci_disable_sriov(pci_dev); + return 0; + } + + ret = pci_enable_sriov(pci_dev, num_vfs); + if (ret < 0) + return ret; + + return num_vfs; +} + static struct pci_driver virtio_pci_driver = { .name = "virtio-pci", .id_table = virtio_pci_id_table, @@ -596,6 +625,7 @@ static struct pci_driver virtio_pci_driver = { #ifdef CONFIG_PM_SLEEP .driver.pm = &virtio_pci_pm_ops, #endif + .sriov_configure = virtio_pci_sriov_configure, }; module_pci_driver(virtio_pci_driver); diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c index 2555d80f6eec..07571daccfec 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c @@ -153,14 +153,28 @@ static u64 vp_get_features(struct virtio_device *vdev) return features; } +static void vp_transport_features(struct virtio_device *vdev, u64 features) +{ + struct virtio_pci_device *vp_dev = to_vp_device(vdev); + struct pci_dev *pci_dev = vp_dev->pci_dev; + + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) && + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV)) + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV); +} + /* virtio config->finalize_features() implementation */ static int vp_finalize_features(struct virtio_device *vdev) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); + u64 features = vdev->features; /* Give virtio_ring a chance to accept features. */ vring_transport_features(vdev); + /* Give virtio_pci a chance to accept features. */ + vp_transport_features(vdev, features); + if (!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) { dev_err(&vdev->dev, "virtio: device uses modern interface " "but does not have VIRTIO_F_VERSION_1\n"); diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h index 308e2096291f..b7c1f4e7d59e 100644 --- a/include/uapi/linux/virtio_config.h +++ b/include/uapi/linux/virtio_config.h @@ -49,7 +49,7 @@ * transport being used (eg. virtio_ring), the rest are per-device feature * bits. */ #define VIRTIO_TRANSPORT_F_START 28 -#define VIRTIO_TRANSPORT_F_END 34 +#define VIRTIO_TRANSPORT_F_END 38 #ifndef VIRTIO_CONFIG_NO_LEGACY /* Do we get callbacks when the ring is completely used, even if we've @@ -71,4 +71,9 @@ * this is for compatibility with legacy systems. */ #define VIRTIO_F_IOMMU_PLATFORM 33 + +/* + * Does the device support Single Root I/O Virtualization? + */ +#define VIRTIO_F_SR_IOV 37 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */