On Thu, 7 Jul 2022 at 14:42, Alex Bennée <[email protected]> wrote: > > > Stefan Hajnoczi <[email protected]> writes: > > > [[PGP Signed Part:Undecided]] > > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote: > >> Hi, > >> > >> This series ostensibly adds virtio-user-gpio stubs to the build for > >> use with an external vhost-user daemon. We've been testing it with our > >> rust daemons from: > >> > >> https://github.com/rust-vmm/vhost-device > >> > >> Getting the test enabled took some doing most likely because the need > >> for CONFIG support exercised additional paths in the code that were > >> not used for the simpler virtio-net tests. As a result the series has > >> a number of cleanup and documentation patches. > >> > >> The final thing that needed fixing was the ensuring that > >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation > >> process. This was the hardest thing to track down as we store the > >> feature bits in several places variously as: > >> > >> in VirtIODevice as: > >> uint64_t guest_features; > >> uint64_t host_features; > >> uint64_t backend_features; > > > > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's > > unfiltered feature bits should never be passed to VirtIODevice. > > > >> > >> in vhost_dev as: > >> uint64_t features; > >> uint64_t acked_features; > >> uint64_t backend_features; > > > > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES > > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw > > vhost-user GET_FEATURES. > > So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set > with the VHOST_USER_SET_FEATURES message? Currently it's fed via: > > uint64_t features = vhost_dev->acked_features; > > in vhost_dev_set_features() which does apply a few extra bits > (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding > VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the > vhost-user backend that this should be done? Overload the function?
A modern vhost-user server replies to VHOST_USER_GET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES set. That's when the vhost-user client encounters this bit. The vhost-user client should then filter out VHOST_USER_F_PROTOCOL_FEATURES because it belongs to the vhost-user protocol and isn't a real VIRTIO feature bit. The client uses the filtered VIRTIO feature bits and it now knows whether the vhost-user server supports the VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES messages. I think vhost_user_set_features() should set VHOST_USER_F_PROTOCOL_FEATURES if the server returned it from VHOST_USER_GET_FEATURES. At the moment vhost_user_backend_init() stores VHOST_USER_F_PROTOCOL_FEATURES in struct vhost_dev->backend_features, which only seems to be used by vhost-net code. The other vhost-user devices set acked_features = guest_features and ignore backend_features. As a result I guess they don't set VHOST_USER_F_PROTOCOL_FEATURES in the VHOST_USER_SET_FEATURES message. Most vhost-user servers probably don't care and still respond to VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES messages (although the vhost-user protocol spec mentions other protocol differences when VHOST_USER_F_PROTOCOL_FEATURES is not negotiated). Does this match what you've found? The code is a maze so I may have gotten something wrong. In general I think hw/virtio/vhost-user.c should be responsible for VHOST_USER_F_PROTOCOL_FEATURES and no other part of the QEMU codebase should ever see the bit since it's a vhost-user protocol detail and not part of VIRTIO or even a common part of vhost. Stefan
