Re: vhost-user question about VHOST_USER_F_PROTOCOL_FEATURES

[Michael S. Tsirkin]

On Thu, Oct 12, 2023 at 09:20:28PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> We now have a problem in downstream:
> 
> We have a vhost-user server, which doesn't support 
> VHOST_USER_SET_VRING_ENABLE. As I understand, vhost-user specification allows 
> it. So the server behaves as follows:
> 
> 1. in GET_FEATURES, it sets VHOST_USER_F_PROTOCOL_FEATURES, to report support 
> for protocol features.
> 
> 2. if this flag VHOST_USER_F_PROTOCOL_FEATURES is set in SET_FEATURES, it 
> reports an error
> 
> In my opinion, this doesn't violate the specification, maybe I'm wrong.

well it doesn't but it's a quality of implementation issue.
if you set a feature bit in GET_FEATURES you should fully expect clients
to set this bit.

> Newer QEMU, after commits
> 
>     02b61f38d357490 "hw/virtio: incorporate backend features in features"
>     and
>     4daa5054c599c8a
>       "vhost: enable vrings in vhost_dev_start() for vhost-user devices"
> 
> actually doesn't support such behavior, as QEMU assumes that if we have the 
> flag in GET_FEATURES, it is supported by SET_FEATURES.
> 
> 
> I don't see any possibility to clearly support in QEMU both servers which 
> supports vring enable/disable and older servers that doesn't.
> 
> =====
> 
> I also want to clarify,
> 
> 1. is "feature negotiated" means set both in GET_FEATURES and SET_FEATURES, 
> as this term is not directly defined.

generally both but yes, it is unfortunately vague in virtio spec too.

> 2. What VHOST_USER_SET_VRING_ENABLE should do for vhost-user-blk? What meant 
> by "side effects" in this case? All IO requests should just fail, not 
> touching the actual storage?

we discussed this recently upstream.
I think most devices should just not process the ring.
network is one of exceptions where you can harmlessly discard packets and
guest does not care much.


> 3. Except for VHOST_USER_SET_VRING_ENABLE, there is another command require 
> not just VHOST_USER_F_PROTOCOL_FEATURES being present in GET_FEATURES, but 
> "negotiated" i.e., as I understand being then set in SET_OPTIONS by client. 
> That's VHOST_USER_SET_SLAVE_REQ_FD. Why? Is it a mistake?

generally frontend must set a bit before using it.
this way backend knows what is going to be used.


> 4. Also, in VHOST_USER_SET_FEATURES definition:
> 
>          Feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` signals
>   back-end support for ``VHOST_USER_GET_PROTOCOL_FEATURES`` and
>   ``VHOST_USER_SET_PROTOCOL_FEATURES``.
> 
> 
> That seems wrong, as the flag in SET command is about vring enable/disable 
> related behavior.

I think we meant frontend use not backend support.

> -- 
> Best regards,
> Vladimir