Re: [PATCH 2/5] vhost: involve device backends in feature negotiation

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Fri, May 29, 2020 at 03:04:54PM +0800, Jason Wang wrote:
> > 
> > On 2020/5/23 上午1:17, Stefan Hajnoczi wrote:
> > > Many vhost devices in QEMU currently do not involve the device backend
> > > in feature negotiation. This seems fine at first glance for device types
> > > without their own feature bits (virtio-net has many but other device
> > > types have none).
> > > 
> > > This overlooks the fact that QEMU's virtqueue implementation and the
> > > device backend's implementation may support different features.  QEMU
> > > must not report features to the guest that the the device backend
> > > doesn't support.
> > > 
> > > For example, QEMU supports VIRTIO 1.1 packed virtqueues while many
> > > existing vhost device backends do not. When the user sets packed=on the
> > > device backend breaks. This should have been handled gracefully by
> > > feature negotiation instead.
> > > 
> > > Introduce vhost_get_default_features() and update all vhost devices in
> > > QEMU to involve the device backend in feature negotiation.
> > > 
> > > This patch fixes the following error:
> > > 
> > >    $ x86_64-softmmu/qemu-system-x86_64 \
> > >        -drive if=virtio,file=test.img,format=raw \
> > >        -chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \
> > >        -device vhost-user-blk-pci,chardev=char0,packed=on \
> > >        -object memory-backend-memfd,size=1G,share=on,id=ram0 \
> > >        -M accel=kvm,memory-backend=ram0
> > >    qemu-system-x86_64: Failed to set msg fds.
> > >    qemu-system-x86_64: vhost VQ 0 ring restore failed: -1: Success (0)
> > 
> > 
> > It looks to me this could be fixed simply by adding VIRTIO_F_RING_PACKED
> > into user_feature_bits in vhost-user-blk.c.
> >
> > And for the rest, we need require them to call vhost_get_features() with the
> > proper feature bits that needs to be acked by the backend.
> 
> There is a backwards-compatibility problem: we cannot call
> vhost_get_features() with the full set of feature bits that each device
> type supports because existing vhost-user backends don't advertise
> features properly. QEMU disables features not advertised by the
> vhost-user backend.
> 
> For example, if we add VIRTIO_RING_F_EVENT_IDX then it will always be
> disabled for existing libvhost-user backends because they don't
> advertise this bit :(.
> 
> The reason I introduced vhost_get_default_features() is to at least
> salvage VIRTIO_F_IOMMU_PLATFORM and VIRTIO_F_RING_PACKED. These bits can
> be safely negotiated for all devices.
> 
> Any new transport/vring VIRTIO feature bits can also be added to
> vhost_get_default_features() safely.
> 
> If a vhost-user device wants to use device type-specific feature bits
> then it really needs to call vhost_get_features() directly as you
> suggest. But it's important to check whether existing vhost-user
> backends actually advertise them - because if they don't advertise them
> but rely on them then we'll break existing backends.

What about adding a VHOST_USER_PROTOCOL_F_BACKEND_F to indicate the
backend knows how to advertise the backend features.

(Although my understanding of virtio feature flags is thin)

Dave

> Would you prefer a different approach?
> 
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index aff98a0ede..f8a144dcd0 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -48,6 +48,23 @@ static unsigned int used_memslots;
> > >   static QLIST_HEAD(, vhost_dev) vhost_devices =
> > >       QLIST_HEAD_INITIALIZER(vhost_devices);
> > > +/*
> > > + * Feature bits that device backends must explicitly report. Feature 
> > > bits not
> > > + * listed here maybe set by QEMU without checking with the device 
> > > backend.
> > > + * Ideally all feature bits would be listed here but existing vhost 
> > > device
> > > + * implementations do not explicitly report bits like 
> > > VIRTIO_F_VERSION_1, so we
> > > + * can only assume they are supported.
> > 
> > 
> > For most backends, we care about the features for datapath. So this is not
> > true at least for networking device, since VIRTIO_F_VERSION_1 have impact on
> > the length of vnet header length.
> 
> The net device is in good shape and doesn't use vhost_default_feature_bits[].
> 
> vhost_default_feature_bits[] is for devices that haven't been
> negotiating feature bits properly. The goal is start involving them in
> feature negotiation without breaking existing backends.
> 
> Would you like me to rephrase this comment in some way?
> 
> > > + *
> > > + * New feature bits added to the VIRTIO spec should usually be included 
> > > here
> > > + * so that existing vhost device backends that do not support them yet 
> > > continue
> > > + * to work.
> > 
> > 
> > It actually depends on the type of backend.
> > 
> > Kernel vhost-net does not validate GSO features since qemu can talk directly
> > to TAP and vhost doesn't report those features. But for vhost-user GSO
> > features must be validated by qemu since qemu can't see what is behind
> > vhost-user.
> 
> Maybe the comment should say "New transport/vring feature bits". I'm
> thinking about things like VIRTIO_F_RING_PACKED that are not
> device-specific but require backend support.
> 
> The GSO features you mentioned are device-specific. Devices that want to
> let the backend advertise device-specific features cannot use
> vhost_default_feature_bits[].
> 
> > > + */
> > > +static const int vhost_default_feature_bits[] = {
> > > +    VIRTIO_F_IOMMU_PLATFORM,
> > > +    VIRTIO_F_RING_PACKED,
> > > +    VHOST_INVALID_FEATURE_BIT
> > > +};
> > > +
> > >   bool vhost_has_free_slot(void)
> > >   {
> > >       unsigned int slots_limit = ~0U;
> > > @@ -1468,6 +1485,11 @@ uint64_t vhost_get_features(struct vhost_dev 
> > > *hdev, const int *feature_bits,
> > >       return features;
> > >   }
> > > +uint64_t vhost_get_default_features(struct vhost_dev *hdev, uint64_t 
> > > features)
> > > +{
> > > +    return vhost_get_features(hdev, vhost_default_feature_bits, 
> > > features);
> > > +}
> > 
> > 
> > There's probably no need for a new helper, we can do all these inside
> > vhost_get_features().
> 
> Would you prefer:
> 
>   extern const int vhost_default_feature_bits[];
> 
> And then callers do:
> 
>   vhost_get_features(hdev, vhost_default_feature_bits, features);
> 
> ?


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK