Re: [PATCH v2 06/14] vdpa: adapt vhost_ops callbacks to svq

[Eugenio Perez Martin]

On Mon, Feb 28, 2022 at 4:59 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/2/27 下午9:41, Eugenio Pérez 写道:
> > First half of the buffers forwarding part, preparing vhost-vdpa
> > callbacks to SVQ to offer it. QEMU cannot enable it at this moment, so
> > this is effectively dead code at the moment, but it helps to reduce
> > patch size.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-vdpa.c | 84 ++++++++++++++++++++++++++++++++++++------
> >   1 file changed, 73 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index d614c435f3..b2c4e92fcf 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -344,6 +344,16 @@ static bool vhost_vdpa_one_time_request(struct 
> > vhost_dev *dev)
> >       return v->index != 0;
> >   }
> >
> > +static int vhost_vdpa_get_dev_features(struct vhost_dev *dev,
> > +                                       uint64_t *features)
> > +{
> > +    int ret;
> > +
> > +    ret = vhost_vdpa_call(dev, VHOST_GET_FEATURES, features);
> > +    trace_vhost_vdpa_get_features(dev, *features);
> > +    return ret;
> > +}
> > +
> >   static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa 
> > *v,
> >                                  Error **errp)
> >   {
> > @@ -356,7 +366,7 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, 
> > struct vhost_vdpa *v,
> >           return 0;
> >       }
> >
> > -    r = hdev->vhost_ops->vhost_get_features(hdev, &dev_features);
> > +    r = vhost_vdpa_get_dev_features(hdev, &dev_features);
> >       if (r != 0) {
> >           error_setg_errno(errp, -r, "Can't get vdpa device features");
> >           return r;
> > @@ -583,12 +593,26 @@ static int vhost_vdpa_set_mem_table(struct vhost_dev 
> > *dev,
> >   static int vhost_vdpa_set_features(struct vhost_dev *dev,
> >                                      uint64_t features)
> >   {
> > +    struct vhost_vdpa *v = dev->opaque;
> >       int ret;
> >
> >       if (vhost_vdpa_one_time_request(dev)) {
> >           return 0;
> >       }
> >
> > +    if (v->shadow_vqs_enabled) {
> > +        uint64_t features_ok = features;
> > +        bool ok;
> > +
> > +        ok = vhost_svq_valid_features(&features_ok);
> > +        if (unlikely(!ok)) {
> > +            error_report(
> > +                "Invalid guest acked feature flag, acked: 0x%"
> > +                PRIx64", ok: 0x%"PRIx64, features, features_ok);
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> >       trace_vhost_vdpa_set_features(dev, features);
> >       ret = vhost_vdpa_call(dev, VHOST_SET_FEATURES, &features);
> >       if (ret) {
> > @@ -735,6 +759,13 @@ static int vhost_vdpa_get_config(struct vhost_dev 
> > *dev, uint8_t *config,
> >       return ret;
> >    }
> >
> > +static int vhost_vdpa_set_dev_vring_base(struct vhost_dev *dev,
> > +                                         struct vhost_vring_state *ring)
> > +{
> > +    trace_vhost_vdpa_set_vring_base(dev, ring->index, ring->num);
> > +    return vhost_vdpa_call(dev, VHOST_SET_VRING_BASE, ring);
> > +}
> > +
> >   static int vhost_vdpa_set_vring_dev_kick(struct vhost_dev *dev,
> >                                            struct vhost_vring_file *file)
> >   {
> > @@ -749,6 +780,18 @@ static int vhost_vdpa_set_vring_dev_call(struct 
> > vhost_dev *dev,
> >       return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file);
> >   }
> >
> > +static int vhost_vdpa_set_vring_dev_addr(struct vhost_dev *dev,
> > +                                         struct vhost_vring_addr *addr)
> > +{
> > +    trace_vhost_vdpa_set_vring_addr(dev, addr->index, addr->flags,
> > +                                addr->desc_user_addr, addr->used_user_addr,
> > +                                addr->avail_user_addr,
> > +                                addr->log_guest_addr);
> > +
> > +    return vhost_vdpa_call(dev, VHOST_SET_VRING_ADDR, addr);
> > +
> > +}
> > +
> >   /**
> >    * Set the shadow virtqueue descriptors to the device
> >    *
> > @@ -859,11 +902,17 @@ static int vhost_vdpa_set_log_base(struct vhost_dev 
> > *dev, uint64_t base,
> >   static int vhost_vdpa_set_vring_addr(struct vhost_dev *dev,
> >                                          struct vhost_vring_addr *addr)
> >   {
> > -    trace_vhost_vdpa_set_vring_addr(dev, addr->index, addr->flags,
> > -                                    addr->desc_user_addr, 
> > addr->used_user_addr,
> > -                                    addr->avail_user_addr,
> > -                                    addr->log_guest_addr);
> > -    return vhost_vdpa_call(dev, VHOST_SET_VRING_ADDR, addr);
> > +    struct vhost_vdpa *v = dev->opaque;
> > +
> > +    if (v->shadow_vqs_enabled) {
> > +        /*
> > +         * Device vring addr was set at device start. SVQ base is handled 
> > by
> > +         * VirtQueue code.
> > +         */
> > +        return 0;
> > +    }
> > +
> > +    return vhost_vdpa_set_vring_dev_addr(dev, addr);
> >   }
> >
> >   static int vhost_vdpa_set_vring_num(struct vhost_dev *dev,
> > @@ -876,8 +925,17 @@ static int vhost_vdpa_set_vring_num(struct vhost_dev 
> > *dev,
> >   static int vhost_vdpa_set_vring_base(struct vhost_dev *dev,
> >                                          struct vhost_vring_state *ring)
> >   {
> > -    trace_vhost_vdpa_set_vring_base(dev, ring->index, ring->num);
> > -    return vhost_vdpa_call(dev, VHOST_SET_VRING_BASE, ring);
> > +    struct vhost_vdpa *v = dev->opaque;
> > +
> > +    if (v->shadow_vqs_enabled) {
> > +        /*
> > +         * Device vring base was set at device start. SVQ base is handled 
> > by
> > +         * VirtQueue code.
> > +         */
> > +        return 0;
> > +    }
> > +
> > +    return vhost_vdpa_set_dev_vring_base(dev, ring);
> >   }
> >
> >   static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
> > @@ -924,10 +982,14 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev 
> > *dev,
> >   static int vhost_vdpa_get_features(struct vhost_dev *dev,
> >                                        uint64_t *features)
> >   {
> > -    int ret;
> > +    struct vhost_vdpa *v = dev->opaque;
> > +    int ret = vhost_vdpa_get_dev_features(dev, features);
> > +
> > +    if (ret == 0 && v->shadow_vqs_enabled) {
> > +        /* Filter only features that SVQ can offer to guest */
> > +        vhost_svq_valid_features(features);
>
>
> I think it's better not silently clear features here (e.g the feature
> could be explicitly enabled via cli). This may give more troubles in the
> future cross vendor/backend live migration.
>
> We can simple during vhost_vdpa init.
>

Do you mean to fail the initialization as long as the emulated device
feature set contains any other feature set than mandatory
VIRTIO_F_ACCESS_PLATFORM and VIRTIO_F_VERSION_1?

I think that it makes sense at this moment, I'll move to that for the
next version.

Thanks!

> Thanks
>
>
> > +    }
> >
> > -    ret = vhost_vdpa_call(dev, VHOST_GET_FEATURES, features);
> > -    trace_vhost_vdpa_get_features(dev, *features);
> >       return ret;
> >   }
> >
>