On Fri, May 29, 2020 at 03:29:13PM +0100, Dr. David Alan Gilbert wrote:
> * 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)

Luckily the feature bits we've missed out on are mostly legacy features
or features we always want to enable, so I'm not sure a solution for
negotiating all feature bits is needed.

Not all features should be controlled by the backend since vhost devices
often are a strict subset of a VIRTIO device.

Documenting which feature bits are controlled by the backend seems like
a reasonable way of clarifying the current state and preventing similar
issues in the future.

Stefan