Re: [dpdk-dev] [PATCH v3] vhost: fix vhost user virtqueue not accessible

["Liu, Yong" <yong.liu@intel.com>]

> -----Original Message-----
> From: Adrian Moreno [mailto:amorenoz@redhat.com]
> Sent: Friday, November 01, 2019 1:48 AM
> To: Liu, Yong <yong.liu@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v3] vhost: fix vhost user virtqueue not accessible
> 
> Hi Marvin,
> 
> On 10/31/19 3:54 PM, Liu, Yong wrote:
> >
> >
> >> -----Original Message-----
> >> From: Bie, Tiwei
> >> Sent: Thursday, October 31, 2019 6:42 PM
> >> To: Liu, Yong <yong.liu@intel.com>
> >> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> >> amorenoz@redhat.com; dev@dpdk.org
> >> Subject: Re: [PATCH v3] vhost: fix vhost user virtqueue not accessible
> >>
> >> On Wed, Oct 30, 2019 at 10:56:02PM +0800, Marvin Liu wrote:
> >>> Log feature is disabled in vhost user, so that log address was invalid
> >>> when checking. Check whether log address is valid can workaround it.
> >>> Also log address should be translated in packed ring virtqueue.
> >>>
> >>> Fixes: 04cfc7fdbfca ("vhost: translate incoming log address to gpa")
> >>>
> >>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >>> ---
> >>>  lib/librte_vhost/vhost_user.c | 30 +++++++++++++-----------------
> >>>  1 file changed, 13 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/lib/librte_vhost/vhost_user.c
> >> b/lib/librte_vhost/vhost_user.c
> >>> index 61ef699ac..7754d2467 100644
> >>> --- a/lib/librte_vhost/vhost_user.c
> >>> +++ b/lib/librte_vhost/vhost_user.c
> >>> @@ -641,11 +641,23 @@ translate_ring_addresses(struct virtio_net *dev,
> >> int vq_index)
> >>>  	struct vhost_vring_addr *addr = &vq->ring_addrs;
> >>>  	uint64_t len, expected_len;
> >>>
> >>> +	dev = numa_realloc(dev, vq_index);
> >>
> >> We need to update `vq->desc` first before doing numa_realloc.
> >>
> https://github.com/DPDK/dpdk/blob/19397c7bf2545e6adab41b657a1f1da3c7344e7b/
> >> lib/librte_vhost/vhost_user.c#L445
> >>
> >>> +	vq = dev->virtqueue[vq_index];
> >>> +	if (addr->flags & (1 << VHOST_VRING_F_LOG)) {
> >>
> I fear the possible consequences of this change.
> Before 04cfc7fdbfca the approach was "best-effort". The log address would
> be
> assigned without further checks:
> 
> 	vq->log_guest_addr = addr->log_guest_addr;
> 
> Then, the behavior changed and an error was generated if the log address
> was
> invalid, which I guess is the problem you have hit:
> 
> 	vq->log_guest_addr =
> 		translate_log_addr(dev, vq, addr->log_guest_addr);
> 	if (vq->log_guest_addr == 0) {
> 		RTE_LOG(DEBUG, VHOST_CONFIG,
> 			"(%d) failed to map log_guest_addr .\n",
> 			dev->vid);
> 		return dev;
> 	}
> 
> In the tests I ran I always saw valid log addresses being sent at ring
> initialization phase, but if, as you claim, it's possible that invalid
> addresses
> are given at initialization phase, maybe we should go back to "best-effort"
> (i.e: remove the return statement)
> 
> But it's unlikely that qemu has enabled logging at ring initialization so
> this
> would effectively disable the translation at the initialization phase. I
> cannot
> forecast the consequences of this change without deeper analysis.

That's fine, Adrian. This issue only occurred when using dpdk virtio user device.
Since address logging is disabled in virtio user, simple flag check fix it.

> 
> >> `vq` can be reallocated by numa_realloc.
> >> We need to update the `addr` pointer before using it.
> >>
> >
> > Hi Tiwei,
> > Numa_realloc function will copy data from original vq structure to new vq
> when reallocating.
> > The content of vhost_ring_addr will be the same in new and old vqs, it
> may not be necessary to update pointer.
> That's true but 'addr' still holds a pointer to the old structure, assigned
> at
> line 641.
> 
> Also, note Tiwei's comment regarding updating 'vq->desc'. The idea behind
> numa_realloc is to reallocate the vhost_virtqueue structure to the same
> numa
> node as the descriptor ring. This function is updating the descriptor rings,
> so
> I think the idea is to update the ring addresses and then reallocate the
> virtqueue structure if needed.
> 
You are right, I misunderstood tiwei's comment. The ring address is useful for 
checking numa id, numa reallocation should be after address translation.

Regards,
Marvin

> Thanks,
> Adrian
> 
> > Regards,
> > Marvin
> >
> >> Thanks,
> >> Tiwei
> >>
> >>
> >>> +		vq->log_guest_addr =
> >>> +			translate_log_addr(dev, vq, addr->log_guest_addr);
> >>> +		if (vq->log_guest_addr == 0) {
> >>> +			RTE_LOG(DEBUG, VHOST_CONFIG,
> >>> +					"(%d) failed to map log_guest_addr.\n",
> >>> +					dev->vid);
> >>> +			return dev;
> >>> +		}
> >>> +	}
> >>> +
> >>>  	if (vq_is_packed(dev)) {
> >>>  		len = sizeof(struct vring_packed_desc) * vq->size;
> >>>  		vq->desc_packed = (struct vring_packed_desc *)(uintptr_t)
> >>>  			ring_addr_to_vva(dev, vq, addr->desc_user_addr, &len);
> >>> -		vq->log_guest_addr = 0;
> >>>  		if (vq->desc_packed == NULL ||
> >>>  				len != sizeof(struct vring_packed_desc) *
> >>>  				vq->size) {
> >>> @@ -655,10 +667,6 @@ translate_ring_addresses(struct virtio_net *dev,
> int
> >> vq_index)
> >>>  			return dev;
> >>>  		}
> >>>
> >>> -		dev = numa_realloc(dev, vq_index);
> >>> -		vq = dev->virtqueue[vq_index];
> >>> -		addr = &vq->ring_addrs;
> >>> -
> >>>  		len = sizeof(struct vring_packed_desc_event);
> >>>  		vq->driver_event = (struct vring_packed_desc_event *)
> >>>  					(uintptr_t)ring_addr_to_vva(dev,
> >>> @@ -701,10 +709,6 @@ translate_ring_addresses(struct virtio_net *dev,
> int
> >> vq_index)
> >>>  		return dev;
> >>>  	}
> >>>
> >>> -	dev = numa_realloc(dev, vq_index);
> >>> -	vq = dev->virtqueue[vq_index];
> >>> -	addr = &vq->ring_addrs;
> >>> -
> >>>  	len = sizeof(struct vring_avail) + sizeof(uint16_t) * vq->size;
> >>>  	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))
> >>>  		len += sizeof(uint16_t);
> >>> @@ -741,14 +745,6 @@ translate_ring_addresses(struct virtio_net *dev,
> int
> >> vq_index)
> >>>  		vq->last_avail_idx = vq->used->idx;
> >>>  	}
> >>>
> >>> -	vq->log_guest_addr =
> >>> -		translate_log_addr(dev, vq, addr->log_guest_addr);
> >>> -	if (vq->log_guest_addr == 0) {
> >>> -		RTE_LOG(DEBUG, VHOST_CONFIG,
> >>> -			"(%d) failed to map log_guest_addr .\n",
> >>> -			dev->vid);
> >>> -		return dev;
> >>> -	}
> >>>  	vq->access_ok = 1;
> >>>
> >>>  	VHOST_LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n",
> >>> --
> >>> 2.17.1
> >>>