vhost-user-backend: Fix SET_VRING_KICK should not disable the vring

[germag]

Summary of the PR

This was introduced in 7f326dd314fdea0f98a19fa039c630b50deaec0b, but doesn't work with qemu versions < 7.2. SET_FEATURES is received before SET_VRING_KICK, we will enable the vrings in set_features() and disabled them later in set_vring_kick().

Currently, the libvhostuser in qemu disables the vring upon receiving RESET_OWNER, but that message is currently deprecated. There is not a sane place to disable the vring, since according to the spec we can only do that upon receiving RESET_OWNER (deprecated), RESET_DEVICE (currently not supported in this crate), and SET_VRING_ENABLE.

The current state of the vhost-user spec is a mess, we need a more formal spec that just a wall of english.

It also, fixes an incorrect comment.

[stefano-garzarella]

This issue affects vhost-user-backend v0.12.0 and v0.13.0, so at least we should release a v0.13.1

[Ablu]

Is there a discussion ongoing upstream that we could link for reference?

[stefano-garzarella]

I tested with vhost-device-vsock and I confirm that this PR fixes the issue with QEMU < 7.2 (where PROTOCOL_FEATURES was not negotiated by QEMU).

[stefano-garzarella]

@germag I'm fine with this, I'd just suggest adding a mention in the commit description that QEMU < 7.2 doesn't negotiate PROTOCOL_FEATURES. The rest LGTM.

[germag]

Is there a discussion ongoing upstream that we could link for reference?

Not sure what you mean, I just found this error today after updating virtiofsd deps, so this is the upstream discussion I guess :)

[Ablu]

Not sure what you mean, I just found this error today after updating virtiofsd deps, so this is the upstream discussion I guess :)

Sorry, my understanding was that something is iffy in the spec. At least https://qemu-project.gitlab.io/qemu/interop/vhost-user.html still says:

"The back-end must start a ring upon receiving a kick (that is, detecting that file descriptor is readable) on the descriptor specified by VHOST_USER_SET_VRING_KICK".

If that does not really work due to backwards-compatibility I assume the spec needs a change?

Or do I misunderstand something here?

[stefano-garzarella]

Not sure what you mean, I just found this error today after updating virtiofsd deps, so this is the upstream discussion I guess :)

Sorry, my understanding was that something is iffy in the spec. At least https://qemu-project.gitlab.io/qemu/interop/vhost-user.html still says:

"The back-end must start a ring upon receiving a kick (that is, detecting that file descriptor is readable) on the descriptor specified by VHOST_USER_SET_VRING_KICK".

If that does not really work due to backwards-compatibility I assume the spec needs a change?

Or do I misunderstand something here?

IIUC, with commit https://github.com/rust-vmm/vhost/commit/7f326dd314fdea0f98a19fa039c630b50deaec0b we introduced a regression for backwards-compatibility (when PROTOCOL_FEATURES is not negotiated), that we are fixing with this PR. But we are still following the spec.

[Ablu]

Ah, I got confused and thought we no longer enabled it now. LGTM