Fix backend compatibility with QEMU as frontend #22
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Quoting from the vhost-user spec[1]: > With this protocol extension negotiated, the sender (QEMU) can set the > need_reply [Bit 3] flag to any command. This indicates that the client > MUST respond with a Payload VhostUserMsg indicating success or > failure. The payload should be set to zero on success or non-zero on > failure, unless the message already has an explicit reply body. We were only checking that the VHOST_USER_PROTOCOL_F_REPLY_ACK protocol feature had been negotiated, not that the per-message need_reply flag was set. This meant that frontends would receive replies they weren't expecting. [1]: https://qemu.readthedocs.io/en/latest/interop/vhost-user.html#vhost-user-protocol-f-reply-ack Signed-off-by: Alyssa Ross <hi@alyssa.is>
Quoting an email[1] from Michael S. Tsirkin, who wrote the vhost-user spec[2]: > However, this is for functionality dealing with guest activity. > VHOST_USER_SET_PROTOCOL_FEATURES has nothing to do with guest directly, > it's about negotiation between qemu and backend: it is only in > VHOST_USER_GET_FEATURES for the reason that this is the only message > (very) old backends reported. Thus, the backend should not check > whether VHOST_USER_SET_FEATURES sets VHOST_USER_F_PROTOCOL_FEATURES, > instead it should simply always be ready to receive > VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES. So the check that the VHOST_USER_F_PROTOCOL_FEATURES bit has been acked that we were doing before updating the REPLY_ACK flag was incorrect. And since we don't need to wait for SET_FEATURES any more before updating the flag, there shouldn't be any need to call update_reply_ack_flag in the SET_FEATURES handler. SET_PROTOCOL_FEATURES still won't enable features immediately if GET_FEATURES and GET_PROTOCOL_FEATURES haven't happened yet, but that should be fine because if the frontend didn't send GET_FEATURES, it doesn't know that protocol features are supported, and if it didn't send GET_PROTOCOL_FEATURES, it wouldn't know which features it can enable. So it wouldn't make sense for a client to send SET_PROTOCOL_FEATURES before either getter message anyway. [1]: https://lore.kernel.org/qemu-devel/20200805181352-mutt-send-email-mst@kernel.org/ [2]: https://qemu.readthedocs.io/en/latest/interop/vhost-user.html Signed-off-by: Alyssa Ross <hi@alyssa.is>
|
Fixed in 2e4396c. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Wasn’t sure whether to PR this here or to @arronwy’s dragonball branch, which I see is a PR to rust-vmm’s copy of the repo.
I’ve been trying to use cloud-hypervisor’s vhost-user-net backend with QEMU. In doing so, I’ve found a few compatibility problems resulting from cloud-hypervisor not quite correctly following the vhost-user spec. This series contains the changes to this crate required to use the cloud-hypervisor vhost-user-net with QEMU. There’s another thing that’ll have to be fixed in the cloud-hypervisor tree itself.