Re: [PATCH v2 04/14] vhost: Add vhost_svq_valid_features to shadow vq

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

On Mon, Feb 28, 2022 at 4:25 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/2/27 下午9:41, Eugenio Pérez 写道:
> > This allows SVQ to negotiate features with the guest and the device. For
> > the device, SVQ is a driver. While this function bypasses all
> > non-transport features, it needs to disable the features that SVQ does
> > not support when forwarding buffers. This includes packed vq layout,
> > indirect descriptors or event idx.
> >
> > Future changes can add support to offer more features to the guest,
> > since the use of VirtQueue gives this for free. This is left out at the
> > moment for simplicity.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.h |  2 ++
> >   hw/virtio/vhost-shadow-virtqueue.c | 39 ++++++++++++++++++++++++++++++
> >   hw/virtio/vhost-vdpa.c             | 18 ++++++++++++++
> >   3 files changed, 59 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> > index 1d4c160d0a..84747655ad 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -33,6 +33,8 @@ typedef struct VhostShadowVirtqueue {
> >       EventNotifier svq_call;
> >   } VhostShadowVirtqueue;
> >
> > +bool vhost_svq_valid_features(uint64_t *features);
> > +
> >   void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
> >   void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int call_fd);
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > index 54c701a196..34354aea2c 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -14,6 +14,45 @@
> >   #include "qemu/main-loop.h"
> >   #include "linux-headers/linux/vhost.h"
> >
> > +/**
> > + * Validate the transport device features that both guests can use with the SVQ
> > + * and SVQs can use with the device.
> > + *
> > + * @dev_features  The features. If success, the acknowledged features. If
> > + *                failure, the minimal set from it.
> > + *
> > + * Returns true if SVQ can go with a subset of these, false otherwise.
> > + */
> > +bool vhost_svq_valid_features(uint64_t *features)
> > +{
> > +    bool r = true;
> > +
> > +    for (uint64_t b = VIRTIO_TRANSPORT_F_START; b <= VIRTIO_TRANSPORT_F_END;
> > +         ++b) {
> > +        switch (b) {
> > +        case VIRTIO_F_ANY_LAYOUT:
> > +            continue;
> > +
> > +        case VIRTIO_F_ACCESS_PLATFORM:
> > +            /* SVQ trust in the host's IOMMU to translate addresses */
> > +        case VIRTIO_F_VERSION_1:
> > +            /* SVQ trust that the guest vring is little endian */
> > +            if (!(*features & BIT_ULL(b))) {
> > +                set_bit(b, features);
> > +                r = false;
> > +            }
> > +            continue;
>
>
> It looks to me the *features is only used for logging errors, if this is
> the truth. I suggest to do error_setg in this function instead of
> letting the caller to pass a pointer.
>

I'm fine with passing the Error pointer, but it's not only used for
logging: if SVQ is enabled, the feature set of the device and the
guest is also checked against this.

For example if the vdpa device supports event_idx, this is the
function that the caller uses to filter out that feature bit. Same
with indirect, packed, and all unknown transport ones. It's different
from device's features like CVQ, where this function does not apply.

Now that this is stated, I think the cover letter should reflect that
better, and that this function should include a verb in its name.
Maybe vhost_svq_filter_valid_transport_features is more appropriate.
Let me know if you see that way too.

>
> > +
> > +        default:
> > +            if (*features & BIT_ULL(b)) {
> > +                clear_bit(b, features);
> > +            }
>
>
> Do we need to check indirect and event idx here?
>

These are simply cleared at this moment.

Thanks!

> Thanks
>
>
> > +        }
> > +    }
> > +
> > +    return r;
> > +}
> > +
> >   /** Forward guest notifications */
> >   static void vhost_handle_guest_kick(EventNotifier *n)
> >   {
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index c73215751d..d614c435f3 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -348,11 +348,29 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> >                                  Error **errp)
> >   {
> >       g_autoptr(GPtrArray) shadow_vqs = NULL;
> > +    uint64_t dev_features, svq_features;
> > +    int r;
> > +    bool ok;
> >
> >       if (!v->shadow_vqs_enabled) {
> >           return 0;
> >       }
> >
> > +    r = hdev->vhost_ops->vhost_get_features(hdev, &dev_features);
> > +    if (r != 0) {
> > +        error_setg_errno(errp, -r, "Can't get vdpa device features");
> > +        return r;
> > +    }
> > +
> > +    svq_features = dev_features;
> > +    ok = vhost_svq_valid_features(&svq_features);
> > +    if (unlikely(!ok)) {
> > +        error_setg(errp,
> > +            "SVQ Invalid device feature flags, offer: 0x%"PRIx64", ok: 0x%"PRIx64,
> > +            dev_features, svq_features);
> > +        return -1;
> > +    }
> > +
> >       shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> >       for (unsigned n = 0; n < hdev->nvqs; ++n) {
> >           g_autoptr(VhostShadowVirtqueue) svq = vhost_svq_new();
>