feat(vhost-user): add a device-type agnostic vhost-user frontend - #6072
feat(vhost-user): add a device-type agnostic vhost-user frontend#60721stvamp wants to merge 2 commits into
Conversation
|
Hi @1stvamp, sorry for a long delay. Taking a very quick look at the description of this PR, I see you decided to keep working on exposing the generic vhost-user device first instead of adapting the existing vhost-user-blk to use generic paths, thus de-duplicating the code and making the at least initial set of changes smaller (since updating vhost-user-blk does not need to touch all the api/config paths). Well, I would really appreciate if you could rework this set of patches to be in another order (the option 2 I mentioned in #5773 (comment)):
The reasoning here is that 1 and 2 are just some internal FC changes which can be reviewed and merged separately and don't require any new CI tests. And the 3 can built on top of them with only minimal changes only required to expose the API and add necessary CI tests. Once you do the change, feel free to ping me or anyone else on the team to take a look. |
|
@ShadowCurse sorry, I misread your orig comment: I saw option 1 by mistake. 🙃 While starting on cutting this up for option 2 I found a read-only negotiation bug in vhost-user-blk, up separately as #6083. I'll redo this in your 1/2/3 order, and keep 1 and 2 together in one PR as you suggested on #5773 with the API exposure stacked on top. I'll ping you once it's up. |
22d31f7 to
2a2bad4
Compare
1stvamp
changed the title
|
@ShadowCurse split up into 3 phases/PRs, hth |
|
@ShadowCurse this is reworked in your 1/2/3 order now, sorry it took a while. It ended up as three PRs rather than the two I said I'd do. You mentioned 1 and 2 could be reviewed and merged separately, and once they were actually split they didn't really want to share a PR, so:
Feature passthrough is implemented now, so the read-only xfail is gone: the frontend passes the backend's features on and withholds only the bits it would have to implement itself (packed rings, a platform IOMMU, notification data, per-queue reset, an admin queue, SR-IOV and dirty page logging). That has a consequence I hadn't expected, a guest offered the backend's features can size itself from the backend's config space, which an agnostic frontend can't parse. VIRTIO_BLK_F_MQ is the one I hit, the guest takes its queue count from the backend and then hangs if the frontend was configured with fewer, so the configured queue count has to agree with the backend and that's in the docs now. One behaviour does change in step 2: block's config space fetch goes through the shared frontend, which requires the backend to return as many bytes as were asked for, so a backend answering a 60 byte request with fewer now fails to attach where before it was accepted. Happy to reshuffle if you'd rather they were combined differently. |
The VIRTIO_BLK_F_RO check read acked_features after it had been reassigned to acked_features & PROTOCOL_FEATURES, so it tested bit 5 against 0x4000_0000 and never matched. read_only was therefore always false, even though the device does ask the backend for VIRTIO_BLK_F_RO, and docs/api_requests/block-vhost-user.md documents the backend as the place a readonly vhost-user drive is configured. read_only feeds Block::read_only(), which decides whether a root device gets ro or rw on the guest kernel cmdline, so a readonly vhost-user root device was given rw. The guest still sees VIRTIO_BLK_F_RO and retries the root mount readonly, so it boots, but /proc/cmdline is wrong and a later remount,rw fails. There is no way to correct this from config: VhostUserBlockConfig::try_from rejects is_read_only, which leaves the backend as the only source of the flag. Compute read_only before acked_features is narrowed, so the shadowing cannot swallow the bit again. test_new_all_features already advertises VIRTIO_BLK_F_RO from its mock backend and asserted the broken result, so it now asserts read_only is set, and test_vhost_user_block asserts ro reaches the guest cmdline. Signed-off-by: Wes Mason <wes@1stvamp.org>
The vhost-user frontends duplicate everything that is not specific to the virtio device type they implement: connecting to the backend socket, negotiating virtio and protocol features, fetching the config space, allocating queues and eventfds, and setting up the backend's vrings on activation. Add VhostUserDevice to hold that shared state, built from a VhostUserDeviceSpec carrying the parts that do vary: socket path, queue count and size, the features to offer, the config space size, whether CONFIG is mandatory, and the metrics name. Device types embed it and keep their own state alongside. Two details worth noting for the callers that follow. Fetching the config space is skipped when the backend does not ack CONFIG, leaving an empty config space, since only a frontend with no device-specific fallback needs to insist on it. And activate() sets up the queues the guest marked ready rather than every allocated queue, because the number a driver initialises is dictated by the backend-owned config space, so initializing an unconfigured queue returns NotReady and aborts activation. Real vring indices are preserved: queues 0 and 2 being ready maps to vrings 0 and 2, not 0 and 1. Activating with no ready queues is rejected rather than quietly setting up nothing. The spec is validated before the socket is touched, since a frontend built from it may be configured at runtime rather than from constants: the queue count is bounded by what the PCI notification region can address, the queue size has to be a power of two, and the config space size has to fit the protocol's own limit. The config space size is a size rather than an upper bound, in that the backend has to return exactly that many bytes. The vhost crate checks the size the reply declares but not the length of the payload behind it, so the length is checked here too. Otherwise a backend could declare the size that was asked for, send fewer bytes, and leave the guest reading past the end of what it sent. No device type uses this yet. Signed-off-by: Wes Mason <wes@1stvamp.org>
2a2bad4 to
099cc7e
Compare
zulinx86
added
the
Status: Awaiting assignee
3 participants
Changes
Adds
VhostUserDevice, a vhost-user frontend that carries no knowledge of the virtio device type it serves. It owns the parts of the protocol that aren't device specific: feature negotiation, the config space fetch, queue allocation and vring setup.Nothing uses it yet. vhost-user-block moves onto it in the next PR of the stack, and the generic device is built on it after that.
Reason
vhost-user-block owns all of this logic today, and a generic vhost-user device needs the same again. Pulling it out on its own keeps the block refactor separate from the new device, so each can be reviewed for one thing at a time.
This is step 1 of the 3 @ShadowCurse asked for in #6072.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.This is part 2 of 4 in a stack made with GitButler: