Re: [PATCH 11/31] vhost: Add vhost_svq_valid_device_features to shadow vq

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

On Mon, Jan 31, 2022 at 4:49 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Sat, Jan 29, 2022 at 9:11 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/1/22 上午4:27, Eugenio Pérez 写道:
> > > This allows SVQ to negotiate features with the device. For the device,
> > > SVQ is a driver. While this function needs to bypass 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.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   hw/virtio/vhost-shadow-virtqueue.h |  2 ++
> > >   hw/virtio/vhost-shadow-virtqueue.c | 44 ++++++++++++++++++++++++++++++
> > >   hw/virtio/vhost-vdpa.c             | 21 ++++++++++++++
> > >   3 files changed, 67 insertions(+)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> > > index c9ffa11fce..d963867a04 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > > @@ -15,6 +15,8 @@
> > >
> > >   typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> > >
> > > +bool vhost_svq_valid_device_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);
> > >   const EventNotifier *vhost_svq_get_dev_kick_notifier(
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > index 9619c8082c..51442b3dbf 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -45,6 +45,50 @@ const EventNotifier *vhost_svq_get_dev_kick_notifier(
> > >       return &svq->hdev_kick;
> > >   }
> > >
> > > +/**
> > > + * Validate the transport device features that SVQ can use with the device
> > > + *
> > > + * @dev_features  The device features. If success, the acknowledged features.
> > > + *
> > > + * Returns true if SVQ can go with a subset of these, false otherwise.
> > > + */
> > > +bool vhost_svq_valid_device_features(uint64_t *dev_features)
> > > +{
> > > +    bool r = true;
> > > +
> > > +    for (uint64_t b = VIRTIO_TRANSPORT_F_START; b <= VIRTIO_TRANSPORT_F_END;
> > > +         ++b) {
> > > +        switch (b) {
> > > +        case VIRTIO_F_NOTIFY_ON_EMPTY:
> > > +        case VIRTIO_F_ANY_LAYOUT:
> > > +            continue;
> > > +
> > > +        case VIRTIO_F_ACCESS_PLATFORM:
> > > +            /* SVQ does not know how to translate addresses */
> >
> >
> > I may miss something but any reason that we need to disable
> > ACCESS_PLATFORM? I'd expect the vring helper we used for shadow
> > virtqueue can deal with vIOMMU perfectly.
> >
>
> This function is validating SVQ <-> Device communications features,
> that may or may not be the same as guest <-> SVQ. These feature flags
> are valid for guest <-> SVQ communication, same as with indirect
> descriptors one.
>
> Having said that, there is a point in the series where
> VIRTIO_F_ACCESS_PLATFORM is actually mandatory, so I think we could
> use the latter addition of x-svq cmdline parameter and delay the
> feature validations where it makes more sense.
>
> >
> > > +            if (*dev_features & BIT_ULL(b)) {
> > > +                clear_bit(b, dev_features);
> > > +                r = false;
> > > +            }
> > > +            break;
> > > +
> > > +        case VIRTIO_F_VERSION_1:
> >
> >
> > I had the same question here.
> >
>
> For VERSION_1 it's easier to assume that guest is little endian at
> some points, but we could try harder to support both endianness if
> needed.
>

Re-thinking the SVQ feature isolation stuff for this first iteration
based on your comments.

Maybe it's easier to simply fail if the device does not *match* the
expected feature set, and add all of the "feature isolation" later.
While a lot of guest <-> SVQ communication details are already solved
for free with qemu's VirtQueue (indirect, packed, ...), we may
simplify this series in particular and add the support for it later.

For example, at this moment would be valid for the device to export
indirect descriptors feature flag, and SVQ simply forward that feature
flag offering to the guest. So the guest <-> SVQ communication could
have indirect descriptors (qemu's VirtQueue code handles it for free),
but SVQ would not acknowledge it for the device. As a side note, to
negotiate it would have been harmless actually, but it's not the case
of packed vq.

So maybe for the v2 we can simply force the device to just export the
strictly needed features and nothing else with qemu cmdline, and then
enable the feature negotiation isolation for each side of SVQ?

Thanks!


> Thanks!
>
> > Thanks
> >
> >
> > > +            /* SVQ trust that guest vring is little endian */
> > > +            if (!(*dev_features & BIT_ULL(b))) {
> > > +                set_bit(b, dev_features);
> > > +                r = false;
> > > +            }
> > > +            continue;
> > > +
> > > +        default:
> > > +            if (*dev_features & BIT_ULL(b)) {
> > > +                clear_bit(b, dev_features);
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    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 bdb45c8808..9d801cf907 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -855,10 +855,31 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > >       size_t n_svqs = v->shadow_vqs_enabled ? hdev->nvqs : 0;
> > >       g_autoptr(GPtrArray) shadow_vqs = g_ptr_array_new_full(n_svqs,
> > >                                                              vhost_psvq_free);
> > > +    uint64_t dev_features;
> > > +    uint64_t svq_features;
> > > +    int r;
> > > +    bool ok;
> > > +
> > >       if (!v->shadow_vqs_enabled) {
> > >           goto out;
> > >       }
> > >
> > > +    r = vhost_vdpa_get_features(hdev, &dev_features);
> > > +    if (r != 0) {
> > > +        error_setg(errp, "Can't get vdpa device features, got (%d)", r);
> > > +        return r;
> > > +    }
> > > +
> > > +    svq_features = dev_features;
> > > +    ok = vhost_svq_valid_device_features(&svq_features);
> > > +    if (unlikely(!ok)) {
> > > +        error_setg(errp,
> > > +            "SVQ Invalid device feature flags, offer: 0x%"PRIx64", ok: 0x%"PRIx64,
> > > +            hdev->features, svq_features);
> > > +        return -1;
> > > +    }
> > > +
> > > +    shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_psvq_free);
> > >       for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > >           VhostShadowVirtqueue *svq = vhost_svq_new();
> > >
> >