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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7254 bytes --]

On Mon, Jun 01, 2020 at 08:51:43PM +0800, Jason Wang wrote:
> 
> 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.

Good idea. I'll update the vhost-user spec.

> 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

Thanks, will check them and add them if possible.

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

Will fix.

> 
> 
> > 
> > > > + *
> > > > + * 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().

Will fix.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]