On Thu, Mar 04, 2021 at 06:11:26PM +0000, Alex Bennée wrote: > > Stefan Hajnoczi <stefa...@redhat.com> writes: > > > On Wed, Mar 03, 2021 at 05:01:05PM -0500, Michael S. Tsirkin wrote: > >> On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote: > >> Also, are we sure it's ok to send the messages and then send > >> VHOST_USER_SET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES clear? > >> Looks more like a violation to me ... > > > > Looking again I agree it would be a violation to omit > > VHOST_USER_F_PROTOCOL_FEATURES in VHOST_USER_SET_FEATURES. > > > > Previously I only looked at VHOST_USER_SET_PROTOCOL_FEATURES where the > > spec says: > > > > Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in > > ``VHOST_USER_GET_FEATURES``. > > > > So negotiation is *not* necessary for sending > > VHOST_USER_SET_PROTOCOL_FEATURES. > > > > However, I missed that other features *do* require negotiation of > > VHOST_USER_F_PROTOCOL_FEATURES according to the spec. For example: > > > > If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the > > ring is initialized in an enabled state. > > > > Now I think: > > > > 1. VHOST_USER_F_PROTOCOL_FEATURES *must* be included > > VHOST_USER_SET_FEATURES if the master supports it. > > So added by the master - still invisible to the guest?
The relationship between a guest-visible VIRTIO device and a vhost-user device is not covered by any specification AFAIK. My opinion is that VMMs should not expose VHOST_USER_F_PROTOCOL_FEATURES to the guest since that bit has a different meaning in VIRTIO. If VMMs do expose it to the guest then they'll probably get away with it since bit 30 is unused in VIRTIO. Drivers will probably not enable the bit, even if the device reports it. > > > > 2. VHOST_USER_SET_PROTOCOL_FEATURES does not require negotiation, > > instead the master just needs to check that > > VHOST_USER_F_PROTOCOL_FEATURES is included in the > > VHOST_USER_GET_FEATURES reply. It's an exception. > > OK I'm now thoroughly confused but I guess that's a good thing. However > if we make the changes to QEMU to honour this won't we break with > existing vhost-user receivers? We'll also need to track the state of a > SET_FEATURES has happened and then gate the sending of things like > reply_ack requests? (To avoid confusion below when I write about negotiating VHOST_USER_F_PROTOCOL_FEATURES it means sending VHOST_USER_SET_FEATURES with the feature bit enabled. It shouldn't be confused with sending VHOST_USER_SET_PROTOCOL_FEATURES to negotiate protocol features.) I went through the spec and checked where VHOST_USER_F_PROTOCOL_FEATURES must be negotiated. There is only one place: ring initialization state (enabled/disabled). This makes sense: for backwards compatibility the device backend needs to know whether or not the master supports VHOST_USER_F_PROTOCOL_FEATURES. There are also two messages where the spec says they "should" only be sent when VHOST_USER_F_PROTOCOL_FEATURES has been negotiated: VHOST_USER_SET_VRING_ENABLE and VHOST_USER_SET_SLAVE_REQ_FD. However, QEMU's vhost-user master implementation actually sends VHOST_USER_SET_SLAVE_REQ_FD before negotiating VHOST_USER_F_PROTOCOL_FEATURES. I *think* VHOST_USER_SET_VRING_ENABLE is really only sent after VHOST_USER_F_PROTOCOL_FEATURES has been negotiated but I haven't checked all code paths. All other mentions of VHOST_USER_F_PROTOCOL_FEATURES in the spec only require checking VHOST_USER_GET_FEATURES, not negotiation. So QEMU follows the current spec pretty closely. I don't anything needs to be changed in QEMU. Have you found something? Stefan
signature.asc
Description: PGP signature
