[PATCH] virtio_pci: support enabling VFs

[PATCH] 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>
---
This patch depends on below proposal:
https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html

This patch depends on below patch:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238

 drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
 drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
 include/uapi/linux/virtio_config.h |  7 ++++++-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 48d4d1cf1cb6..023da80a7a3e 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -588,6 +588,25 @@ 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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
+
+	if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
+		return -EBUSY;
+
+	if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
+		return -EINVAL;
+
+	sriov_configure = pci_sriov_configure_simple;
+	if (sriov_configure == NULL)
+		return -ENOENT;
+
+	return sriov_configure(pci_dev, num_vfs);
+}
+
 static struct pci_driver virtio_pci_driver = {
 	.name		= "virtio-pci",
 	.id_table	= virtio_pci_id_table,
@@ -596,6 +615,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] [PATCH] virtio_pci: support enabling VFs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

On Wed, May 30, 2018 at 04:55:21PM +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>
> ---
> This patch depends on below proposal:
> https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> 
> This patch depends on below patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> 
>  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
>  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
>  include/uapi/linux/virtio_config.h |  7 ++++++-
>  3 files changed, 40 insertions(+), 1 deletion(-)

Looks good and matches the virtio spec change, but I'm not familiar with
PCI SR-IOV details, so not a full Reviewed-by.

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH] virtio_pci: support enabling VFs

[Michael S. Tsirkin]

On Wed, May 30, 2018 at 04:55:21PM +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: Michael S. Tsirkin <mst@redhat.com>

> ---
> This patch depends on below proposal:
> https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> 
> This patch depends on below patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> 
>  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
>  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
>  include/uapi/linux/virtio_config.h |  7 ++++++-
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 48d4d1cf1cb6..023da80a7a3e 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -588,6 +588,25 @@ 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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> +
> +	if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> +		return -EBUSY;
> +
> +	if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> +		return -EINVAL;
> +
> +	sriov_configure = pci_sriov_configure_simple;
> +	if (sriov_configure == NULL)
> +		return -ENOENT;
> +
> +	return sriov_configure(pci_dev, num_vfs);
> +}
> +
>  static struct pci_driver virtio_pci_driver = {
>  	.name		= "virtio-pci",
>  	.id_table	= virtio_pci_id_table,
> @@ -596,6 +615,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] [PATCH] virtio_pci: support enabling VFs

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

On May 30, 2018, at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>
> ---
> This patch depends on below proposal:
> https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
>
> This patch depends on below patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
>
>  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
>  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
>  include/uapi/linux/virtio_config.h |  7 ++++++-
>  3 files changed, 40 insertions(+), 1 deletion(-)
>

<snip>

> 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 @@

There is a value in the comment directly before this that should
be updated as well to be consistent with the new value for
VIRTIO_TRANSPORT_F_END below.

>   * 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

<snip>

--
Mark Rustad, Networking Division, Intel Corporation

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

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

[Michael S. Tsirkin]

On Wed, May 30, 2018 at 04:03:37PM +0000, Rustad, Mark D wrote:
> On May 30, 2018, at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>
> > ---
> > This patch depends on below proposal:
> > https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> > 
> > This patch depends on below patch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> > 
> >  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
> >  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> >  include/uapi/linux/virtio_config.h |  7 ++++++-
> >  3 files changed, 40 insertions(+), 1 deletion(-)
> > 
> 
> <snip>
> 
> > 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 @@
> 
> There is a value in the comment directly before this that should
> be updated as well to be consistent with the new value for
> VIRTIO_TRANSPORT_F_END below.

It hasn't been updated to 34 yet.
I suggest a separate patch to replace the numbers with
VIRTIO_TRANSPORT_F_START and VIRTIO_TRANSPORT_F_END
in the comment.
Maybe replace "e.g. virtio_ring" with "e.g. virtio_ring,
virtio_pci etc." as well.

> >   * 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
> 
> <snip>
> 
> --
> Mark Rustad, Networking Division, Intel Corporation

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

[Alexander Duyck]

On Wed, May 30, 2018 at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>

So from a quick glance it looks like we are leaving SR-IOV enabled if
the driver is removed. Do we want to have that behavior or should we
be adding the code to disable SR-IOV and free the VFs on driver
removal?

> ---
> This patch depends on below proposal:
> https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
>
> This patch depends on below patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
>
>  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
>  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
>  include/uapi/linux/virtio_config.h |  7 ++++++-
>  3 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 48d4d1cf1cb6..023da80a7a3e 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -588,6 +588,25 @@ 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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> +
> +       if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> +               return -EBUSY;
> +
> +       if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> +               return -EINVAL;
> +
> +       sriov_configure = pci_sriov_configure_simple;
> +       if (sriov_configure == NULL)
> +               return -ENOENT;
> +
> +       return sriov_configure(pci_dev, num_vfs);
> +}
> +
>  static struct pci_driver virtio_pci_driver = {
>         .name           = "virtio-pci",
>         .id_table       = virtio_pci_id_table,
> @@ -596,6 +615,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: [PATCH] virtio_pci: support enabling VFs

[Michael S. Tsirkin]

On Wed, May 30, 2018 at 04:55:21PM +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>
> ---
> This patch depends on below proposal:
> https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> 
> This patch depends on below patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> 
>  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
>  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
>  include/uapi/linux/virtio_config.h |  7 ++++++-
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 48d4d1cf1cb6..023da80a7a3e 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -588,6 +588,25 @@ 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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> +
> +	if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> +		return -EBUSY;
> +
> +	if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> +		return -EINVAL;
> +
> +	sriov_configure = pci_sriov_configure_simple;
> +	if (sriov_configure == NULL)
> +		return -ENOENT;

BTW what is all this trickery in aid of?

> +
> +	return sriov_configure(pci_dev, num_vfs);
> +}
> +
>  static struct pci_driver virtio_pci_driver = {
>  	.name		= "virtio-pci",
>  	.id_table	= virtio_pci_id_table,
> @@ -596,6 +615,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] [PATCH] virtio_pci: support enabling VFs

[Michael S. Tsirkin]

On Wed, May 30, 2018 at 09:10:57AM -0700, Alexander Duyck wrote:
> On Wed, May 30, 2018 at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>
> 
> So from a quick glance it looks like we are leaving SR-IOV enabled if
> the driver is removed. Do we want to have that behavior or should we
> be adding the code to disable SR-IOV and free the VFs on driver
> removal?

Could pci core handle it for us somehow?

> > ---
> > This patch depends on below proposal:
> > https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> >
> > This patch depends on below patch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> >
> >  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
> >  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> >  include/uapi/linux/virtio_config.h |  7 ++++++-
> >  3 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index 48d4d1cf1cb6..023da80a7a3e 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -588,6 +588,25 @@ 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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> > +
> > +       if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > +               return -EBUSY;
> > +
> > +       if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > +               return -EINVAL;
> > +
> > +       sriov_configure = pci_sriov_configure_simple;
> > +       if (sriov_configure == NULL)
> > +               return -ENOENT;
> > +
> > +       return sriov_configure(pci_dev, num_vfs);
> > +}
> > +
> >  static struct pci_driver virtio_pci_driver = {
> >         .name           = "virtio-pci",
> >         .id_table       = virtio_pci_id_table,
> > @@ -596,6 +615,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] [PATCH] virtio_pci: support enabling VFs

[Duyck, Alexander H]

On Wed, 2018-05-30 at 19:22 +0300, Michael S. Tsirkin wrote:
> On Wed, May 30, 2018 at 09:10:57AM -0700, Alexander Duyck wrote:
> > On Wed, May 30, 2018 at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>
> > 
> > So from a quick glance it looks like we are leaving SR-IOV enabled if
> > the driver is removed. Do we want to have that behavior or should we
> > be adding the code to disable SR-IOV and free the VFs on driver
> > removal?
> 
> Could pci core handle it for us somehow?

Maybe, but it would require changes to the pci core to do it.

The problem is some drivers want to leave the VFs there since the PF
doesn't really do anything, or they have the option of essentially
putting the VFs into a standby state when the PF is gone.

My main concern is do we care if VFs are allocated and then somebody
removes the driver and binds a different driver to the interface? If
not then this code and be left as is, but I just wanted to be certain
since I know this isn't just enabling SR-IOV we are having to do a
number of other checks against the virtio device.

Re: [PATCH] virtio_pci: support enabling VFs

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:

>> +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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
>> +
>> +	if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
>> +		return -EBUSY;
>> +
>> +	if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
>> +		return -EINVAL;
>> +
>> +	sriov_configure = pci_sriov_configure_simple;
>> +	if (sriov_configure == NULL)
>> +		return -ENOENT;
>
> BTW what is all this trickery in aid of?

When SR-IOV support is not compiled into the kernel,  
pci_sriov_configure_simple is #defined as NULL. This allows it to compile  
in that case, even though there is utterly no way for it to be called in  
that case. It is an alternative to #ifs in the code.

--
Mark Rustad, Networking Division, Intel Corporation

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] virtio_pci: support enabling VFs

[Duyck, Alexander H]

On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
> On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > > +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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> > > +
> > > +	if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > +		return -EBUSY;
> > > +
> > > +	if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > +		return -EINVAL;
> > > +
> > > +	sriov_configure = pci_sriov_configure_simple;
> > > +	if (sriov_configure == NULL)
> > > +		return -ENOENT;
> > 
> > BTW what is all this trickery in aid of?
> 
> When SR-IOV support is not compiled into the kernel,  
> pci_sriov_configure_simple is #defined as NULL. This allows it to compile  
> in that case, even though there is utterly no way for it to be called in  
> that case. It is an alternative to #ifs in the code.

Why even have the call though? I would wrap all of this in an #ifdef
and strip it out since you couldn't support SR-IOV if it isn't present
in the kernel anyway.

Re: [PATCH] virtio_pci: support enabling VFs

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On May 30, 2018, at 9:54 AM, Duyck, Alexander H  
<alexander.h.duyck@intel.com> wrote:

> On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
>> On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>>>> +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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
>>>> +
>>>> +	if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
>>>> +		return -EBUSY;
>>>> +
>>>> +	if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
>>>> +		return -EINVAL;
>>>> +
>>>> +	sriov_configure = pci_sriov_configure_simple;
>>>> +	if (sriov_configure == NULL)
>>>> +		return -ENOENT;
>>>
>>> BTW what is all this trickery in aid of?
>>
>> When SR-IOV support is not compiled into the kernel,
>> pci_sriov_configure_simple is #defined as NULL. This allows it to compile
>> in that case, even though there is utterly no way for it to be called in
>> that case. It is an alternative to #ifs in the code.
>
> Why even have the call though? I would wrap all of this in an #ifdef
> and strip it out since you couldn't support SR-IOV if it isn't present
> in the kernel anyway.

I am inclined to agree. In this case, the presence of #ifdefs I think would  
be clearer. As written, someone will want to get rid of the pointer only to  
create a build problem when SR-IOV is not configured.

--
Mark Rustad, Networking Division, Intel Corporation

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

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

[Michael S. Tsirkin]

On Wed, May 30, 2018 at 04:26:30PM +0000, Duyck, Alexander H wrote:
> On Wed, 2018-05-30 at 19:22 +0300, Michael S. Tsirkin wrote:
> > On Wed, May 30, 2018 at 09:10:57AM -0700, Alexander Duyck wrote:
> > > On Wed, May 30, 2018 at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>
> > > 
> > > So from a quick glance it looks like we are leaving SR-IOV enabled if
> > > the driver is removed. Do we want to have that behavior or should we
> > > be adding the code to disable SR-IOV and free the VFs on driver
> > > removal?
> > 
> > Could pci core handle it for us somehow?
> 
> Maybe, but it would require changes to the pci core to do it.
> 
> The problem is some drivers want to leave the VFs there since the PF
> doesn't really do anything, or they have the option of essentially
> putting the VFs into a standby state when the PF is gone.
> 
> My main concern is do we care if VFs are allocated and then somebody
> removes the driver and binds a different driver to the interface? If
> not then this code and be left as is, but I just wanted to be certain
> since I know this isn't just enabling SR-IOV we are having to do a
> number of other checks against the virtio device.

Well the spec says features have to be negotiated, and since we reset
the device when we unbind from it I think it's a given we should keep a
driver bound to the PF.

IOW until we are sure we need the capability to keep it enabled, let's
disable it to be safe.

-- 
MST

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

[Tiwei Bie]

On Wed, May 30, 2018 at 07:09:37PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 30, 2018 at 04:03:37PM +0000, Rustad, Mark D wrote:
> > On May 30, 2018, at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>
> > > ---
> > > This patch depends on below proposal:
> > > https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> > > 
> > > This patch depends on below patch:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> > > 
> > >  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
> > >  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > >  include/uapi/linux/virtio_config.h |  7 ++++++-
> > >  3 files changed, 40 insertions(+), 1 deletion(-)
> > > 
> > 
> > <snip>
> > 
> > > 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 @@
> > 
> > There is a value in the comment directly before this that should
> > be updated as well to be consistent with the new value for
> > VIRTIO_TRANSPORT_F_END below.
> 
> It hasn't been updated to 34 yet.
> I suggest a separate patch to replace the numbers with
> VIRTIO_TRANSPORT_F_START and VIRTIO_TRANSPORT_F_END
> in the comment.
> Maybe replace "e.g. virtio_ring" with "e.g. virtio_ring,
> virtio_pci etc." as well.

Good idea! Thanks for the suggestion! I'll do it.

Best regards,
Tiwei Bie


> 
> > >   * 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
> > 
> > <snip>
> > 
> > --
> > Mark Rustad, Networking Division, Intel Corporation
> 
> 

Re: [PATCH] virtio_pci: support enabling VFs

[Tiwei Bie]

On Thu, May 31, 2018 at 01:11:37AM +0800, Rustad, Mark D wrote:
> On May 30, 2018, at 9:54 AM, Duyck, Alexander H
> <alexander.h.duyck@intel.com> wrote:
> 
> > On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
> > > On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > 
> > > > > +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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> > > > > +
> > > > > +	if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > +		return -EBUSY;
> > > > > +
> > > > > +	if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	sriov_configure = pci_sriov_configure_simple;
> > > > > +	if (sriov_configure == NULL)
> > > > > +		return -ENOENT;
> > > > 
> > > > BTW what is all this trickery in aid of?
> > > 
> > > When SR-IOV support is not compiled into the kernel,
> > > pci_sriov_configure_simple is #defined as NULL. This allows it to compile
> > > in that case, even though there is utterly no way for it to be called in
> > > that case. It is an alternative to #ifs in the code.
> > 
> > Why even have the call though? I would wrap all of this in an #ifdef
> > and strip it out since you couldn't support SR-IOV if it isn't present
> > in the kernel anyway.
> 
> I am inclined to agree. In this case, the presence of #ifdefs I think would
> be clearer. As written, someone will want to get rid of the pointer only to
> create a build problem when SR-IOV is not configured.

In my opinion, maybe it would be better to make
pci_sriov_configure_simple() always available
just like other sriov functions.

Based on the comments in the original patch:

https://patchwork.kernel.org/patch/10353197/
"""
+/* this is expected to be used as a function pointer, just define as NULL */
+#define pci_sriov_configure_simple NULL
"""

This function could be defined as NULL just because
it was expected to be used as a function pointer.
But actually it could be called directly as a
function, just like this case.

So I prefer to make this function always available
just like other sriov functions.

Best regards,
Tiwei Bie

Re: [PATCH] virtio_pci: support enabling VFs

[Alexander Duyck]

On Wed, May 30, 2018 at 8:20 PM, Tiwei Bie <tiwei.bie@intel.com> wrote:
> On Thu, May 31, 2018 at 01:11:37AM +0800, Rustad, Mark D wrote:
>> On May 30, 2018, at 9:54 AM, Duyck, Alexander H
>> <alexander.h.duyck@intel.com> wrote:
>>
>> > On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
>> > > On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > >
>> > > > > +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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
>> > > > > +
>> > > > > +     if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
>> > > > > +             return -EBUSY;
>> > > > > +
>> > > > > +     if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
>> > > > > +             return -EINVAL;
>> > > > > +
>> > > > > +     sriov_configure = pci_sriov_configure_simple;
>> > > > > +     if (sriov_configure == NULL)
>> > > > > +             return -ENOENT;
>> > > >
>> > > > BTW what is all this trickery in aid of?
>> > >
>> > > When SR-IOV support is not compiled into the kernel,
>> > > pci_sriov_configure_simple is #defined as NULL. This allows it to compile
>> > > in that case, even though there is utterly no way for it to be called in
>> > > that case. It is an alternative to #ifs in the code.
>> >
>> > Why even have the call though? I would wrap all of this in an #ifdef
>> > and strip it out since you couldn't support SR-IOV if it isn't present
>> > in the kernel anyway.
>>
>> I am inclined to agree. In this case, the presence of #ifdefs I think would
>> be clearer. As written, someone will want to get rid of the pointer only to
>> create a build problem when SR-IOV is not configured.
>
> In my opinion, maybe it would be better to make
> pci_sriov_configure_simple() always available
> just like other sriov functions.
>
> Based on the comments in the original patch:
>
> https://patchwork.kernel.org/patch/10353197/
> """
> +/* this is expected to be used as a function pointer, just define as NULL */
> +#define pci_sriov_configure_simple NULL
> """
>
> This function could be defined as NULL just because
> it was expected to be used as a function pointer.
> But actually it could be called directly as a
> function, just like this case.
>
> So I prefer to make this function always available
> just like other sriov functions.
>
> Best regards,
> Tiwei Bie

The fact that you are having to add additional code kind of implies
that maybe this doesn't fall into the pci_sriov_configure_simple case
anymore. The PF itself is defining what the VF can and can't do via
the feature flags you are testing for.

For example how is the bit of code below valid if the kernel itself
doesn't support SR-IOV:
+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);
+}
+

It really seems like we should be wrapping these functions at the very
minimum so that they don't imply you have SR-IOV support when it isn't
supported in the kernel.

Also it seems like we should be disabling the VFs if the driver is
unbound from this interface. We need to add logic to disable SR-IOV if
the driver is removed. What we don't want to do is leave VFs allocated
and then have the potential for us to unbind/rebind the driver as the
new driver may change the negotiated features.

- Alex

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

[Michael S. Tsirkin]

On Thu, May 31, 2018 at 10:55:02AM +0800, Tiwei Bie wrote:
> On Wed, May 30, 2018 at 07:09:37PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 30, 2018 at 04:03:37PM +0000, Rustad, Mark D wrote:
> > > On May 30, 2018, at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>
> > > > ---
> > > > This patch depends on below proposal:
> > > > https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> > > > 
> > > > This patch depends on below patch:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> > > > 
> > > >  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
> > > >  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > > >  include/uapi/linux/virtio_config.h |  7 ++++++-
> > > >  3 files changed, 40 insertions(+), 1 deletion(-)
> > > > 
> > > 
> > > <snip>
> > > 
> > > > 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 @@
> > > 
> > > There is a value in the comment directly before this that should
> > > be updated as well to be consistent with the new value for
> > > VIRTIO_TRANSPORT_F_END below.
> > 
> > It hasn't been updated to 34 yet.
> > I suggest a separate patch to replace the numbers with
> > VIRTIO_TRANSPORT_F_START and VIRTIO_TRANSPORT_F_END
> > in the comment.
> > Maybe replace "e.g. virtio_ring" with "e.g. virtio_ring,
> > virtio_pci etc." as well.
> 
> Good idea! Thanks for the suggestion! I'll do it.
> 
> Best regards,
> Tiwei Bie

Or just focus on the new feature, tweak start/end
as a separate patch. Up to you, the important thing
is to have a ready to roll patch before the merge
window.

> 
> > 
> > > >   * 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
> > > 
> > > <snip>
> > > 
> > > --
> > > Mark Rustad, Networking Division, Intel Corporation
> > 
> > 

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

[Tiwei Bie]

On Fri, Jun 01, 2018 at 04:21:03AM +0300, Michael S. Tsirkin wrote:
> On Thu, May 31, 2018 at 10:55:02AM +0800, Tiwei Bie wrote:
> > On Wed, May 30, 2018 at 07:09:37PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, May 30, 2018 at 04:03:37PM +0000, Rustad, Mark D wrote:
> > > > On May 30, 2018, at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> 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>
> > > > > ---
> > > > > This patch depends on below proposal:
> > > > > https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> > > > > 
> > > > > This patch depends on below patch:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> > > > > 
> > > > >  drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
> > > > >  drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > > > >  include/uapi/linux/virtio_config.h |  7 ++++++-
> > > > >  3 files changed, 40 insertions(+), 1 deletion(-)
> > > > > 
> > > > 
> > > > <snip>
> > > > 
> > > > > 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 @@
> > > > 
> > > > There is a value in the comment directly before this that should
> > > > be updated as well to be consistent with the new value for
> > > > VIRTIO_TRANSPORT_F_END below.
> > > 
> > > It hasn't been updated to 34 yet.
> > > I suggest a separate patch to replace the numbers with
> > > VIRTIO_TRANSPORT_F_START and VIRTIO_TRANSPORT_F_END
> > > in the comment.
> > > Maybe replace "e.g. virtio_ring" with "e.g. virtio_ring,
> > > virtio_pci etc." as well.
> > 
> > Good idea! Thanks for the suggestion! I'll do it.
> > 
> > Best regards,
> > Tiwei Bie
> 
> Or just focus on the new feature, tweak start/end
> as a separate patch. Up to you, the important thing
> is to have a ready to roll patch before the merge
> window.

Got it! Thank you very much!

Best regards,
Tiwei Bie

Re: [PATCH] virtio_pci: support enabling VFs

[Tiwei Bie]

On Thu, May 31, 2018 at 07:27:43AM -0700, Alexander Duyck wrote:
> On Wed, May 30, 2018 at 8:20 PM, Tiwei Bie <tiwei.bie@intel.com> wrote:
> > On Thu, May 31, 2018 at 01:11:37AM +0800, Rustad, Mark D wrote:
> >> On May 30, 2018, at 9:54 AM, Duyck, Alexander H
> >> <alexander.h.duyck@intel.com> wrote:
> >>
> >> > On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
> >> > > On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > >
> >> > > > > +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 (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> >> > > > > +
> >> > > > > +     if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> >> > > > > +             return -EBUSY;
> >> > > > > +
> >> > > > > +     if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> >> > > > > +             return -EINVAL;
> >> > > > > +
> >> > > > > +     sriov_configure = pci_sriov_configure_simple;
> >> > > > > +     if (sriov_configure == NULL)
> >> > > > > +             return -ENOENT;
> >> > > >
> >> > > > BTW what is all this trickery in aid of?
> >> > >
> >> > > When SR-IOV support is not compiled into the kernel,
> >> > > pci_sriov_configure_simple is #defined as NULL. This allows it to compile
> >> > > in that case, even though there is utterly no way for it to be called in
> >> > > that case. It is an alternative to #ifs in the code.
> >> >
> >> > Why even have the call though? I would wrap all of this in an #ifdef
> >> > and strip it out since you couldn't support SR-IOV if it isn't present
> >> > in the kernel anyway.
> >>
> >> I am inclined to agree. In this case, the presence of #ifdefs I think would
> >> be clearer. As written, someone will want to get rid of the pointer only to
> >> create a build problem when SR-IOV is not configured.
> >
> > In my opinion, maybe it would be better to make
> > pci_sriov_configure_simple() always available
> > just like other sriov functions.
> >
> > Based on the comments in the original patch:
> >
> > https://patchwork.kernel.org/patch/10353197/
> > """
> > +/* this is expected to be used as a function pointer, just define as NULL */
> > +#define pci_sriov_configure_simple NULL
> > """
> >
> > This function could be defined as NULL just because
> > it was expected to be used as a function pointer.
> > But actually it could be called directly as a
> > function, just like this case.
> >
> > So I prefer to make this function always available
> > just like other sriov functions.
> >
> > Best regards,
> > Tiwei Bie
> 
> The fact that you are having to add additional code kind of implies
> that maybe this doesn't fall into the pci_sriov_configure_simple case
> anymore. The PF itself is defining what the VF can and can't do via
> the feature flags you are testing for.

I think you're right about pci_sriov_configure_simple
isn't designed for this case. I dropped the use of
pci_sriov_configure_simple in v2. Thanks!

> 
> For example how is the bit of code below valid if the kernel itself
> doesn't support SR-IOV:
> +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);
> +}
> +
> 
> It really seems like we should be wrapping these functions at the very
> minimum so that they don't imply you have SR-IOV support when it isn't
> supported in the kernel.

I think it's OK to accept this feature bit in this case.
The IOV support not enabled in kernel just means there is
no way for users to use SR-IOV. But it doesn't mean that
the virtio driver doesn't understand *this feature bit*.
Even if the IOV support is enabled in kernel, there is
still no way for virtio driver to know whether users will
enable VFs or not. Accepting this feature is to tell the
device the virtio driver understands this feature (i.e.
this is not an incompatible virtio driver). And it's not
to tell the device whether users will enable VFs or not,
or how many VFs will be enabled.

> 
> Also it seems like we should be disabling the VFs if the driver is
> unbound from this interface. We need to add logic to disable SR-IOV if
> the driver is removed. What we don't want to do is leave VFs allocated
> and then have the potential for us to unbind/rebind the driver as the
> new driver may change the negotiated features.

Right. Thanks!

FYI, here is v2:
https://lists.oasis-open.org/archives/virtio-dev/201805/msg00206.html

Best regards,
Tiwei Bie