Conversation
| │ VMBus Channel │ | ||
| │ │ | ||
| │ ┌──────────────────┐ ┌──────────────────┐ │ | ||
| │ │ Incoming Ring │ │ Outgoing Ring │ │ | ||
| │ │ (guest → host) │ │ (host → guest) │ │ | ||
| │ └────────┬─────────┘ └────────┬─────────┘ │ | ||
| │ │ │ │ | ||
| │ GPADL-backed memory (guest-allocated) │ | ||
| │ │ | ||
| │ Signal: guest → host │ Signal: host → guest │ | ||
| │ Target VP: set at open time │ | ||
| └────────────────────────────────────────────────┘ |
There was a problem hiding this comment.
Nit:
- put a box around GPADL-backed memory (guest-allocated)
- The | | between Incoming Ring and Outgoing Ring don't line up
Why is this change being made? - The VMBus channel model — identity tuples, ring buffer pairs, subchannels, target VP, and lifecycle — is foundational to understanding any VMBus device but was not documented in the Guide. - VP index vs APIC ID vs Linux CPU number confusion comes up frequently in debugging. What changed? - New Guide page: `Guide/src/reference/architecture/vmbus/channels.md` covering channel identity (`OfferKey`), ring buffer pairs, subchannel lifecycle (mermaid state diagram), target VP semantics, VP index vs CPU number vs APIC ID clarification, ring buffer model, and key types. - New `VMBus Architecture` section in SUMMARY.md. - Expanded rustdoc for [`VmbusDevice`](https://openvmm.dev/rustdoc/linux/vmbus_channel/channel/trait.VmbusDevice.html) — `max_subchannels()` and `retarget_vp()` now document semantics and usage patterns. - Expanded rustdoc for [`ChannelControl`](https://openvmm.dev/rustdoc/linux/vmbus_channel/channel/struct.ChannelControl.html) — documents error handling for `enable_subchannels()` and device protocol handler usage context. How was the change tested? - ✅ `cargo doc --no-deps -p vmbus_channel` — no warnings - ✅ Guide cross-links verified Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattkur
force-pushed
the
guide/vmbus-channels
branch
from
e4c7bcd to
14db471
Compare
There was a problem hiding this comment.
Pull request overview
Adds foundational documentation for the VMBus channel model (identity tuples, ring buffers, subchannels, target VP semantics, and lifecycle) to reduce recurring debugging confusion around VP/CPU/APIC identifiers, and aligns relevant rustdoc with the documented behavior.
Changes:
- Adds a new Guide reference page describing the VMBus channel model (
channels.md) and wires it into the Guide navigation. - Expands rustdoc on
VmbusDeviceandChannelControlto clarify subchannel limits, VP retargeting semantics, and expected caller behavior. - Documents a concrete error-mapping expectation for
enable_subchannels()over-limit requests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vm/devices/vmbus/vmbus_channel/src/channel.rs | Clarifies rustdoc around subchannel limits, subchannel enablement, and interrupt retargeting callbacks. |
| Guide/src/reference/architecture/vmbus/channels.md | New architecture doc page explaining VMBus channel/subchannel concepts and VP identifier mapping. |
| Guide/src/SUMMARY.md | Adds a new navigation section for VMBus architecture docs and links the new Channels page. |
| @@ -140,8 +159,10 @@ impl ChannelControl { | |||
| /// | |||
| /// If more than `count` subchannels are already enabled, this does nothing. | |||
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| This is used when VPs come online/offline (e.g., CPU hot-remove) and | ||
| the guest needs to rebalance channel assignments. | ||
|
|
||
| ### VP index, CPU number, and APIC ID |
There was a problem hiding this comment.
@jstarks , @smalis-msft, @chris-oo , could you review this section? I am not confident in its correctness, but this is based on what I have observed.
| VMBus server at offer time. The channel's lifecycle is: **offered → | ||
| opened → active → closed**. | ||
|
|
||
| ```text |
There was a problem hiding this comment.
Does this diagram really add anything?
There was a problem hiding this comment.
Personally: I'm a visual person, so like having the boxes with an incoming and outgoing ring. This helps me conceptualize VMBus channels as rings.
| This is used when VPs come online/offline (e.g., CPU hot-remove) and | ||
| the guest needs to rebalance channel assignments. | ||
|
|
||
| ### VP index, CPU number, and APIC ID |
There was a problem hiding this comment.
I feel like this could be its own page somewhere else with just a link from here. It's an important topic, and not vmbus specific
There was a problem hiding this comment.
Agreed. Probably makes sense to move it to the page I add #2975, or even its own section. I can refactor immediately after I take all these PRs to avoid too much structural churn. Do you mind confirming correctness of this section as written?
There was a problem hiding this comment.
I think it's right but Chris should double check
There was a problem hiding this comment.
talked with matt offline, but we assume that vp_index == linux vcpu number via the bootshim, see #190 and validate_vp_hw_ids in openhcl_boot
There was a problem hiding this comment.
I agree this VP/APIC ID/CPU discussion should not be in this file. But it looks good.
will-j-wright
left a comment
will-j-wright
left a comment
There was a problem hiding this comment.
This all seems right to me, but @SvenGroot might want to take a read through for correctness.
| ordering matches VP index ordering (panics if not), and controls the | ||
| CPU online sequence to maintain the mapping. | ||
|
|
||
| The APIC ID is a separate concept. On x86, the APIC ID may not |
There was a problem hiding this comment.
I would probably reword this section to note that "Each platform has its own architectural way of describing cpus, with x86 APIC IDs and MPIDR on AArch64. Note that these values cannot be assumed to map directly to VP index, as the physical or virtual topology of a system determines the values for these architectural identifiers."
| ``` | ||
|
|
||
| In OpenHCL today, the VP index is used directly as the Linux CPU | ||
| number (`let cpu = vp.vp_index().index()`). This is a simplifying |
There was a problem hiding this comment.
I would say "This mapping is guaranteed by openhcl_boot, as this mapping in a general purpose guest is not guaranteed. This simplifies code throughout OpenHCL, as there is no need to maintain a separate mapping for Linux CPU number to hypervisor VP indices.".
| the OpenHCL threadpool. In OpenVMM, it maps to a dedicated worker | ||
| thread (without physical CPU affinity). The strength of the targeting |
There was a problem hiding this comment.
This openvmm bit will probably change soon. Hopefully I'll remember to update the guide.
The VMBus channel model — identity tuples, ring buffer pairs, subchannels, target VP, and lifecycle — is foundational to understanding any VMBus device but was not documented in the Guide. VP index vs APIC ID vs Linux CPU number confusion comes up frequently in debugging.
Changes
architecture/vmbus/channels.mdcovering channel identity (OfferKey), ring buffer pairs (diagram), subchannel lifecycle (mermaid state diagram), target VP semantics, VP index vs CPU number vs APIC ID clarification, ring buffer queuing model, and key types table.VMBus Architecturesection in SUMMARY.md.VmbusDevicetrait —max_subchannels()now documents that it's the device's upper bound (not current count) and how the framework uses it;retarget_vp()now documents when it's called and what devices should do.ChannelControl— documents theenable_subchannels()error case and usage context (called from device protocol handlers like StorVSP'sCREATE_SUB_CHANNELS).Can be reviewed independently from the StorVSP channels (#2976) and CPU scheduling (#2975) PRs.