Re: [RFC PATCH v8 14/21] vhost: Make possible to check for device exclusive vq group

[Jason Wang]

On Thu, Jun 9, 2022 at 3:22 AM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Wed, Jun 8, 2022 at 6:25 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/5/20 03:12, Eugenio Pérez 写道:
> > > CVQ needs to be in its own group, not shared with any data vq. Enable
> > > the checking of it here, before introducing address space id concepts.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   include/hw/virtio/vhost.h |  2 +
> > >   hw/net/vhost_net.c        |  4 +-
> > >   hw/virtio/vhost-vdpa.c    | 79 ++++++++++++++++++++++++++++++++++++++-
> > >   hw/virtio/trace-events    |  1 +
> > >   4 files changed, 84 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > index b291fe4e24..cebec1d817 100644
> > > --- a/include/hw/virtio/vhost.h
> > > +++ b/include/hw/virtio/vhost.h
> > > @@ -84,6 +84,8 @@ struct vhost_dev {
> > >       int vq_index_end;
> > >       /* if non-zero, minimum required value for max_queues */
> > >       int num_queues;
> > > +    /* Must be a vq group different than any other vhost dev */
> > > +    bool independent_vq_group;
> >
> >
> > We probably need a better abstraction here.
> >
> > E.g having a parent vhost_dev_group structure.
> >
>
> I think there is room for improvement too, but to make this work we
> don't need the device model to know all the other devices at this
> moment. I'm open to implementing it if we decide that solution is more
> maintainable or whatever other reason though.

I see, so let's keep it as is and do the enhancement in the future.

>
> >
> > >       uint64_t features;
> > >       uint64_t acked_features;
> > >       uint64_t backend_features;
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index ccac5b7a64..1c2386c01c 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -339,14 +339,16 @@ int vhost_net_start(VirtIODevice *dev, 
> > > NetClientState *ncs,
> > >       }
> > >
> > >       for (i = 0; i < nvhosts; i++) {
> > > +        bool cvq_idx = i >= data_queue_pairs;
> > >
> > > -        if (i < data_queue_pairs) {
> > > +        if (!cvq_idx) {
> > >               peer = qemu_get_peer(ncs, i);
> > >           } else { /* Control Virtqueue */
> > >               peer = qemu_get_peer(ncs, n->max_queue_pairs);
> > >           }
> > >
> > >           net = get_vhost_net(peer);
> > > +        net->dev.independent_vq_group = !!cvq_idx;
> > >           vhost_net_set_vq_index(net, i * 2, index_end);
> > >
> > >           /* Suppress the masking guest notifiers on vhost user
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index eec6d544e9..52dd8baa8d 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -685,7 +685,8 @@ static int vhost_vdpa_set_backend_cap(struct 
> > > vhost_dev *dev)
> > >   {
> > >       uint64_t features;
> > >       uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
> > > -        0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH;
> > > +        0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
> > > +        0x1ULL << VHOST_BACKEND_F_IOTLB_ASID;
> > >       int r;
> > >
> > >       if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
> > > @@ -1110,6 +1111,78 @@ static bool vhost_vdpa_svqs_stop(struct vhost_dev 
> > > *dev)
> > >       return true;
> > >   }
> > >
> > > +static int vhost_vdpa_get_vring_group(struct vhost_dev *dev,
> > > +                                      struct vhost_vring_state *state)
> > > +{
> > > +    int ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_VRING_GROUP, state);
> > > +    trace_vhost_vdpa_get_vring_group(dev, state->index, state->num);
> > > +    return ret;
> > > +}
> > > +
> > > +static bool vhost_dev_is_independent_group(struct vhost_dev *dev)
> > > +{
> > > +    struct vhost_vdpa *v = dev->opaque;
> > > +    struct vhost_vring_state this_vq_group = {
> > > +        .index = dev->vq_index,
> > > +    };
> > > +    int ret;
> > > +
> > > +    if (!(dev->backend_cap & VHOST_BACKEND_F_IOTLB_ASID)) {
> > > +        return true;
> > > +    }
> >
> >
> > This should be false?
> >
> >
> > > +
> > > +    if (!v->shadow_vqs_enabled) {
> > > +        return true;
> > > +    }
> >
> >
> > And here?
> >
>
> They're true so it doesn't get in the middle if the device already
> knows there is no need to check vhost_dev for an independent group.

I'm not sure I understand this.

Without ASID but with MQ, we know all vhost_devs are not independent.

>
> With recent mq changes, I think I can delete these checks and move
> them to net/vhost-vdpa.
>
> >
> > > +
> > > +    ret = vhost_vdpa_get_vring_group(dev, &this_vq_group);
> > > +    if (unlikely(ret)) {
> > > +        goto call_err;
> > > +    }
> > > +
> > > +    for (int i = 1; i < dev->nvqs; ++i) {
> > > +        struct vhost_vring_state vq_group = {
> > > +            .index = dev->vq_index + i,
> > > +        };
> > > +
> > > +        ret = vhost_vdpa_get_vring_group(dev, &vq_group);
> > > +        if (unlikely(ret)) {
> > > +            goto call_err;
> > > +        }
> > > +        if (unlikely(vq_group.num != this_vq_group.num)) {
> > > +            error_report("VQ %d group is different than VQ %d one",
> > > +                         this_vq_group.index, vq_group.index);
> >
> >
> > Not sure this is needed. The group id is not tied to vq index if I
> > understand correctly.
> >
> > E.g we have 1 qp with cvq, we can have
> >
> > group 0 cvq
> >
> > group 1 tx/rx
> >
>
> This function is severly undocumented, thanks for pointing out :).
>
> It checks if the virtqueues that belong to this vhost_dev does not
> share vq group with any other virtqueue in the device. We need to
> check it at device startup, since cvq index changes depending on _F_MQ
> negotiated.
>
> Since we're going to check all other virtqueues, we don't need to know
> other vhost_dev individually: We know the set of vqs to check is all
> vqs but our vhost_dev one.
>
> Does it make it more clear?

Kind of, but

We make independent_vq_group to be true for cvq unconditionally, and
check other vhost_dev during start. This seems less optimal. Any
reason we do all of the logic during net_inti_vhost_vdpa()?

E.g we can get and set up the group asid stuff once there instead of
on each stop/start.

Thanks

>
> Thanks!
>
> > Thanks
> >
> >
> > > +            return false;
> > > +        }
> > > +    }
> > > +
> > > +    for (int i = 0; i < dev->vq_index_end; ++i) {
> > > +        struct vhost_vring_state vq_group = {
> > > +            .index = i,
> > > +        };
> > > +
> > > +        if (dev->vq_index <= i && i < dev->vq_index + dev->nvqs) {
> > > +            continue;
> > > +        }
> > > +
> > > +        ret = vhost_vdpa_get_vring_group(dev, &vq_group);
> > > +        if (unlikely(ret)) {
> > > +            goto call_err;
> > > +        }
> > > +        if (unlikely(vq_group.num == this_vq_group.num)) {
> > > +            error_report("VQ %d group is the same as VQ %d one",
> > > +                         this_vq_group.index, vq_group.index);
> > > +            return false;
> > > +        }
> > > +    }
> > > +
> > > +    return true;
> > > +
> > > +call_err:
> > > +    error_report("Can't read vq group, errno=%d (%s)", ret, 
> > > g_strerror(-ret));
> > > +    return false;
> > > +}
> > > +
> > >   static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> > >   {
> > >       struct vhost_vdpa *v = dev->opaque;
> > > @@ -1118,6 +1191,10 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
> > > *dev, bool started)
> > >
> > >       if (started) {
> > >           vhost_vdpa_host_notifiers_init(dev);
> > > +        if (dev->independent_vq_group &&
> > > +            !vhost_dev_is_independent_group(dev)) {
> > > +            return -1;
> > > +        }
> > >           ok = vhost_vdpa_svqs_start(dev);
> > >           if (unlikely(!ok)) {
> > >               return -1;
> > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > > index ab8e095b73..ffb8eb26e7 100644
> > > --- a/hw/virtio/trace-events
> > > +++ b/hw/virtio/trace-events
> > > @@ -46,6 +46,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
> > >   vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
> > >   vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, 
> > > uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 
> > > 0x%"PRIx32
> > >   vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) 
> > > "dev: %p config: %p config_len: %"PRIu32
> > > +vhost_vdpa_get_vring_group(void *dev, unsigned int index, unsigned int 
> > > num) "dev: %p index: %u num: %u"
> > >   vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d"
> > >   vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long long 
> > > size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64" size: 
> > > %llu refcnt: %d fd: %d log: %p"
> > >   vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned int 
> > > flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t 
> > > avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags: 0x%x 
> > > desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64" avail_user_addr: 
> > > 0x%"PRIx64" log_guest_addr: 0x%"PRIx64
> >
>