[PATCH v3] virtio_pci: support enabling VFs

[PATCH v3] virtio_pci: support enabling VFs

[Tiwei Bie]

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>
---
v3:
- Drop the acks;

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

Re: [PATCH v3] virtio_pci: support enabling VFs

[Michael S. Tsirkin]

On Fri, Jun 01, 2018 at 12:02:39PM +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>
> ---

OK but what about freeze/restore functions?

I also wonder about kexec - virtio.c currently does:

        /* We always start by resetting the device, in case a previous
         * driver messed it up.  This also tests that code path a little. */
        dev->config->reset(dev);

Do we need to do something like this for sriov?

I also wonder whether PCI core should disable sriov for us.


I wish there was a patch emulating this without vDPA for QEMU,
would make it easy to test your patches. Do you happen
to have something like this?

Thanks,


> v3:
> - Drop the acks;
> 
> 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

Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[Tiwei Bie]

On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> On Fri, Jun 01, 2018 at 12:02:39PM +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>
> > ---
> 
> OK but what about freeze/restore functions?
> 
> I also wonder about kexec - virtio.c currently does:
> 
>         /* We always start by resetting the device, in case a previous
>          * driver messed it up.  This also tests that code path a little. */
>         dev->config->reset(dev);
> 
> Do we need to do something like this for sriov?

I think VFs are managed by PCI core. Once they are
allocated, virtio driver doesn't have to care too
much about how to manage them. The proposal for the
spec is just to provide a feature bit based virtio
way for virtio drivers to know whether a virtio
device is SR-IOV capable (and virtio drivers can
support configuring SR-IOV based on the feature
bit negotiation result).

> 
> I also wonder whether PCI core should disable sriov for us.
> 
> 
> I wish there was a patch emulating this without vDPA for QEMU,
> would make it easy to test your patches. Do you happen
> to have something like this?

Sorry, currently I don't have anything like this..

Best regards,
Tiwei Bie

> 
> Thanks,
> 
> 
> > v3:
> > - Drop the acks;
> > 
> > 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
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[Michael S. Tsirkin]

On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > On Fri, Jun 01, 2018 at 12:02:39PM +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>
> > > ---
> > 
> > OK but what about freeze/restore functions?

So for restore, don't you need to restore the
sriov capability?


> > I also wonder about kexec - virtio.c currently does:
> > 
> >         /* We always start by resetting the device, in case a previous
> >          * driver messed it up.  This also tests that code path a little. */
> >         dev->config->reset(dev);
> > 
> > Do we need to do something like this for sriov?
> 
> I think VFs are managed by PCI core. Once they are
> allocated, virtio driver doesn't have to care too
> much about how to manage them. The proposal for the
> spec is just to provide a feature bit based virtio
> way for virtio drivers to know whether a virtio
> device is SR-IOV capable (and virtio drivers can
> support configuring SR-IOV based on the feature
> bit negotiation result).
> 
> > 
> > I also wonder whether PCI core should disable sriov for us.
> > 
> > 
> > I wish there was a patch emulating this without vDPA for QEMU,
> > would make it easy to test your patches. Do you happen
> > to have something like this?
> 
> Sorry, currently I don't have anything like this..
> 
> Best regards,
> Tiwei Bie
> 
> > 
> > Thanks,
> > 
> > 
> > > v3:
> > > - Drop the acks;
> > > 
> > > 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
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > 

Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[Tiwei Bie]

On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > On Fri, Jun 01, 2018 at 12:02:39PM +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>
> > > > ---
> > > 
> > > OK but what about freeze/restore functions?
> 
> So for restore, don't you need to restore the
> sriov capability?

Currently I'm not familiar with the PM part.
But I still think the sriov capability should
be handled by PCI core. I'm trying to understand
all the relevant code..

For your question, based on what I found from
the code currently, I guess the sriov capability
will be restored by pci_restore_state() which
will be called by the ops in pci_dev_pm_ops.
The sriov_restore_state() will be called
eventually.

Best regards,
Tiwei Bie


> 
> 
> > > I also wonder about kexec - virtio.c currently does:
> > > 
> > >         /* We always start by resetting the device, in case a previous
> > >          * driver messed it up.  This also tests that code path a little. */
> > >         dev->config->reset(dev);
> > > 
> > > Do we need to do something like this for sriov?
> > 
> > I think VFs are managed by PCI core. Once they are
> > allocated, virtio driver doesn't have to care too
> > much about how to manage them. The proposal for the
> > spec is just to provide a feature bit based virtio
> > way for virtio drivers to know whether a virtio
> > device is SR-IOV capable (and virtio drivers can
> > support configuring SR-IOV based on the feature
> > bit negotiation result).
> > 
> > > 
> > > I also wonder whether PCI core should disable sriov for us.
> > > 
> > > 
> > > I wish there was a patch emulating this without vDPA for QEMU,
> > > would make it easy to test your patches. Do you happen
> > > to have something like this?
> > 
> > Sorry, currently I don't have anything like this..
> > 
> > Best regards,
> > Tiwei Bie
> > 
> > > 
> > > Thanks,
> > > 
> > > 
> > > > v3:
> > > > - Drop the acks;
> > > > 
> > > > 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
> > > 
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > 

Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 01, 2018 at 12:02:39PM +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>
> > > > > ---
> > > > 
> > > > OK but what about freeze/restore functions?
> > 
> > So for restore, don't you need to restore the
> > sriov capability?
> 
> Currently I'm not familiar with the PM part.
> But I still think the sriov capability should
> be handled by PCI core.

OK but the point is restore looks just like power up for device.

> I'm trying to understand
> all the relevant code..
> For your question, based on what I found from
> the code currently, I guess the sriov capability
> will be restored by pci_restore_state() which
> will be called by the ops in pci_dev_pm_ops.
> The sriov_restore_state() will be called
> eventually.
> 
> Best regards,
> Tiwei Bie

Right but my point is during resume SRIOV gets enabled first before
driver ok.

Maybe we should relax the requirements in the spec:
- only require FEATURES_OK from device, not DRIVER_OK from driver
- explain that it only has to happen once, not on each reset,
  and driver can remember the result


> 
> > 
> > 
> > > > I also wonder about kexec - virtio.c currently does:
> > > > 
> > > >         /* We always start by resetting the device, in case a previous
> > > >          * driver messed it up.  This also tests that code path a little. */
> > > >         dev->config->reset(dev);
> > > > 
> > > > Do we need to do something like this for sriov?
> > > 
> > > I think VFs are managed by PCI core. Once they are
> > > allocated, virtio driver doesn't have to care too
> > > much about how to manage them. The proposal for the
> > > spec is just to provide a feature bit based virtio
> > > way for virtio drivers to know whether a virtio
> > > device is SR-IOV capable (and virtio drivers can
> > > support configuring SR-IOV based on the feature
> > > bit negotiation result).
> > > 
> > > > 
> > > > I also wonder whether PCI core should disable sriov for us.
> > > > 
> > > > 
> > > > I wish there was a patch emulating this without vDPA for QEMU,
> > > > would make it easy to test your patches. Do you happen
> > > > to have something like this?
> > > 
> > > Sorry, currently I don't have anything like this..
> > > 
> > > Best regards,
> > > Tiwei Bie
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > 
> > > > > v3:
> > > > > - Drop the acks;
> > > > > 
> > > > > 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
> > > > 
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > 

Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[Tiwei Bie]

On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > > > On Fri, Jun 01, 2018 at 12:02:39PM +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>
> > > > > > ---
> > > > > 
> > > > > OK but what about freeze/restore functions?
> > > 
> > > So for restore, don't you need to restore the
> > > sriov capability?
> > 
> > Currently I'm not familiar with the PM part.
> > But I still think the sriov capability should
> > be handled by PCI core.
> 
> OK but the point is restore looks just like power up for device.
> 
> > I'm trying to understand
> > all the relevant code..
> > For your question, based on what I found from
> > the code currently, I guess the sriov capability
> > will be restored by pci_restore_state() which
> > will be called by the ops in pci_dev_pm_ops.
> > The sriov_restore_state() will be called
> > eventually.
> > 
> > Best regards,
> > Tiwei Bie
> 
> Right but my point is during resume SRIOV gets enabled first before
> driver ok.
> 
> Maybe we should relax the requirements in the spec:
> - only require FEATURES_OK from device, not DRIVER_OK from driver
> - explain that it only has to happen once, not on each reset,
>   and driver can remember the result

I got your point now! I'd like to relax the
requirements in the spec.

Best regards,
Tiwei Bie

> 
> 
> > 
> > > 
> > > 
> > > > > I also wonder about kexec - virtio.c currently does:
> > > > > 
> > > > >         /* We always start by resetting the device, in case a previous
> > > > >          * driver messed it up.  This also tests that code path a little. */
> > > > >         dev->config->reset(dev);
> > > > > 
> > > > > Do we need to do something like this for sriov?
> > > > 
> > > > I think VFs are managed by PCI core. Once they are
> > > > allocated, virtio driver doesn't have to care too
> > > > much about how to manage them. The proposal for the
> > > > spec is just to provide a feature bit based virtio
> > > > way for virtio drivers to know whether a virtio
> > > > device is SR-IOV capable (and virtio drivers can
> > > > support configuring SR-IOV based on the feature
> > > > bit negotiation result).
> > > > 
> > > > > 
> > > > > I also wonder whether PCI core should disable sriov for us.
> > > > > 
> > > > > 
> > > > > I wish there was a patch emulating this without vDPA for QEMU,
> > > > > would make it easy to test your patches. Do you happen
> > > > > to have something like this?
> > > > 
> > > > Sorry, currently I don't have anything like this..
> > > > 
> > > > Best regards,
> > > > Tiwei Bie
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > 
> > > > > > v3:
> > > > > > - Drop the acks;
> > > > > > 
> > > > > > 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
> > > > > 
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > 

Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 10:19:43PM +0800, Tiwei Bie wrote:
> On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jun 01, 2018 at 12:02:39PM +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>
> > > > > > > ---
> > > > > > 
> > > > > > OK but what about freeze/restore functions?
> > > > 
> > > > So for restore, don't you need to restore the
> > > > sriov capability?
> > > 
> > > Currently I'm not familiar with the PM part.
> > > But I still think the sriov capability should
> > > be handled by PCI core.
> > 
> > OK but the point is restore looks just like power up for device.
> > 
> > > I'm trying to understand
> > > all the relevant code..
> > > For your question, based on what I found from
> > > the code currently, I guess the sriov capability
> > > will be restored by pci_restore_state() which
> > > will be called by the ops in pci_dev_pm_ops.
> > > The sriov_restore_state() will be called
> > > eventually.
> > > 
> > > Best regards,
> > > Tiwei Bie
> > 
> > Right but my point is during resume SRIOV gets enabled first before
> > driver ok.
> > 
> > Maybe we should relax the requirements in the spec:
> > - only require FEATURES_OK from device, not DRIVER_OK from driver
> > - explain that it only has to happen once, not on each reset,
> >   and driver can remember the result
> 
> I got your point now! I'd like to relax the
> requirements in the spec.
> 
> Best regards,
> Tiwei Bie

Well the ballot approving your change closed.  I think we should apply
the first chunks reserving the feature bit then, and defer the rest, and
you can work on new wording documenting the actual behaviour with a new
github issue to track that - does this make sense?
Let's do it quickly though - I don't want to bother the
TC with re-voting the deferral, then the new patch.

> > 
> > 
> > > 
> > > > 
> > > > 
> > > > > > I also wonder about kexec - virtio.c currently does:
> > > > > > 
> > > > > >         /* We always start by resetting the device, in case a previous
> > > > > >          * driver messed it up.  This also tests that code path a little. */
> > > > > >         dev->config->reset(dev);
> > > > > > 
> > > > > > Do we need to do something like this for sriov?
> > > > > 
> > > > > I think VFs are managed by PCI core. Once they are
> > > > > allocated, virtio driver doesn't have to care too
> > > > > much about how to manage them. The proposal for the
> > > > > spec is just to provide a feature bit based virtio
> > > > > way for virtio drivers to know whether a virtio
> > > > > device is SR-IOV capable (and virtio drivers can
> > > > > support configuring SR-IOV based on the feature
> > > > > bit negotiation result).
> > > > > 
> > > > > > 
> > > > > > I also wonder whether PCI core should disable sriov for us.
> > > > > > 
> > > > > > 
> > > > > > I wish there was a patch emulating this without vDPA for QEMU,
> > > > > > would make it easy to test your patches. Do you happen
> > > > > > to have something like this?
> > > > > 
> > > > > Sorry, currently I don't have anything like this..
> > > > > 
> > > > > Best regards,
> > > > > Tiwei Bie
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > 
> > > > > > > v3:
> > > > > > > - Drop the acks;
> > > > > > > 
> > > > > > > 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
> > > > > > 
> > > > > > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > > 

Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs

[Tiwei Bie]

On Wed, Jun 06, 2018 at 05:27:07PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2018 at 10:19:43PM +0800, Tiwei Bie wrote:
> > On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > > > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Jun 01, 2018 at 12:02:39PM +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>
> > > > > > > > ---
> > > > > > > 
> > > > > > > OK but what about freeze/restore functions?
> > > > > 
> > > > > So for restore, don't you need to restore the
> > > > > sriov capability?
> > > > 
> > > > Currently I'm not familiar with the PM part.
> > > > But I still think the sriov capability should
> > > > be handled by PCI core.
> > > 
> > > OK but the point is restore looks just like power up for device.
> > > 
> > > > I'm trying to understand
> > > > all the relevant code..
> > > > For your question, based on what I found from
> > > > the code currently, I guess the sriov capability
> > > > will be restored by pci_restore_state() which
> > > > will be called by the ops in pci_dev_pm_ops.
> > > > The sriov_restore_state() will be called
> > > > eventually.
> > > > 
> > > > Best regards,
> > > > Tiwei Bie
> > > 
> > > Right but my point is during resume SRIOV gets enabled first before
> > > driver ok.
> > > 
> > > Maybe we should relax the requirements in the spec:
> > > - only require FEATURES_OK from device, not DRIVER_OK from driver
> > > - explain that it only has to happen once, not on each reset,
> > >   and driver can remember the result
> > 
> > I got your point now! I'd like to relax the
> > requirements in the spec.
> > 
> > Best regards,
> > Tiwei Bie
> 
> Well the ballot approving your change closed.  I think we should apply
> the first chunks reserving the feature bit then, and defer the rest, and
> you can work on new wording documenting the actual behaviour with a new
> github issue to track that - does this make sense?
> Let's do it quickly though - I don't want to bother the
> TC with re-voting the deferral, then the new patch.

Yeah. It makes sense! I think it's a good idea
to reserve the feature bit first and defer the
rest. I'll work on the new wording documenting
the actual behavior with a new github issue to
track it. Thanks a lot!

Best regards,
Tiwei Bie

> 
> > > 
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > > I also wonder about kexec - virtio.c currently does:
> > > > > > > 
> > > > > > >         /* We always start by resetting the device, in case a previous
> > > > > > >          * driver messed it up.  This also tests that code path a little. */
> > > > > > >         dev->config->reset(dev);
> > > > > > > 
> > > > > > > Do we need to do something like this for sriov?
> > > > > > 
> > > > > > I think VFs are managed by PCI core. Once they are
> > > > > > allocated, virtio driver doesn't have to care too
> > > > > > much about how to manage them. The proposal for the
> > > > > > spec is just to provide a feature bit based virtio
> > > > > > way for virtio drivers to know whether a virtio
> > > > > > device is SR-IOV capable (and virtio drivers can
> > > > > > support configuring SR-IOV based on the feature
> > > > > > bit negotiation result).
> > > > > > 
> > > > > > > 
> > > > > > > I also wonder whether PCI core should disable sriov for us.
> > > > > > > 
> > > > > > > 
> > > > > > > I wish there was a patch emulating this without vDPA for QEMU,
> > > > > > > would make it easy to test your patches. Do you happen
> > > > > > > to have something like this?
> > > > > > 
> > > > > > Sorry, currently I don't have anything like this..
> > > > > > 
> > > > > > Best regards,
> > > > > > Tiwei Bie
> > > > > > 
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > 
> > > > > > > > v3:
> > > > > > > > - Drop the acks;
> > > > > > > > 
> > > > > > > > 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
> > > > > > > 
> > > > > > > ---------------------------------------------------------------------
> > > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > > >