> -----Original Message----- > From: Adrian Moreno [mailto:amore...@redhat.com] > Sent: Friday, October 25, 2019 8:21 PM > To: Liu, Yong <yong....@intel.com>; maxime.coque...@redhat.com; Bie, Tiwei > <tiwei....@intel.com>; Wang, Zhihong <zhihong.w...@intel.com> > Cc: dev@dpdk.org > Subject: Re: [PATCH] vhost: fix vhost user virtqueue not accessable > > Hi Marvin, > > On 10/25/19 6:20 PM, Marvin Liu wrote: > > Log feature is disabled in vhost user, so that log address was invalid > > when checking. Add feature bit check can skip useless address check. > > > Just so I understand, what conditions is the log address invalid? > > > Fixes: 04cfc7fdbfca ("vhost: translate incoming log address to gpa") > > > > Signed-off-by: Marvin Liu <yong....@intel.com> > > --- > > lib/librte_vhost/vhost_user.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/lib/librte_vhost/vhost_user.c > b/lib/librte_vhost/vhost_user.c > > index 61ef699ac..0407fdc29 100644 > > --- a/lib/librte_vhost/vhost_user.c > > +++ b/lib/librte_vhost/vhost_user.c > > @@ -741,13 +741,15 @@ 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; > > + if (dev->features & (1ULL << VHOST_F_LOG_ALL)) { > > + vq->log_guest_addr = > > + translate_log_addr(dev, vq, addr->log_guest_addr); > > VHOST_F_LOG_ALL is only negotiated once the migration has started (at least > from > qemu's perspective). > That means that we will postponing the translation of the log address to > the > vhost_user_set_vring_addr() call that follows the VHOST_F_LOG_ALL enabling. > In > that call there are (at least) two things that could go wrong and lead to a > migration failure: > - If VHOST_USER_F_PROTOCOL_FEATURES is not enabled, the address won't be > translated: > > vhost_user:795 > if ((vq->enabled && (dev->features & > (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) || > access_ok) { > dev = translate_ring_addresses(dev, msg->payload.addr.index); > if (!dev) > return RTE_VHOST_MSG_RESULT_ERR; > > *pdev = dev; > } > > - If the IOMMU is enabled and there's a miss, we would have to wait for the > IOTLB_UPDATE and during that time, there would be failed accesses to the > (still > untranslated) log address. > >
Thanks, Adrian. Log address can be zero when logging is not enabled. How about add other criteria after translation? Log address will be translated anyway and not affect vq status. vq->log_guest_addr = translate_log_addr(dev, vq, addr->log_guest_addr); - if (vq->log_guest_addr == 0) { + if (vq->log_guest_addr == 0 && addr->flags) { RTE_LOG(DEBUG, VHOST_CONFIG, "(%d) failed to map log_guest_addr .\n", dev->vid); return dev; } Meanwhile, log address of packed ring is fixed to zero. Is any special reason to do that? Regards, Marvin > > > + 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; > > > > > Thanks, > Adrian