On Thu, Jul 7, 2022, 17:28 Alex Bennée <[email protected]> wrote:
> > Stefan Hajnoczi <[email protected]> writes: > > > 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. > > I can clean-up the documentation for this. I wonder if the VirtIODevice > backend_features is a duplication that should be removed? > I don't know the code well enough to say, but it's possible that it can be simplified. Stefan
