Stefan Hajnoczi <stefanha@gmail.com> writes:
> On Thu, 7 Jul 2022 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Stefan Hajnoczi <stefanha@redhat.com> 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