Re: [RFC v2 08/13] virtio: Add vhost_shadow_vq_get_vring_addr

[Eugenio Perez Martin <eperezma@redhat.com>]

On Tue, Mar 16, 2021 at 4:20 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Tue, Mar 16, 2021 at 8:50 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2021/3/16 上午3:48, Eugenio Pérez 写道:
> > > It reports the shadow virtqueue address from qemu virtual address space
> >
> >
> > Note that to be used by vDPA, we can't use qemu VA directly here.
> >
>
> Right, I'm planning to use a different virtual address space if the
> device has such limitations.
>
> >
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   hw/virtio/vhost-shadow-virtqueue.h |  2 ++
> > >   hw/virtio/vhost-shadow-virtqueue.c | 24 +++++++++++++++++++++++-
> > >   2 files changed, 25 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> > > index 2ca4b92b12..d82c35bccf 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > > @@ -19,6 +19,8 @@ typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> > >
> > >   void vhost_shadow_vq_mask(VhostShadowVirtqueue *svq, EventNotifier *masked);
> > >   void vhost_shadow_vq_unmask(VhostShadowVirtqueue *svq);
> > > +void vhost_shadow_vq_get_vring_addr(const VhostShadowVirtqueue *svq,
> > > +                                    struct vhost_vring_addr *addr);
> > >
> > >   bool vhost_shadow_vq_start(struct vhost_dev *dev,
> > >                              unsigned idx,
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > index b6bab438d6..1460d1d5d1 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -17,6 +17,9 @@
> > >
> > >   /* Shadow virtqueue to relay notifications */
> > >   typedef struct VhostShadowVirtqueue {
> > > +    /* Shadow vring */
> > > +    struct vring vring;
> > > +
> > >       /* Shadow kick notifier, sent to vhost */
> > >       EventNotifier kick_notifier;
> > >       /* Shadow call notifier, sent to vhost */
> > > @@ -51,6 +54,9 @@ typedef struct VhostShadowVirtqueue {
> > >
> > >       /* Virtio device */
> > >       VirtIODevice *vdev;
> > > +
> > > +    /* Descriptors copied from guest */
> > > +    vring_desc_t descs[];
> > >   } VhostShadowVirtqueue;
> > >
> > >   /* Forward guest notifications */
> > > @@ -132,6 +138,19 @@ void vhost_shadow_vq_unmask(VhostShadowVirtqueue *svq)
> > >       qemu_event_wait(&svq->masked_notifier.is_free);
> > >   }
> > >
> > > +/*
> > > + * Get the shadow vq vring address.
> > > + * @svq Shadow virtqueue
> > > + * @addr Destination to store address
> > > + */
> > > +void vhost_shadow_vq_get_vring_addr(const VhostShadowVirtqueue *svq,
> > > +                                    struct vhost_vring_addr *addr)
> > > +{
> > > +    addr->desc_user_addr = (uint64_t)svq->vring.desc;
> > > +    addr->avail_user_addr = (uint64_t)svq->vring.avail;
> > > +    addr->used_user_addr = (uint64_t)svq->vring.used;
> > > +}
> > > +
> > >   /*
> > >    * Restore the vhost guest to host notifier, i.e., disables svq effect.
> > >    */
> > > @@ -262,7 +281,9 @@ void vhost_shadow_vq_stop(struct vhost_dev *dev,
> > >   VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx)
> > >   {
> > >       int vq_idx = dev->vq_index + idx;
> > > -    g_autofree VhostShadowVirtqueue *svq = g_new0(VhostShadowVirtqueue, 1);
> > > +    unsigned num = virtio_queue_get_num(dev->vdev, vq_idx);
> > > +    size_t ring_size = vring_size(num, VRING_DESC_ALIGN_SIZE);
> > > +    g_autofree VhostShadowVirtqueue *svq = g_malloc0(sizeof(*svq) + ring_size);
> > >       int r;
> > >
> > >       r = event_notifier_init(&svq->kick_notifier, 0);
> > > @@ -279,6 +300,7 @@ VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx)
> > >           goto err_init_call_notifier;
> > >       }
> > >
> > > +    vring_init(&svq->vring, num, svq->descs, VRING_DESC_ALIGN_SIZE);
> >
> >
> > We had some dicussion in the past. Exporting vring_init() is wrong but
> > too late to fix (assumes a legacy split layout). Let's not depend on
> > this buggy uAPI.
> >
>
> Ok, I will change the way to allocate and initialize it.
>

Could we define VIRTIO_RING_NO_LEGACY macro in qemu/osdep.h or similar
to avoid repeating this mistake in the future?

Thanks!

> > Thanks
> >
> >
> > >       svq->vq = virtio_get_queue(dev->vdev, vq_idx);
> > >       svq->vdev = dev->vdev;
> > >       event_notifier_set_handler(&svq->call_notifier,
> >