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.

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);

?