* [PATCH] virtio_pci: support enabling VFs
@ 2018-05-30 8:55 Tiwei Bie
0 siblings, 0 replies; 19+ messages in thread
From: Tiwei Bie @ 2018-05-30 8:55 UTC (permalink / raw)
To: mst, bhelgaas, virtualization, linux-kernel, virtio-dev, linux-pci
Cc: alexander.h.duyck, mark.d.rustad, zhihong.wang
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH] virtio_pci: support enabling VFs
@ 2018-05-30 8:55 Tiwei Bie
2018-05-30 12:41 ` Michael S. Tsirkin
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Tiwei Bie @ 2018-05-30 8:55 UTC (permalink / raw)
To: mst, bhelgaas, virtualization, linux-kernel, virtio-dev, linux-pci
Cc: dan.daly, mark.d.rustad, alexander.h.duyck, cunming.liang, zhihong.wang
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 8:55 Tiwei Bie
@ 2018-05-30 12:41 ` Michael S. Tsirkin
2018-05-30 12:41 ` Michael S. Tsirkin
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2018-05-30 12:41 UTC (permalink / raw)
To: Tiwei Bie
Cc: alexander.h.duyck, virtio-dev, linux-pci, linux-kernel,
virtualization, zhihong.wang, bhelgaas, mark.d.rustad
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 8:55 Tiwei Bie
2018-05-30 12:41 ` Michael S. Tsirkin
@ 2018-05-30 12:41 ` Michael S. Tsirkin
2018-05-30 16:22 ` Michael S. Tsirkin
2018-05-30 16:22 ` Michael S. Tsirkin
3 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2018-05-30 12:41 UTC (permalink / raw)
To: Tiwei Bie
Cc: bhelgaas, virtualization, linux-kernel, virtio-dev, linux-pci,
dan.daly, mark.d.rustad, alexander.h.duyck, cunming.liang,
zhihong.wang
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 8:55 Tiwei Bie
2018-05-30 12:41 ` Michael S. Tsirkin
2018-05-30 12:41 ` Michael S. Tsirkin
@ 2018-05-30 16:22 ` Michael S. Tsirkin
2018-05-30 16:44 ` Rustad, Mark D
2018-05-30 16:22 ` Michael S. Tsirkin
3 siblings, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2018-05-30 16:22 UTC (permalink / raw)
To: Tiwei Bie
Cc: bhelgaas, virtualization, linux-kernel, virtio-dev, linux-pci,
dan.daly, mark.d.rustad, alexander.h.duyck, cunming.liang,
zhihong.wang
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 16:22 ` Michael S. Tsirkin
@ 2018-05-30 16:44 ` Rustad, Mark D
0 siblings, 0 replies; 19+ messages in thread
From: Rustad, Mark D @ 2018-05-30 16:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Bie, Tiwei, Bjorn Helgaas, virtualization, linux-kernel,
virtio-dev, linux-pci, Daly, Dan, Duyck, Alexander H, Liang,
Cunming, Wang, Zhihong
[-- 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 --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
@ 2018-05-30 16:44 ` Rustad, Mark D
0 siblings, 0 replies; 19+ messages in thread
From: Rustad, Mark D @ 2018-05-30 16:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Duyck, Alexander H, virtio-dev, linux-pci, linux-kernel,
virtualization, Wang, Zhihong, Bjorn Helgaas
[-- Attachment #1.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 #1.2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 16:44 ` Rustad, Mark D
@ 2018-05-30 16:54 ` Duyck, Alexander H
-1 siblings, 0 replies; 19+ messages in thread
From: Duyck, Alexander H @ 2018-05-30 16:54 UTC (permalink / raw)
To: Rustad, Mark D, mst
Cc: linux-kernel, virtualization, virtio-dev, Daly, Dan, Bie, Tiwei,
linux-pci, bhelgaas, Liang, Cunming, Wang, Zhihong
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
@ 2018-05-30 16:54 ` Duyck, Alexander H
0 siblings, 0 replies; 19+ messages in thread
From: Duyck, Alexander H @ 2018-05-30 16:54 UTC (permalink / raw)
To: Rustad, Mark D, mst
Cc: linux-kernel, virtualization, virtio-dev, Daly, Dan, Bie, Tiwei,
linux-pci, bhelgaas, Liang, Cunming, Wang, Zhihong
T24gV2VkLCAyMDE4LTA1LTMwIGF0IDA5OjQ0IC0wNzAwLCBSdXN0YWQsIE1hcmsgRCB3cm90ZToN
Cj4gT24gTWF5IDMwLCAyMDE4LCBhdCA5OjIyIEFNLCBNaWNoYWVsIFMuIFRzaXJraW4gPG1zdEBy
ZWRoYXQuY29tPiB3cm90ZToNCj4gDQo+ID4gPiArc3RhdGljIGludCB2aXJ0aW9fcGNpX3NyaW92
X2NvbmZpZ3VyZShzdHJ1Y3QgcGNpX2RldiAqcGNpX2RldiwgaW50ICANCj4gPiA+IG51bV92ZnMp
DQo+ID4gPiArew0KPiA+ID4gKwlzdHJ1Y3QgdmlydGlvX3BjaV9kZXZpY2UgKnZwX2RldiA9IHBj
aV9nZXRfZHJ2ZGF0YShwY2lfZGV2KTsNCj4gPiA+ICsJc3RydWN0IHZpcnRpb19kZXZpY2UgKnZk
ZXYgPSAmdnBfZGV2LT52ZGV2Ow0KPiA+ID4gKwlpbnQgKCpzcmlvdl9jb25maWd1cmUpKHN0cnVj
dCBwY2lfZGV2ICpwY2lfZGV2LCBpbnQgbnVtX3Zmcyk7DQo+ID4gPiArDQo+ID4gPiArCWlmICgh
KHZkZXYtPmNvbmZpZy0+Z2V0X3N0YXR1cyh2ZGV2KSAmIFZJUlRJT19DT05GSUdfU19EUklWRVJf
T0spKQ0KPiA+ID4gKwkJcmV0dXJuIC1FQlVTWTsNCj4gPiA+ICsNCj4gPiA+ICsJaWYgKCFfX3Zp
cnRpb190ZXN0X2JpdCh2ZGV2LCBWSVJUSU9fRl9TUl9JT1YpKQ0KPiA+ID4gKwkJcmV0dXJuIC1F
SU5WQUw7DQo+ID4gPiArDQo+ID4gPiArCXNyaW92X2NvbmZpZ3VyZSA9IHBjaV9zcmlvdl9jb25m
aWd1cmVfc2ltcGxlOw0KPiA+ID4gKwlpZiAoc3Jpb3ZfY29uZmlndXJlID09IE5VTEwpDQo+ID4g
PiArCQlyZXR1cm4gLUVOT0VOVDsNCj4gPiANCj4gPiBCVFcgd2hhdCBpcyBhbGwgdGhpcyB0cmlj
a2VyeSBpbiBhaWQgb2Y/DQo+IA0KPiBXaGVuIFNSLUlPViBzdXBwb3J0IGlzIG5vdCBjb21waWxl
ZCBpbnRvIHRoZSBrZXJuZWwsICANCj4gcGNpX3NyaW92X2NvbmZpZ3VyZV9zaW1wbGUgaXMgI2Rl
ZmluZWQgYXMgTlVMTC4gVGhpcyBhbGxvd3MgaXQgdG8gY29tcGlsZSAgDQo+IGluIHRoYXQgY2Fz
ZSwgZXZlbiB0aG91Z2ggdGhlcmUgaXMgdXR0ZXJseSBubyB3YXkgZm9yIGl0IHRvIGJlIGNhbGxl
ZCBpbiAgDQo+IHRoYXQgY2FzZS4gSXQgaXMgYW4gYWx0ZXJuYXRpdmUgdG8gI2lmcyBpbiB0aGUg
Y29kZS4NCg0KV2h5IGV2ZW4gaGF2ZSB0aGUgY2FsbCB0aG91Z2g/IEkgd291bGQgd3JhcCBhbGwg
b2YgdGhpcyBpbiBhbiAjaWZkZWYNCmFuZCBzdHJpcCBpdCBvdXQgc2luY2UgeW91IGNvdWxkbid0
IHN1cHBvcnQgU1ItSU9WIGlmIGl0IGlzbid0IHByZXNlbnQNCmluIHRoZSBrZXJuZWwgYW55d2F5
Lg==
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 16:54 ` Duyck, Alexander H
@ 2018-05-30 17:11 ` Rustad, Mark D
-1 siblings, 0 replies; 19+ messages in thread
From: Rustad, Mark D @ 2018-05-30 17:11 UTC (permalink / raw)
To: Duyck, Alexander H
Cc: mst, linux-kernel, virtualization, virtio-dev, Daly, Dan, Bie,
Tiwei, linux-pci, bhelgaas, Liang, Cunming, Wang, Zhihong
[-- 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 --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
@ 2018-05-30 17:11 ` Rustad, Mark D
0 siblings, 0 replies; 19+ messages in thread
From: Rustad, Mark D @ 2018-05-30 17:11 UTC (permalink / raw)
To: Duyck, Alexander H
Cc: virtio-dev, mst, linux-pci, linux-kernel, virtualization, Wang,
Zhihong, bhelgaas
[-- Attachment #1.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 #1.2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 17:11 ` Rustad, Mark D
(?)
@ 2018-05-31 3:20 ` Tiwei Bie
2018-05-31 14:27 ` Alexander Duyck
2018-05-31 14:27 ` Alexander Duyck
-1 siblings, 2 replies; 19+ messages in thread
From: Tiwei Bie @ 2018-05-31 3:20 UTC (permalink / raw)
To: Duyck, Alexander H, Rustad, Mark D
Cc: mst, linux-kernel, virtualization, virtio-dev, Daly, Dan,
linux-pci, bhelgaas, Liang, Cunming, Wang, Zhihong
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-31 3:20 ` Tiwei Bie
@ 2018-05-31 14:27 ` Alexander Duyck
2018-05-31 14:27 ` Alexander Duyck
1 sibling, 0 replies; 19+ messages in thread
From: Alexander Duyck @ 2018-05-31 14:27 UTC (permalink / raw)
To: Tiwei Bie
Cc: Duyck, Alexander H, virtio-dev, mst, linux-pci, linux-kernel,
virtualization, Wang, Zhihong, bhelgaas, Rustad, Mark D
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-31 3:20 ` Tiwei Bie
2018-05-31 14:27 ` Alexander Duyck
@ 2018-05-31 14:27 ` Alexander Duyck
2018-06-01 2:30 ` Tiwei Bie
2018-06-01 2:30 ` Tiwei Bie
1 sibling, 2 replies; 19+ messages in thread
From: Alexander Duyck @ 2018-05-31 14:27 UTC (permalink / raw)
To: Tiwei Bie
Cc: Duyck, Alexander H, Rustad, Mark D, mst, linux-kernel,
virtualization, virtio-dev, Daly, Dan, linux-pci, bhelgaas,
Liang, Cunming, Wang, Zhihong
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-31 14:27 ` Alexander Duyck
@ 2018-06-01 2:30 ` Tiwei Bie
2018-06-01 2:30 ` Tiwei Bie
1 sibling, 0 replies; 19+ messages in thread
From: Tiwei Bie @ 2018-06-01 2:30 UTC (permalink / raw)
To: Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, mst, linux-pci, linux-kernel,
virtualization, Wang, Zhihong, bhelgaas, Rustad, Mark D
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-31 14:27 ` Alexander Duyck
2018-06-01 2:30 ` Tiwei Bie
@ 2018-06-01 2:30 ` Tiwei Bie
1 sibling, 0 replies; 19+ messages in thread
From: Tiwei Bie @ 2018-06-01 2:30 UTC (permalink / raw)
To: Alexander Duyck
Cc: Duyck, Alexander H, Rustad, Mark D, mst, linux-kernel,
virtualization, virtio-dev, Daly, Dan, linux-pci, bhelgaas,
Liang, Cunming, Wang, Zhihong
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 17:11 ` Rustad, Mark D
(?)
(?)
@ 2018-05-31 3:20 ` Tiwei Bie
-1 siblings, 0 replies; 19+ messages in thread
From: Tiwei Bie @ 2018-05-31 3:20 UTC (permalink / raw)
To: Duyck, Alexander H, Rustad, Mark D
Cc: virtio-dev, mst, linux-pci, linux-kernel, virtualization, Wang,
Zhihong, bhelgaas
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 16:44 ` Rustad, Mark D
(?)
(?)
@ 2018-05-30 16:54 ` Duyck, Alexander H
-1 siblings, 0 replies; 19+ messages in thread
From: Duyck, Alexander H @ 2018-05-30 16:54 UTC (permalink / raw)
To: Rustad, Mark D, mst
Cc: virtio-dev, linux-pci, linux-kernel, virtualization, Wang,
Zhihong, bhelgaas
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] virtio_pci: support enabling VFs
2018-05-30 8:55 Tiwei Bie
` (2 preceding siblings ...)
2018-05-30 16:22 ` Michael S. Tsirkin
@ 2018-05-30 16:22 ` Michael S. Tsirkin
3 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2018-05-30 16:22 UTC (permalink / raw)
To: Tiwei Bie
Cc: alexander.h.duyck, virtio-dev, linux-pci, linux-kernel,
virtualization, zhihong.wang, bhelgaas, mark.d.rustad
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
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-06-01 2:30 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-30 8:55 [PATCH] virtio_pci: support enabling VFs Tiwei Bie
2018-05-30 8:55 Tiwei Bie
2018-05-30 12:41 ` Michael S. Tsirkin
2018-05-30 12:41 ` Michael S. Tsirkin
2018-05-30 16:22 ` Michael S. Tsirkin
2018-05-30 16:44 ` Rustad, Mark D
2018-05-30 16:44 ` Rustad, Mark D
2018-05-30 16:54 ` Duyck, Alexander H
2018-05-30 16:54 ` Duyck, Alexander H
2018-05-30 17:11 ` Rustad, Mark D
2018-05-30 17:11 ` Rustad, Mark D
2018-05-31 3:20 ` Tiwei Bie
2018-05-31 14:27 ` Alexander Duyck
2018-05-31 14:27 ` Alexander Duyck
2018-06-01 2:30 ` Tiwei Bie
2018-06-01 2:30 ` Tiwei Bie
2018-05-31 3:20 ` Tiwei Bie
2018-05-30 16:54 ` Duyck, Alexander H
2018-05-30 16:22 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.