On Fri, Jun 22, 2018 at 03:43:26PM +0200, Maxime Coquelin wrote: > Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com> > --- > lib/librte_vhost/vhost.c | 55 +++++++++++++++++++++++++++---- > lib/librte_vhost/vhost.h | 71 > ++++++++++++++++++++++++++++++++++++---- > lib/librte_vhost/vhost_user.c | 29 +++++++++++++--- > lib/librte_vhost/virtio-packed.h | 11 +++++++ > lib/librte_vhost/virtio_net.c | 12 +++---- > 5 files changed, 154 insertions(+), 24 deletions(-) > > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c > index 7cbf1eded..c26162542 100644 > --- a/lib/librte_vhost/vhost.c > +++ b/lib/librte_vhost/vhost.c > @@ -595,7 +595,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx) > if (!vq) > return -1; > > - vhost_vring_call(dev, vq); > + if (vq_is_packed(dev)) > + vhost_vring_call_packed(dev, vq); > + else > + vhost_vring_call_split(dev, vq); > + > return 0; > } > > @@ -616,20 +620,59 @@ rte_vhost_avail_entries(int vid, uint16_t queue_id) > return *(volatile uint16_t *)&vq->avail->idx - vq->last_used_idx; > } > > +static inline int > +vhost_enable_notify_split(struct vhost_virtqueue *vq, int enable) > +{ > + if (enable) > + vq->used->flags &= ~VRING_USED_F_NO_NOTIFY; > + else > + vq->used->flags |= VRING_USED_F_NO_NOTIFY; > + > + return 0; > +} > + > +static inline int > +vhost_enable_notify_packed(struct virtio_net *dev, > + struct vhost_virtqueue *vq, int enable) > +{ > + uint16_t flags; > + > + if (!enable) { > + vq->device_event->desc_event_flags = RING_EVENT_FLAGS_DISABLE; > + return 0; > + } > + > + flags = RING_EVENT_FLAGS_DISABLE;
Should be RING_EVENT_FLAGS_ENABLE. > + if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) { > + flags = RING_EVENT_FLAGS_DESC; > + vq->device_event->desc_event_off_wrap = vq->last_avail_idx | > + vq->avail_wrap_counter << 15; > + } > + > + rte_smp_wmb(); > + > + vq->device_event->desc_event_flags = flags; > + > + rte_smp_wmb(); > + > + return 0; > +} > + > int > rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable) > { > struct virtio_net *dev = get_device(vid); > + struct vhost_virtqueue *vq; > > if (!dev) > return -1; > > - if (enable) > - dev->virtqueue[queue_id]->used->flags &= > - ~VRING_USED_F_NO_NOTIFY; > + vq = dev->virtqueue[queue_id]; > + > + if (vq_is_packed(dev)) > + return vhost_enable_notify_packed(dev, vq, enable); > else > - dev->virtqueue[queue_id]->used->flags |= VRING_USED_F_NO_NOTIFY; > - return 0; > + return vhost_enable_notify_split(vq, enable); > } > > void > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h > index 5dcb61c71..99db688a8 100644 > --- a/lib/librte_vhost/vhost.h > +++ b/lib/librte_vhost/vhost.h > @@ -21,6 +21,7 @@ > > #include "rte_vhost.h" > #include "rte_vdpa.h" > +#include "virtio-packed.h" The definitions in virtio-packed.h probably will conflit with the definitions in linux/virtio_ring.h which is included by linux/vhost.h (which is also included by this file) when packed ring is upstreamed to linux. It probably will cause build issues in the future. > > /* Used to indicate that the device is running on a data core */ > #define VIRTIO_DEV_RUNNING 1 > @@ -95,8 +96,14 @@ struct vhost_virtqueue { > struct vring_desc *desc; > struct vring_desc_packed *desc_packed; > }; > - struct vring_avail *avail; > - struct vring_used *used; > + union { > + struct vring_avail *avail; > + struct vring_packed_desc_event *driver_event; > + }; > + union { > + struct vring_used *used; > + struct vring_packed_desc_event *device_event; > + }; > uint32_t size; > > uint16_t last_avail_idx; > @@ -614,7 +621,7 @@ vhost_need_event(uint16_t event_idx, uint16_t new_idx, > uint16_t old) > } > > static __rte_always_inline void > -vhost_vring_call(struct virtio_net *dev, struct vhost_virtqueue *vq) > +vhost_vring_call_split(struct virtio_net *dev, struct vhost_virtqueue *vq) > { > /* Flush used->idx update before we read avail->flags. */ > rte_mb(); > @@ -625,11 +632,11 @@ vhost_vring_call(struct virtio_net *dev, struct > vhost_virtqueue *vq) > uint16_t new = vq->last_used_idx; > > VHOST_LOG_DEBUG(VHOST_DATA, "%s: used_event_idx=%d, old=%d, > new=%d\n", > - __func__, > - vhost_used_event(vq), > - old, new); > + __func__, > + vhost_used_event(vq), > + old, new); > if (vhost_need_event(vhost_used_event(vq), new, old) > - && (vq->callfd >= 0)) { > + && (vq->callfd >= 0)) { > vq->signalled_used = vq->last_used_idx; > eventfd_write(vq->callfd, (eventfd_t) 1); > } > @@ -641,4 +648,54 @@ vhost_vring_call(struct virtio_net *dev, struct > vhost_virtqueue *vq) > } > } > > +static __rte_always_inline void > +vhost_vring_call_packed(struct virtio_net *dev, struct vhost_virtqueue *vq) > +{ > + uint16_t old, new, off, off_wrap, wrap; > + bool kick = false; > + > + There is no need to have two blank lines. > + /* Flush used desc update. */ > + rte_smp_mb(); > + > + if (!(dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))) { > + if (vq->driver_event->desc_event_flags != > + RING_EVENT_FLAGS_DISABLE) > + kick = true; > + goto kick; > + } > + > + old = vq->signalled_used; > + new = vq->last_used_idx; > + vq->signalled_used = new; > + > + if (vq->driver_event->desc_event_flags != RING_EVENT_FLAGS_DESC) { > + if (vq->driver_event->desc_event_flags != > + RING_EVENT_FLAGS_DISABLE) > + kick = true; > + goto kick; > + } > + > + rte_smp_rmb(); > + > + off_wrap = vq->driver_event->desc_event_off_wrap; > + off = off_wrap & ~(1 << 15); > + wrap = vq->used_wrap_counter; > + > + if (new < old) { > + new += vq->size; > + wrap ^= 1; > + } > + > + if (wrap != off_wrap >> 15) > + off += vq->size; > + > + if (vhost_need_event(off, new, old)) > + kick = true; > + > +kick: > + if (kick) > + eventfd_write(vq->callfd, (eventfd_t)1); > +} > + > #endif /* _VHOST_NET_CDEV_H_ */ > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index a08d99314..c74924564 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -509,9 +509,6 @@ translate_ring_addresses(struct virtio_net *dev, int > vq_index) > len = sizeof(struct vring_desc_packed) * vq->size; > vq->desc_packed = (struct vring_desc_packed *) ring_addr_to_vva > (dev, vq, addr->desc_user_addr, &len); > - vq->desc = NULL; > - vq->avail = NULL; > - vq->used = NULL; There is no need to add them from the beginning. > vq->log_guest_addr = 0; > if (vq->desc_packed == 0 || > len != sizeof(struct vring_desc_packed) * > @@ -526,6 +523,30 @@ translate_ring_addresses(struct virtio_net *dev, int > 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, > + vq, addr->avail_user_addr, &len); > + if (vq->driver_event == 0 || It's better to compare with NULL. > + len != sizeof(struct vring_packed_desc_event)) { > + RTE_LOG(DEBUG, VHOST_CONFIG, > + "(%d) failed to find driver area address.\n", > + dev->vid); > + return dev; > + } > + > + len = sizeof(struct vring_packed_desc_event); > + vq->device_event = (struct vring_packed_desc_event *) > + (uintptr_t)ring_addr_to_vva(dev, > + vq, addr->used_user_addr, &len); > + if (vq->device_event == 0 || It's better to compare with NULL. > + len != sizeof(struct vring_packed_desc_event)) { > + RTE_LOG(DEBUG, VHOST_CONFIG, > + "(%d) failed to find device area address.\n", > + dev->vid); > + return dev; > + } > + > return dev; > } > > @@ -542,8 +563,6 @@ translate_ring_addresses(struct virtio_net *dev, int > vq_index) > dev->vid); > return dev; > } > - vq->desc_packed = NULL; There is no need to add it from the beginning. Best regards, Tiwei Bie