[dpdk-dev] [PATCH] vhost: disable reply-ack protocol feature if iommu feature disabled

[dpdk-dev] [PATCH] vhost: disable reply-ack protocol feature if iommu feature disabled

[Maxime Coquelin]

If the application has disabled VIRTIO_F_IOMMU_PLATFORM, disable
VHOST_USER_PROTOCOL_F_REPLY_ACK protocol feature that is only
mandatory with IOMMU for now.

This is done to provide a way for the application to support
multiqueue with old Qemu versions (v2.7.0 to v2.9.0) that have
reply-ack feature broken.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---

This is an alternative to my proposition of adding a new flag at
vhost register time. Advantage of this solution is that it does
not bring API change.

 lib/librte_vhost/vhost_user.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 1f6cba4b9..e35218688 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -878,6 +878,27 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
 }
 
 static void
+vhost_user_get_protocol_features(struct virtio_net *dev,
+				 struct VhostUserMsg *msg)
+{
+	uint64_t features, protocol_features = VHOST_USER_PROTOCOL_FEATURES;
+
+	rte_vhost_driver_get_features(dev->ifname, &features);
+
+	/*
+	 * REPLY_ACK protocol feature is only mandatory for now
+	 * for IOMMU feature. If IOMMU is explicitly disabled by the
+	 * application, disable also REPLY_ACK feature for older buggy
+	 * Qemu versions (from v2.7.0 to v2.9.0).
+	 */
+	if (!(features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)))
+		protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+	msg->payload.u64 = protocol_features;
+	msg->size = sizeof(msg->payload.u64);
+}
+
+static void
 vhost_user_set_protocol_features(struct virtio_net *dev,
 				 uint64_t protocol_features)
 {
@@ -1248,8 +1269,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
-		msg.payload.u64 = VHOST_USER_PROTOCOL_FEATURES;
-		msg.size = sizeof(msg.payload.u64);
+		vhost_user_get_protocol_features(dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
-- 
2.13.6

Re: [dpdk-dev] [PATCH] vhost: disable reply-ack protocol feature if iommu feature disabled

[Kavanagh, Mark B]

>From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>Sent: Friday, November 3, 2017 5:58 PM
>To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B
><mark.b.kavanagh@intel.com>; thomas@monjalon.net; ktraynor@redhat.com
>Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>Subject: [PATCH] vhost: disable reply-ack protocol feature if iommu feature
>disabled
>
>If the application has disabled VIRTIO_F_IOMMU_PLATFORM, disable
>VHOST_USER_PROTOCOL_F_REPLY_ACK protocol feature that is only
>mandatory with IOMMU for now.
>
>This is done to provide a way for the application to support
>multiqueue with old Qemu versions (v2.7.0 to v2.9.0) that have
>reply-ack feature broken.
>
>Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>---
>
>This is an alternative to my proposition of adding a new flag at
>vhost register time. Advantage of this solution is that it does
>not bring API change.
>
> lib/librte_vhost/vhost_user.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
>diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>index 1f6cba4b9..e35218688 100644
>--- a/lib/librte_vhost/vhost_user.c
>+++ b/lib/librte_vhost/vhost_user.c
>@@ -878,6 +878,27 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
> }
>
> static void
>+vhost_user_get_protocol_features(struct virtio_net *dev,
>+				 struct VhostUserMsg *msg)
>+{
>+	uint64_t features, protocol_features = VHOST_USER_PROTOCOL_FEATURES;
>+
>+	rte_vhost_driver_get_features(dev->ifname, &features);
>+
>+	/*
>+	 * REPLY_ACK protocol feature is only mandatory for now
>+	 * for IOMMU feature. If IOMMU is explicitly disabled by the
>+	 * application, disable also REPLY_ACK feature for older buggy
>+	 * Qemu versions (from v2.7.0 to v2.9.0).
>+	 */
>+	if (!(features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)))

Hi Maxime,

Thanks for this patch - I like this approach, as it maintains API compatibility.

Now for the gotchas, unfortunately:
- VIRTIO_F_IOMMU_PLATFORM is defined in vhost.h, as opposed to rte_vhost.h, so it is not exposed to OvS :(
- Currently, we use other similar macros in OvS (VIRTIO_NET_F_CSUM, VIRTIO_NET_F_HOST_TSO[4|6]); however, we obtain the definition of these from a kernel header file, as opposed to a DPDK header (linux/virtio_net.h)
	-- We could adopt the same approach for VIRTIO_F_IOMMU_PLATFORM, and include linux/virtio_config.h; unfortunately, the VIRTIO_F_IOMMU_PLATFORM macro is only defined from kernel 4.8, which creates another problem entirely.

One potential solution is to move the VIRTIO_* definitions to rte_vhost.h, but at this stage in the DPDK release cycle, that's probably a tall ask.

Any thoughts?

Thanks again,
Mark

>+		protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK);
>+
>+	msg->payload.u64 = protocol_features;
>+	msg->size = sizeof(msg->payload.u64);
>+}
>+
>+static void
> vhost_user_set_protocol_features(struct virtio_net *dev,
> 				 uint64_t protocol_features)
> {
>@@ -1248,8 +1269,7 @@ vhost_user_msg_handler(int vid, int fd)
> 		break;
>
> 	case VHOST_USER_GET_PROTOCOL_FEATURES:
>-		msg.payload.u64 = VHOST_USER_PROTOCOL_FEATURES;
>-		msg.size = sizeof(msg.payload.u64);
>+		vhost_user_get_protocol_features(dev, &msg);
> 		send_vhost_reply(fd, &msg);
> 		break;
> 	case VHOST_USER_SET_PROTOCOL_FEATURES:
>--
>2.13.6

[dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Maxime Coquelin]

This series disables IOMMU feature by default, and introduce
a new flag passed at vhost device registration time to enable
it explicitly.

When disabled, patch 1 also disables reply-ack protocol feature
to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.

Last patch adds a Vhost PMD "iommu-support" parameter to enable
the IOMMU feature.

Maxime Coquelin (3):
  vhost: disable reply-ack protocol feature if iommu feature disabled
  vhost: add flag to enable iommu support
  net: vhost: add iommu-support parameter to enable IOMMU feature

 doc/guides/nics/vhost.rst              |  5 +++++
 doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
 doc/guides/rel_notes/release_17_11.rst |  3 ++-
 drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
 lib/librte_vhost/rte_vhost.h           |  1 +
 lib/librte_vhost/socket.c              |  6 ++++++
 lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
 7 files changed, 63 insertions(+), 3 deletions(-)

-- 
2.13.6

[dpdk-dev] [PATCH v2 1/3] vhost: disable reply-ack protocol feature if iommu feature disabled

[Maxime Coquelin]

If the application has disabled VIRTIO_F_IOMMU_PLATFORM, disable
VHOST_USER_PROTOCOL_F_REPLY_ACK protocol feature that is only
mandatory with IOMMU for now.

This is done to provide a way for the application to support
multiqueue with old Qemu versions (v2.7.0 to v2.9.0) that have
reply-ack feature broken.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 1f6cba4b9..e35218688 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -878,6 +878,27 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
 }
 
 static void
+vhost_user_get_protocol_features(struct virtio_net *dev,
+				 struct VhostUserMsg *msg)
+{
+	uint64_t features, protocol_features = VHOST_USER_PROTOCOL_FEATURES;
+
+	rte_vhost_driver_get_features(dev->ifname, &features);
+
+	/*
+	 * REPLY_ACK protocol feature is only mandatory for now
+	 * for IOMMU feature. If IOMMU is explicitly disabled by the
+	 * application, disable also REPLY_ACK feature for older buggy
+	 * Qemu versions (from v2.7.0 to v2.9.0).
+	 */
+	if (!(features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)))
+		protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+	msg->payload.u64 = protocol_features;
+	msg->size = sizeof(msg->payload.u64);
+}
+
+static void
 vhost_user_set_protocol_features(struct virtio_net *dev,
 				 uint64_t protocol_features)
 {
@@ -1248,8 +1269,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
-		msg.payload.u64 = VHOST_USER_PROTOCOL_FEATURES;
-		msg.size = sizeof(msg.payload.u64);
+		vhost_user_get_protocol_features(dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
-- 
2.13.6

[dpdk-dev] [PATCH v2 2/3] vhost: add flag to enable iommu support

[Maxime Coquelin]

Qemu versions from v2.7.0 to v2.9.0 have their reply-ack protocol
feature implementation broken with multiqueue. The reply-ack
protocol feature is optional except for IOMMU feature.

This patch introduce a new RTE_VHOST_USER_IOMMU_SUPPORT flag to
enable VIRTIO_F_IOMMU_PLATFORM virtio feature.

By default, the IOMMU support is now disabled.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
 doc/guides/rel_notes/release_17_11.rst |  3 ++-
 lib/librte_vhost/rte_vhost.h           |  1 +
 lib/librte_vhost/socket.c              |  6 ++++++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index d02e3cf50..e71bdbd51 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -110,6 +110,20 @@ The following is an overview of some key Vhost API functions:
       of those segments, thus the fewer the segments, the quicker we will get
       the mapping. NOTE: we may speed it by using tree searching in future.
 
+  - ``RTE_VHOST_USER_IOMMU_SUPPORT``
+
+    IOMMU support will be enabled when this flag is set. It is disabled by
+    default.
+
+    Enabling this flag makes possible to use guest vIOMMU to protect vhost
+    from accessing memory the virtio device isn't allowed to, when the feature
+    is negotiated and an IOMMU device is declared.
+
+    However, this feature enables vhost-user's reply-ack protocol feature,
+    which implementation is buggy in Qemu v2.7.0-v2.9.0 when doing multiqueue.
+    Enabling this flag with these Qemu version results in Qemu being blocked
+    when multiple queue pairs are declared.
+
 * ``rte_vhost_driver_set_features(path, features)``
 
   This function sets the feature bits the vhost-user driver supports. The
diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst
index b96b23614..06f2a302c 100644
--- a/doc/guides/rel_notes/release_17_11.rst
+++ b/doc/guides/rel_notes/release_17_11.rst
@@ -162,7 +162,8 @@ New Features
 * **Added IOMMU support to libvhost-user**
 
   Implemented device IOTLB in Vhost-user backend, and enabled Virtio's IOMMU
-  feature.
+  feature. The feature is disabled by default, and can be enabled by setting
+  RTE_VHOST_USER_IOMMU_SUPPORT flag at vhost device registration time.
 
 * **Added the Event Ethernet Adapter Library.**
 
diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index fe5c94c69..f65364495 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -56,6 +56,7 @@ extern "C" {
 #define RTE_VHOST_USER_CLIENT		(1ULL << 0)
 #define RTE_VHOST_USER_NO_RECONNECT	(1ULL << 1)
 #define RTE_VHOST_USER_DEQUEUE_ZERO_COPY	(1ULL << 2)
+#define RTE_VHOST_USER_IOMMU_SUPPORT	(1ULL << 3)
 
 /**
  * Information relating to memory regions including offsets to
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 701815021..422da002f 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -68,6 +68,7 @@ struct vhost_user_socket {
 	bool is_server;
 	bool reconnect;
 	bool dequeue_zero_copy;
+	bool iommu_support;
 
 	/*
 	 * The "supported_features" indicates the feature bits the
@@ -669,6 +670,11 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
 	vsocket->supported_features = VIRTIO_NET_SUPPORTED_FEATURES;
 	vsocket->features           = VIRTIO_NET_SUPPORTED_FEATURES;
 
+	if (!(flags & RTE_VHOST_USER_IOMMU_SUPPORT)) {
+		vsocket->supported_features &= ~(1ULL << VIRTIO_F_IOMMU_PLATFORM);
+		vsocket->features &= ~(1ULL << VIRTIO_F_IOMMU_PLATFORM);
+	}
+
 	if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
 		vsocket->reconnect = !(flags & RTE_VHOST_USER_NO_RECONNECT);
 		if (vsocket->reconnect && reconn_tid == 0) {
-- 
2.13.6

[dpdk-dev] [PATCH v2 3/3] net: vhost: add iommu-support parameter to enable IOMMU feature

[Maxime Coquelin]

Introduce a new iommu-support parameter to Vhost PMD that
passes the RTE_VHOST_USER_IOMMU_SUPPORT flag at vhost
device register time.

Default value is 0, meaning that IOMMU support is disabled
if not specified explicitly.

Example to enable IOMMU support for a given device:

--vdev 'net_vhost0,iface=/tmp/vhost-user2,iommu-support=1'

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 doc/guides/nics/vhost.rst         |  5 +++++
 drivers/net/vhost/rte_eth_vhost.c | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
index e651a1661..4f7ae8990 100644
--- a/doc/guides/nics/vhost.rst
+++ b/doc/guides/nics/vhost.rst
@@ -66,6 +66,11 @@ The user can specify below arguments in `--vdev` option.
     It is used to specify the number of queues virtio-net device has.
     (Default: 1)
 
+#.  ``iommu-support``:
+
+    It is used to enable iommu support in vhost library.
+    (Default: 0 (disabled))
+
 Vhost PMD event handling
 ------------------------
 
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index f98c98067..a28cc3b9a 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -52,6 +52,7 @@ enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
 #define ETH_VHOST_QUEUES_ARG		"queues"
 #define ETH_VHOST_CLIENT_ARG		"client"
 #define ETH_VHOST_DEQUEUE_ZERO_COPY	"dequeue-zero-copy"
+#define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
 #define VHOST_MAX_PKT_BURST 32
 
 static const char *valid_arguments[] = {
@@ -59,6 +60,7 @@ static const char *valid_arguments[] = {
 	ETH_VHOST_QUEUES_ARG,
 	ETH_VHOST_CLIENT_ARG,
 	ETH_VHOST_DEQUEUE_ZERO_COPY,
+	ETH_VHOST_IOMMU_SUPPORT,
 	NULL
 };
 
@@ -1164,6 +1166,7 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
 	uint64_t flags = 0;
 	int client_mode = 0;
 	int dequeue_zero_copy = 0;
+	int iommu_support = 0;
 
 	RTE_LOG(INFO, PMD, "Initializing pmd_vhost for %s\n",
 		rte_vdev_device_name(dev));
@@ -1211,6 +1214,16 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
 			flags |= RTE_VHOST_USER_DEQUEUE_ZERO_COPY;
 	}
 
+	if (rte_kvargs_count(kvlist, ETH_VHOST_IOMMU_SUPPORT) == 1) {
+		ret = rte_kvargs_process(kvlist, ETH_VHOST_IOMMU_SUPPORT,
+					 &open_int, &iommu_support);
+		if (ret < 0)
+			goto out_free;
+
+		if (iommu_support)
+			flags |= RTE_VHOST_USER_IOMMU_SUPPORT;
+	}
+
 	if (dev->device.numa_node == SOCKET_ID_ANY)
 		dev->device.numa_node = rte_socket_id();
 
-- 
2.13.6

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Yuanhan Liu]

On Mon, Nov 06, 2017 at 09:38:09PM +0100, Maxime Coquelin wrote:
> This series disables IOMMU feature by default, and introduce
> a new flag passed at vhost device registration time to enable
> it explicitly.
> 
> When disabled, patch 1 also disables reply-ack protocol feature
> to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
> 
> Last patch adds a Vhost PMD "iommu-support" parameter to enable
> the IOMMU feature.

Series Acked-by: Yuanhan Liu <yliu@fridaylinux.org>

Thanks!

	--yliu
> 
> Maxime Coquelin (3):
>   vhost: disable reply-ack protocol feature if iommu feature disabled
>   vhost: add flag to enable iommu support
>   net: vhost: add iommu-support parameter to enable IOMMU feature
> 
>  doc/guides/nics/vhost.rst              |  5 +++++
>  doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
>  doc/guides/rel_notes/release_17_11.rst |  3 ++-
>  drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
>  lib/librte_vhost/rte_vhost.h           |  1 +
>  lib/librte_vhost/socket.c              |  6 ++++++
>  lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
>  7 files changed, 63 insertions(+), 3 deletions(-)
> 
> -- 
> 2.13.6

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Thomas Monjalon]

07/11/2017 04:32, Yuanhan Liu:
> On Mon, Nov 06, 2017 at 09:38:09PM +0100, Maxime Coquelin wrote:
> > This series disables IOMMU feature by default, and introduce
> > a new flag passed at vhost device registration time to enable
> > it explicitly.
> > 
> > When disabled, patch 1 also disables reply-ack protocol feature
> > to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
> > 
> > Last patch adds a Vhost PMD "iommu-support" parameter to enable
> > the IOMMU feature.
> 
> Series Acked-by: Yuanhan Liu <yliu@fridaylinux.org>

Applied, thanks

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Kavanagh, Mark B]

>From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>Sent: Monday, November 6, 2017 8:38 PM
>To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B
><mark.b.kavanagh@intel.com>; thomas@monjalon.net; ktraynor@redhat.com
>Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>Subject: [PATCH v2 0/3] vhost: disable iommu support by default
>
>This series disables IOMMU feature by default, and introduce
>a new flag passed at vhost device registration time to enable
>it explicitly.
>
>When disabled, patch 1 also disables reply-ack protocol feature
>to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
>
>Last patch adds a Vhost PMD "iommu-support" parameter to enable
>the IOMMU feature.

Hi Maxime,

I'm happy to confirm that this patchset resolves the vhost user mutltiq issue for OvS-DPDK, with QEMU v2.7.1.

Additionally, all of the individual patches look good - thanks for all of your efforts on this!

Tested-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com>

Cheers,
Mark

>
>Maxime Coquelin (3):
>  vhost: disable reply-ack protocol feature if iommu feature disabled
>  vhost: add flag to enable iommu support
>  net: vhost: add iommu-support parameter to enable IOMMU feature
>
> doc/guides/nics/vhost.rst              |  5 +++++
> doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
> doc/guides/rel_notes/release_17_11.rst |  3 ++-
> drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
> lib/librte_vhost/rte_vhost.h           |  1 +
> lib/librte_vhost/socket.c              |  6 ++++++
> lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
> 7 files changed, 63 insertions(+), 3 deletions(-)
>
>--
>2.13.6

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Maxime Coquelin]

Hi Mark,

On 11/07/2017 11:56 AM, Kavanagh, Mark B wrote:
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Monday, November 6, 2017 8:38 PM
>> To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B
>> <mark.b.kavanagh@intel.com>; thomas@monjalon.net; ktraynor@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH v2 0/3] vhost: disable iommu support by default
>>
>> This series disables IOMMU feature by default, and introduce
>> a new flag passed at vhost device registration time to enable
>> it explicitly.
>>
>> When disabled, patch 1 also disables reply-ack protocol feature
>> to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
>>
>> Last patch adds a Vhost PMD "iommu-support" parameter to enable
>> the IOMMU feature.
> 
> Hi Maxime,
> 
> I'm happy to confirm that this patchset resolves the vhost user mutltiq issue for OvS-DPDK, with QEMU v2.7.1.

Thanks for the testing.

> Additionally, all of the individual patches look good - thanks for all of your efforts on this!

Great.
Now, what is required on OVS side is the introduction of a new vhost
port option to enable IOMMU support, so that management layer has a way
to enable it when VM has an iommu placed in front of the virtio device.

Note that OVS can set the flag even if no IOMMU is present, as Virtio
feature negotiation will manage this.

> Tested-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com>

Thanks,
Maxime

> Cheers,
> Mark
> 
>>
>> Maxime Coquelin (3):
>>   vhost: disable reply-ack protocol feature if iommu feature disabled
>>   vhost: add flag to enable iommu support
>>   net: vhost: add iommu-support parameter to enable IOMMU feature
>>
>> doc/guides/nics/vhost.rst              |  5 +++++
>> doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
>> doc/guides/rel_notes/release_17_11.rst |  3 ++-
>> drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
>> lib/librte_vhost/rte_vhost.h           |  1 +
>> lib/librte_vhost/socket.c              |  6 ++++++
>> lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
>> 7 files changed, 63 insertions(+), 3 deletions(-)
>>
>> --
>> 2.13.6
> 

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Kavanagh, Mark B]

>From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>Sent: Tuesday, November 7, 2017 11:05 AM
>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@dpdk.org;
>yliu@fridaylinux.org; thomas@monjalon.net; ktraynor@redhat.com
>Subject: Re: [PATCH v2 0/3] vhost: disable iommu support by default
>
>Hi Mark,
>
>On 11/07/2017 11:56 AM, Kavanagh, Mark B wrote:
>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>> Sent: Monday, November 6, 2017 8:38 PM
>>> To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B
>>> <mark.b.kavanagh@intel.com>; thomas@monjalon.net; ktraynor@redhat.com
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: [PATCH v2 0/3] vhost: disable iommu support by default
>>>
>>> This series disables IOMMU feature by default, and introduce
>>> a new flag passed at vhost device registration time to enable
>>> it explicitly.
>>>
>>> When disabled, patch 1 also disables reply-ack protocol feature
>>> to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
>>>
>>> Last patch adds a Vhost PMD "iommu-support" parameter to enable
>>> the IOMMU feature.
>>
>> Hi Maxime,
>>
>> I'm happy to confirm that this patchset resolves the vhost user mutltiq
>issue for OvS-DPDK, with QEMU v2.7.1.
>
>Thanks for the testing.
>
>> Additionally, all of the individual patches look good - thanks for all of
>your efforts on this!
>
>Great.
>Now, what is required on OVS side is the introduction of a new vhost
>port option to enable IOMMU support, so that management layer has a way
>to enable it when VM has an iommu placed in front of the virtio device.

Sounds good - I can add this as part of the DPDK v17.11 upgrade patch :)

Thanks again,
Mark


>
>Note that OVS can set the flag even if no IOMMU is present, as Virtio
>feature negotiation will manage this.
>
>> Tested-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>
>Thanks,
>Maxime
>
>> Cheers,
>> Mark
>>
>>>
>>> Maxime Coquelin (3):
>>>   vhost: disable reply-ack protocol feature if iommu feature disabled
>>>   vhost: add flag to enable iommu support
>>>   net: vhost: add iommu-support parameter to enable IOMMU feature
>>>
>>> doc/guides/nics/vhost.rst              |  5 +++++
>>> doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
>>> doc/guides/rel_notes/release_17_11.rst |  3 ++-
>>> drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
>>> lib/librte_vhost/rte_vhost.h           |  1 +
>>> lib/librte_vhost/socket.c              |  6 ++++++
>>> lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
>>> 7 files changed, 63 insertions(+), 3 deletions(-)
>>>
>>> --
>>> 2.13.6
>>

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Maxime Coquelin]

On 11/07/2017 12:08 PM, Kavanagh, Mark B wrote:
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Tuesday, November 7, 2017 11:05 AM
>> To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@dpdk.org;
>> yliu@fridaylinux.org; thomas@monjalon.net; ktraynor@redhat.com
>> Subject: Re: [PATCH v2 0/3] vhost: disable iommu support by default
>>
>> Hi Mark,
>>
>> On 11/07/2017 11:56 AM, Kavanagh, Mark B wrote:
>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>> Sent: Monday, November 6, 2017 8:38 PM
>>>> To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B
>>>> <mark.b.kavanagh@intel.com>; thomas@monjalon.net; ktraynor@redhat.com
>>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Subject: [PATCH v2 0/3] vhost: disable iommu support by default
>>>>
>>>> This series disables IOMMU feature by default, and introduce
>>>> a new flag passed at vhost device registration time to enable
>>>> it explicitly.
>>>>
>>>> When disabled, patch 1 also disables reply-ack protocol feature
>>>> to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
>>>>
>>>> Last patch adds a Vhost PMD "iommu-support" parameter to enable
>>>> the IOMMU feature.
>>>
>>> Hi Maxime,
>>>
>>> I'm happy to confirm that this patchset resolves the vhost user mutltiq
>> issue for OvS-DPDK, with QEMU v2.7.1.
>>
>> Thanks for the testing.
>>
>>> Additionally, all of the individual patches look good - thanks for all of
>> your efforts on this!
>>
>> Great.
>> Now, what is required on OVS side is the introduction of a new vhost
>> port option to enable IOMMU support, so that management layer has a way
>> to enable it when VM has an iommu placed in front of the virtio device.
> 
> Sounds good - I can add this as part of the DPDK v17.11 upgrade patch :)

Thanks :) Please add me in cc when you'll post the change, so that I can
keep track of it.

Regards,
Maxime
> Thanks again,
> Mark
> 
> 
>>
>> Note that OVS can set the flag even if no IOMMU is present, as Virtio
>> feature negotiation will manage this.
>>
>>> Tested-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>> Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>
>> Thanks,
>> Maxime
>>
>>> Cheers,
>>> Mark
>>>
>>>>
>>>> Maxime Coquelin (3):
>>>>    vhost: disable reply-ack protocol feature if iommu feature disabled
>>>>    vhost: add flag to enable iommu support
>>>>    net: vhost: add iommu-support parameter to enable IOMMU feature
>>>>
>>>> doc/guides/nics/vhost.rst              |  5 +++++
>>>> doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
>>>> doc/guides/rel_notes/release_17_11.rst |  3 ++-
>>>> drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
>>>> lib/librte_vhost/rte_vhost.h           |  1 +
>>>> lib/librte_vhost/socket.c              |  6 ++++++
>>>> lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
>>>> 7 files changed, 63 insertions(+), 3 deletions(-)
>>>>
>>>> --
>>>> 2.13.6
>>>

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Kevin Traynor]

On 11/07/2017 10:56 AM, Kavanagh, Mark B wrote:
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Monday, November 6, 2017 8:38 PM
>> To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B
>> <mark.b.kavanagh@intel.com>; thomas@monjalon.net; ktraynor@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH v2 0/3] vhost: disable iommu support by default
>>
>> This series disables IOMMU feature by default, and introduce
>> a new flag passed at vhost device registration time to enable
>> it explicitly.
>>
>> When disabled, patch 1 also disables reply-ack protocol feature
>> to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
>>
>> Last patch adds a Vhost PMD "iommu-support" parameter to enable
>> the IOMMU feature.
> 

Hi Maxime - OVS-DPDK does not use the vhost pmd, so that means that
iommu could not be used with OVS-DPDK at present. Did I get that right?

Ciara proposed patches for vhost pmd in OVS-DPDK but it is quite an
intrusive change and has been postponed multiple times due to various
issues.

thanks,
Kevin.

> Hi Maxime,
> 
> I'm happy to confirm that this patchset resolves the vhost user mutltiq issue for OvS-DPDK, with QEMU v2.7.1.
> 
> Additionally, all of the individual patches look good - thanks for all of your efforts on this!
> 
> Tested-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> 
> Cheers,
> Mark
> 
>>
>> Maxime Coquelin (3):
>>  vhost: disable reply-ack protocol feature if iommu feature disabled
>>  vhost: add flag to enable iommu support
>>  net: vhost: add iommu-support parameter to enable IOMMU feature
>>
>> doc/guides/nics/vhost.rst              |  5 +++++
>> doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
>> doc/guides/rel_notes/release_17_11.rst |  3 ++-
>> drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
>> lib/librte_vhost/rte_vhost.h           |  1 +
>> lib/librte_vhost/socket.c              |  6 ++++++
>> lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
>> 7 files changed, 63 insertions(+), 3 deletions(-)
>>
>> --
>> 2.13.6
> 

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Maxime Coquelin]

On 11/07/2017 12:25 PM, Kevin Traynor wrote:
> On 11/07/2017 10:56 AM, Kavanagh, Mark B wrote:
>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>> Sent: Monday, November 6, 2017 8:38 PM
>>> To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B
>>> <mark.b.kavanagh@intel.com>; thomas@monjalon.net; ktraynor@redhat.com
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: [PATCH v2 0/3] vhost: disable iommu support by default
>>>
>>> This series disables IOMMU feature by default, and introduce
>>> a new flag passed at vhost device registration time to enable
>>> it explicitly.
>>>
>>> When disabled, patch 1 also disables reply-ack protocol feature
>>> to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
>>>
>>> Last patch adds a Vhost PMD "iommu-support" parameter to enable
>>> the IOMMU feature.
>>

Hi Kevin,

> Hi Maxime - OVS-DPDK does not use the vhost pmd, so that means that
> iommu could not be used with OVS-DPDK at present. Did I get that right?

No :)

This is supported both with and without using Vhost PMD.

When using the Vhost lib directly, you just have to pass
RTE_VHOST_USER_IOMMU_SUPPORT flag to rte_vhost_driver_register(),
this is patch 1.

When using Vhost PMD, passing the iommu-support=1 option to the vdev
cmdline will set this flag, this is patch 2.

Thanks,
Maxime

> Ciara proposed patches for vhost pmd in OVS-DPDK but it is quite an
> intrusive change and has been postponed multiple times due to various
> issues.
>
> thanks,
> Kevin.
> 
>> Hi Maxime,
>>
>> I'm happy to confirm that this patchset resolves the vhost user mutltiq issue for OvS-DPDK, with QEMU v2.7.1.
>>
>> Additionally, all of the individual patches look good - thanks for all of your efforts on this!
>>
>> Tested-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>
>> Cheers,
>> Mark
>>
>>>
>>> Maxime Coquelin (3):
>>>   vhost: disable reply-ack protocol feature if iommu feature disabled
>>>   vhost: add flag to enable iommu support
>>>   net: vhost: add iommu-support parameter to enable IOMMU feature
>>>
>>> doc/guides/nics/vhost.rst              |  5 +++++
>>> doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
>>> doc/guides/rel_notes/release_17_11.rst |  3 ++-
>>> drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
>>> lib/librte_vhost/rte_vhost.h           |  1 +
>>> lib/librte_vhost/socket.c              |  6 ++++++
>>> lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
>>> 7 files changed, 63 insertions(+), 3 deletions(-)
>>>
>>> --
>>> 2.13.6
>>
> 

Re: [dpdk-dev] [PATCH v2 0/3] vhost: disable iommu support by default

[Kevin Traynor]

On 11/07/2017 11:30 AM, Maxime Coquelin wrote:
> 
> 
> On 11/07/2017 12:25 PM, Kevin Traynor wrote:
>> On 11/07/2017 10:56 AM, Kavanagh, Mark B wrote:
>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>> Sent: Monday, November 6, 2017 8:38 PM
>>>> To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B
>>>> <mark.b.kavanagh@intel.com>; thomas@monjalon.net; ktraynor@redhat.com
>>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Subject: [PATCH v2 0/3] vhost: disable iommu support by default
>>>>
>>>> This series disables IOMMU feature by default, and introduce
>>>> a new flag passed at vhost device registration time to enable
>>>> it explicitly.
>>>>
>>>> When disabled, patch 1 also disables reply-ack protocol feature
>>>> to avoid Qemu v2.7.0-v2.9.0 reply-ack bug with multiqueue.
>>>>
>>>> Last patch adds a Vhost PMD "iommu-support" parameter to enable
>>>> the IOMMU feature.
>>>
> 
> Hi Kevin,
> 
>> Hi Maxime - OVS-DPDK does not use the vhost pmd, so that means that
>> iommu could not be used with OVS-DPDK at present. Did I get that right?
> 
> No :)
> 

Good :)

> This is supported both with and without using Vhost PMD.
> 
> When using the Vhost lib directly, you just have to pass
> RTE_VHOST_USER_IOMMU_SUPPORT flag to rte_vhost_driver_register(),
> this is patch 1.
> 
> When using Vhost PMD, passing the iommu-support=1 option to the vdev
> cmdline will set this flag, this is patch 2.
> 

Ah ok, just catching up, thanks for the ABC. It seems like a good
compromise, sorry for the noise.

> Thanks,
> Maxime
> 
>> Ciara proposed patches for vhost pmd in OVS-DPDK but it is quite an
>> intrusive change and has been postponed multiple times due to various
>> issues.
>>
>> thanks,
>> Kevin.
>>
>>> Hi Maxime,
>>>
>>> I'm happy to confirm that this patchset resolves the vhost user
>>> mutltiq issue for OvS-DPDK, with QEMU v2.7.1.
>>>
>>> Additionally, all of the individual patches look good - thanks for
>>> all of your efforts on this!
>>>
>>> Tested-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>> Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>>
>>> Cheers,
>>> Mark
>>>
>>>>
>>>> Maxime Coquelin (3):
>>>>   vhost: disable reply-ack protocol feature if iommu feature disabled
>>>>   vhost: add flag to enable iommu support
>>>>   net: vhost: add iommu-support parameter to enable IOMMU feature
>>>>
>>>> doc/guides/nics/vhost.rst              |  5 +++++
>>>> doc/guides/prog_guide/vhost_lib.rst    | 14 ++++++++++++++
>>>> doc/guides/rel_notes/release_17_11.rst |  3 ++-
>>>> drivers/net/vhost/rte_eth_vhost.c      | 13 +++++++++++++
>>>> lib/librte_vhost/rte_vhost.h           |  1 +
>>>> lib/librte_vhost/socket.c              |  6 ++++++
>>>> lib/librte_vhost/vhost_user.c          | 24 ++++++++++++++++++++++--
>>>> 7 files changed, 63 insertions(+), 3 deletions(-)
>>>>
>>>> -- 
>>>> 2.13.6
>>>
>>