[dpdk-dev] [PATCH v4 0/4] Vhost: fix mq=on but VIRTIO_NET_F_MQ not negotiated

[dpdk-dev] [PATCH v4 0/4] Vhost: fix mq=on but VIRTIO_NET_F_MQ not negotiated

[Maxime Coquelin]

Hi,

This fourth revision fixes patch 1 by not returning early in
SET_FEATURE handling if new features bitfield is same as previous
one. Indeed, as reported by Tiwei, in case negotiated features is 0,
it would return early whereas it should set the Vnet header len.
The change consists in returning early when features are equal only
when the device is un running state.
I did not applied Laszlo's Acked-by and Ladi's Tested-by because of
this change. This new iteration has been tested locally using iPXE.

Having QEMU started with mq=on but guest driver not negotiating
VIRTIO_NET_F_MQ feature ends up in the vhost device to never
start. Indeed, more queues are created in the vhost backend than
configured.

Guest drivers known to not advertise the VIRTIO_NET_F_MQ feature are
iPXE and OVMF Virtio-net drivers.

Queues are created because before starting the guest, QEMU sends
VHOST_USER_SET_VRING_CALL requests for all queues declared in QEMU
command line. Also, once Virtio features negotiated, QEMU sends
VHOST_USER_SET_VRING_ENABLE requests to disable all but the first
queue pair.

This series fixes this by destroying all but first queue pair in
the backend if VIRTIO_NET_F_MQ isn't negotiated. First patches
makes sure that VHOST_USER_SET_FEATURES request doesn't change
Virtio features while the device is running, which should never
happen as per the Virtio spec. This helps to make sure vitqueues
aren't destroyed while being processed, but also protect from
other illegal features changes (e.g. VIRTIO_NET_F_MRG_RXBUF).

Changes since v3:
=================
- Fix Virtio features = 0 case (Tiwei)
Changes since v2:
=================
- Patch 2: Rework & fix VQs destruction loop (Laszlo)
Changes since v1:
=================
- Patch 1: shift bits in the right direction (Ladi)

Maxime Coquelin (4):
  vhost: prevent features to be changed while device is running
  vhost: propagate VHOST_USER_SET_FEATURES handling error
  vhost: extract virtqueue cleaning and freeing functions
  vhost: destroy unused virtqueues when multiqueue not negotiated

 lib/librte_vhost/vhost.c      | 22 ++++++++++++----------
 lib/librte_vhost/vhost.h      |  3 +++
 lib/librte_vhost/vhost_user.c | 39 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 52 insertions(+), 12 deletions(-)

-- 
2.14.3

[dpdk-dev] [PATCH v4 1/4] vhost: prevent features to be changed while device is running

[Maxime Coquelin]

As section 2.2 of the Virtio spec states about features
negotiation:
"During device initialization, the driver reads this and tells
the device the subset that it accepts. The only way to
renegotiate is to reset the device."

This patch implements a check to prevent illegal features change
while the device is running.

One exception is the VHOST_F_LOG_ALL feature bit, which is enabled
when live-migration is initiated. But this feature is not negotiated
with the Virtio driver, but directly with the Vhost master.

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

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index f4c7ce462..545dbcb2b 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -183,7 +183,22 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 		return -1;
 	}
 
-	if ((dev->flags & VIRTIO_DEV_RUNNING) && dev->features != features) {
+	if (dev->flags & VIRTIO_DEV_RUNNING) {
+		if (dev->features == features)
+			return 0;
+
+		/*
+		 * Error out if master tries to change features while device is
+		 * in running state. The exception being VHOST_F_LOG_ALL, which
+		 * is enabled when the live-migration starts.
+		 */
+		if ((dev->features ^ features) & ~(1ULL << VHOST_F_LOG_ALL)) {
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"(%d) features changed while device is running.\n",
+				dev->vid);
+			return -1;
+		}
+
 		if (dev->notify_ops->features_changed)
 			dev->notify_ops->features_changed(dev->vid, features);
 	}
-- 
2.14.3

[dpdk-dev] [PATCH v4 2/4] vhost: propagate VHOST_USER_SET_FEATURES handling error

[Maxime Coquelin]

Not propagating VHOST_USER_SET_FEATURES request handling
error may result in unpredictable behavior, as host and
guests features may no more be synchronized.

This patch fixes this by reporting the error to the upper
layer, which would result in the device being destroyed
and the connection with the master to be closed.

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

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 545dbcb2b..471b1612c 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1263,7 +1263,9 @@ vhost_user_msg_handler(int vid, int fd)
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_FEATURES:
-		vhost_user_set_features(dev, msg.payload.u64);
+		ret = vhost_user_set_features(dev, msg.payload.u64);
+		if (ret)
+			return -1;
 		break;
 
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
-- 
2.14.3

[dpdk-dev] [PATCH v4 3/4] vhost: extract virtqueue cleaning and freeing functions

[Maxime Coquelin]

This patch extracts needed code for vhost_user.c to be able
to clean and free virtqueues unitary.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 22 ++++++++++++----------
 lib/librte_vhost/vhost.h |  3 +++
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 4f8b73a09..df528a4ea 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -103,7 +103,7 @@ get_device(int vid)
 	return dev;
 }
 
-static void
+void
 cleanup_vq(struct vhost_virtqueue *vq, int destroy)
 {
 	if ((vq->callfd >= 0) && (destroy != 0))
@@ -127,6 +127,15 @@ cleanup_device(struct virtio_net *dev, int destroy)
 		cleanup_vq(dev->virtqueue[i], destroy);
 }
 
+void
+free_vq(struct vhost_virtqueue *vq)
+{
+	rte_free(vq->shadow_used_ring);
+	rte_free(vq->batch_copy_elems);
+	rte_mempool_free(vq->iotlb_pool);
+	rte_free(vq);
+}
+
 /*
  * Release virtqueues and device memory.
  */
@@ -134,16 +143,9 @@ static void
 free_device(struct virtio_net *dev)
 {
 	uint32_t i;
-	struct vhost_virtqueue *vq;
-
-	for (i = 0; i < dev->nr_vring; i++) {
-		vq = dev->virtqueue[i];
 
-		rte_free(vq->shadow_used_ring);
-		rte_free(vq->batch_copy_elems);
-		rte_mempool_free(vq->iotlb_pool);
-		rte_free(vq);
-	}
+	for (i = 0; i < dev->nr_vring; i++)
+		free_vq(dev->virtqueue[i]);
 
 	rte_free(dev);
 }
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 1cc81c17c..9cad1bb3c 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -364,6 +364,9 @@ void cleanup_device(struct virtio_net *dev, int destroy);
 void reset_device(struct virtio_net *dev);
 void vhost_destroy_device(int);
 
+void cleanup_vq(struct vhost_virtqueue *vq, int destroy);
+void free_vq(struct vhost_virtqueue *vq);
+
 int alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx);
 
 void vhost_set_ifname(int, const char *if_name, unsigned int if_len);
-- 
2.14.3

[dpdk-dev] [PATCH v4 4/4] vhost: destroy unused virtqueues when multiqueue not negotiated

[Maxime Coquelin]

QEMU sends VHOST_USER_SET_VRING_CALL requests for all queues
declared in QEMU command line before the guest is started.
It has the effect in DPDK vhost-user backend to allocate vrings
for all queues declared by QEMU.

If the first driver being used does not support multiqueue,
the device never changes to VIRTIO_DEV_RUNNING state as only
the first queue pair is initialized. One driver impacted by
this bug is virtio-net's iPXE driver which does not support
VIRTIO_NET_F_MQ feature.

It is safe to destroy unused virtqueues in SET_FEATURES request
handler, as it is ensured the device is not in running state
at this stage, so virtqueues aren't being processed.

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

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 471b1612c..d5ca1ac90 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -216,6 +216,24 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 		(dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
 		(dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
 
+	if (!(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
+		/*
+		 * Remove all but first queue pair if MQ hasn't been
+		 * negotiated. This is safe because the device is not
+		 * running at this stage.
+		 */
+		while (dev->nr_vring > 2) {
+			struct vhost_virtqueue *vq;
+
+			vq = dev->virtqueue[--dev->nr_vring];
+			if (!vq)
+				continue;
+
+			cleanup_vq(vq, 1);
+			free_vq(vq);
+		}
+	}
+
 	return 0;
 }
 
-- 
2.14.3

Re: [dpdk-dev] [PATCH v4 0/4] Vhost: fix mq=on but VIRTIO_NET_F_MQ not negotiated

[Laszlo Ersek]

On 12/11/17 16:14, Maxime Coquelin wrote:
> Hi,
> 
> This fourth revision fixes patch 1 by not returning early in
> SET_FEATURE handling if new features bitfield is same as previous
> one. Indeed, as reported by Tiwei, in case negotiated features is 0,
> it would return early whereas it should set the Vnet header len.
> The change consists in returning early when features are equal only
> when the device is un running state.
> I did not applied Laszlo's Acked-by and Ladi's Tested-by because of
> this change. This new iteration has been tested locally using iPXE.
> 
> Having QEMU started with mq=on but guest driver not negotiating
> VIRTIO_NET_F_MQ feature ends up in the vhost device to never
> start. Indeed, more queues are created in the vhost backend than
> configured.
> 
> Guest drivers known to not advertise the VIRTIO_NET_F_MQ feature are
> iPXE and OVMF Virtio-net drivers.
> 
> Queues are created because before starting the guest, QEMU sends
> VHOST_USER_SET_VRING_CALL requests for all queues declared in QEMU
> command line. Also, once Virtio features negotiated, QEMU sends
> VHOST_USER_SET_VRING_ENABLE requests to disable all but the first
> queue pair.
> 
> This series fixes this by destroying all but first queue pair in
> the backend if VIRTIO_NET_F_MQ isn't negotiated. First patches
> makes sure that VHOST_USER_SET_FEATURES request doesn't change
> Virtio features while the device is running, which should never
> happen as per the Virtio spec. This helps to make sure vitqueues
> aren't destroyed while being processed, but also protect from
> other illegal features changes (e.g. VIRTIO_NET_F_MRG_RXBUF).
> 
> Changes since v3:
> =================
> - Fix Virtio features = 0 case (Tiwei)
> Changes since v2:
> =================
> - Patch 2: Rework & fix VQs destruction loop (Laszlo)
> Changes since v1:
> =================
> - Patch 1: shift bits in the right direction (Ladi)
> 
> Maxime Coquelin (4):
>   vhost: prevent features to be changed while device is running
>   vhost: propagate VHOST_USER_SET_FEATURES handling error
>   vhost: extract virtqueue cleaning and freeing functions
>   vhost: destroy unused virtqueues when multiqueue not negotiated
> 
>  lib/librte_vhost/vhost.c      | 22 ++++++++++++----------
>  lib/librte_vhost/vhost.h      |  3 +++
>  lib/librte_vhost/vhost_user.c | 39 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 52 insertions(+), 12 deletions(-)
> 

I compared patch #1 between v3 and v4 -- I think you could have carried
forward my A-b. (Perhaps a more thorough R-b should have been dropped
indeed.) Anyways, for v4:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [dpdk-dev] [PATCH v4 0/4] Vhost: fix mq=on but VIRTIO_NET_F_MQ not negotiated

[Ladi Prosek]

On Mon, Dec 11, 2017 at 4:14 PM, Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> Hi,
>
> This fourth revision fixes patch 1 by not returning early in
> SET_FEATURE handling if new features bitfield is same as previous
> one. Indeed, as reported by Tiwei, in case negotiated features is 0,
> it would return early whereas it should set the Vnet header len.
> The change consists in returning early when features are equal only
> when the device is un running state.
> I did not applied Laszlo's Acked-by and Ladi's Tested-by because of
> this change. This new iteration has been tested locally using iPXE.
>
> Having QEMU started with mq=on but guest driver not negotiating
> VIRTIO_NET_F_MQ feature ends up in the vhost device to never
> start. Indeed, more queues are created in the vhost backend than
> configured.
>
> Guest drivers known to not advertise the VIRTIO_NET_F_MQ feature are
> iPXE and OVMF Virtio-net drivers.
>
> Queues are created because before starting the guest, QEMU sends
> VHOST_USER_SET_VRING_CALL requests for all queues declared in QEMU
> command line. Also, once Virtio features negotiated, QEMU sends
> VHOST_USER_SET_VRING_ENABLE requests to disable all but the first
> queue pair.
>
> This series fixes this by destroying all but first queue pair in
> the backend if VIRTIO_NET_F_MQ isn't negotiated. First patches
> makes sure that VHOST_USER_SET_FEATURES request doesn't change
> Virtio features while the device is running, which should never
> happen as per the Virtio spec. This helps to make sure vitqueues
> aren't destroyed while being processed, but also protect from
> other illegal features changes (e.g. VIRTIO_NET_F_MRG_RXBUF).
>
> Changes since v3:
> =================
> - Fix Virtio features = 0 case (Tiwei)
> Changes since v2:
> =================
> - Patch 2: Rework & fix VQs destruction loop (Laszlo)
> Changes since v1:
> =================
> - Patch 1: shift bits in the right direction (Ladi)
>
> Maxime Coquelin (4):
>   vhost: prevent features to be changed while device is running
>   vhost: propagate VHOST_USER_SET_FEATURES handling error
>   vhost: extract virtqueue cleaning and freeing functions
>   vhost: destroy unused virtqueues when multiqueue not negotiated
>
>  lib/librte_vhost/vhost.c      | 22 ++++++++++++----------
>  lib/librte_vhost/vhost.h      |  3 +++
>  lib/librte_vhost/vhost_user.c | 39 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 52 insertions(+), 12 deletions(-)

Tested v4 with iPXE and confirming that it still works fine.

Tested-by: Ladi Prosek <lprosek@redhat.com>

Thank you!
Ladi

Re: [dpdk-dev] [PATCH v4 4/4] vhost: destroy unused virtqueues when multiqueue not negotiated

[Tiwei Bie]

On Mon, Dec 11, 2017 at 04:15:03PM +0100, Maxime Coquelin wrote:
> QEMU sends VHOST_USER_SET_VRING_CALL requests for all queues
> declared in QEMU command line before the guest is started.
> It has the effect in DPDK vhost-user backend to allocate vrings
> for all queues declared by QEMU.
> 
> If the first driver being used does not support multiqueue,
> the device never changes to VIRTIO_DEV_RUNNING state as only
> the first queue pair is initialized. One driver impacted by
> this bug is virtio-net's iPXE driver which does not support
> VIRTIO_NET_F_MQ feature.
> 
> It is safe to destroy unused virtqueues in SET_FEATURES request
> handler, as it is ensured the device is not in running state
> at this stage, so virtqueues aren't being processed.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost_user.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 471b1612c..d5ca1ac90 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -216,6 +216,24 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>  		(dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
>  		(dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
>  
> +	if (!(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
> +		/*
> +		 * Remove all but first queue pair if MQ hasn't been
> +		 * negotiated. This is safe because the device is not
> +		 * running at this stage.
> +		 */
> +		while (dev->nr_vring > 2) {
> +			struct vhost_virtqueue *vq;
> +
> +			vq = dev->virtqueue[--dev->nr_vring];
> +			if (!vq)
> +				continue;
> +
> +			cleanup_vq(vq, 1);
> +			free_vq(vq);

Hi,

Sorry, I didn't look into this patch in last version.

The freed dev->virtqueue[$idx] also needs to be zeroed.
Otherwise, it won't be allocated in the future due to the
below check in vhost_user_check_and_alloc_queue_pair(),
and the freed memory will be used again.

/*
 * Allocate a queue pair if it hasn't been allocated yet
 */
static int
vhost_user_check_and_alloc_queue_pair(struct virtio_net *dev, VhostUserMsg *msg)
{
        ........

	if (dev->virtqueue[vring_idx])
		return 0;

	return alloc_vring_queue(dev, vring_idx);
}

Best regards,
Tiwei Bie

> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.14.3
> 

Re: [dpdk-dev] [PATCH v4 4/4] vhost: destroy unused virtqueues when multiqueue not negotiated

[Maxime Coquelin]

On 12/13/2017 04:16 AM, Tiwei Bie wrote:
> On Mon, Dec 11, 2017 at 04:15:03PM +0100, Maxime Coquelin wrote:
>> QEMU sends VHOST_USER_SET_VRING_CALL requests for all queues
>> declared in QEMU command line before the guest is started.
>> It has the effect in DPDK vhost-user backend to allocate vrings
>> for all queues declared by QEMU.
>>
>> If the first driver being used does not support multiqueue,
>> the device never changes to VIRTIO_DEV_RUNNING state as only
>> the first queue pair is initialized. One driver impacted by
>> this bug is virtio-net's iPXE driver which does not support
>> VIRTIO_NET_F_MQ feature.
>>
>> It is safe to destroy unused virtqueues in SET_FEATURES request
>> handler, as it is ensured the device is not in running state
>> at this stage, so virtqueues aren't being processed.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/librte_vhost/vhost_user.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 471b1612c..d5ca1ac90 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -216,6 +216,24 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>>   		(dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
>>   		(dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
>>   
>> +	if (!(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
>> +		/*
>> +		 * Remove all but first queue pair if MQ hasn't been
>> +		 * negotiated. This is safe because the device is not
>> +		 * running at this stage.
>> +		 */
>> +		while (dev->nr_vring > 2) {
>> +			struct vhost_virtqueue *vq;
>> +
>> +			vq = dev->virtqueue[--dev->nr_vring];
>> +			if (!vq)
>> +				continue;
>> +
>> +			cleanup_vq(vq, 1);
>> +			free_vq(vq);
> 
> Hi,
> 
> Sorry, I didn't look into this patch in last version.

Don't be sorry, thanks for catching this bug.

> The freed dev->virtqueue[$idx] also needs to be zeroed.
> Otherwise, it won't be allocated in the future due to the
> below check in vhost_user_check_and_alloc_queue_pair(),
> and the freed memory will be used again.
> 
> /*
>   * Allocate a queue pair if it hasn't been allocated yet
>   */
> static int
> vhost_user_check_and_alloc_queue_pair(struct virtio_net *dev, VhostUserMsg *msg)
> {
>          ........
> 
> 	if (dev->virtqueue[vring_idx])
> 		return 0;
> 
> 	return alloc_vring_queue(dev, vring_idx);
> }

You are right, I'll post v5 setting dev->virtqueue[$idx] to NULL after
free_vq() call.

Thanks for the review,
Maxime

> Best regards,
> Tiwei Bie
> 
>> +		}
>> +	}
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.14.3
>>