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

[Jason Wang]

On 2020/5/29 下午9:56, Stefan Hajnoczi 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 :(.


Agree.


> 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?


I get you now so I think not.

Maybe we need document the expected behavior of VHOST_USER_GET_FEATURES 
e.g which set of features that must be advertised explicitly.

And for the set you mention here, we probably need to add:

VIRTIO_F_ORDER_PLATFORM,
VIRTIO_F_ANY_LAYOUT (not sure if it was too late).

And

VIRTIO_F_IN_ORDER


> > > 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?


That will be better.


> > > + *
> > > + * 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);
>
> ?


That's fine or maybe just do features |= vhost_default_feature_bits 
inside vhost_get_features().

Thanks