[multicast] viona CTRL_RX/CTRL_MAC receive filtering + softnpu egress lock scoping + dep updates - #1093
[multicast] viona CTRL_RX/CTRL_MAC receive filtering + softnpu egress lock scoping + dep updates#1093zeeshanlakhani wants to merge 10 commits into
Conversation
This wires up the softnpu ASIC backend to support multicast end-to-end by translating DPD's sidecar.p4 table operations into sidecar-lite.p4's simplified P4 pipeline. ## AsicMulticastOps We replace the stubbed AsicMulticastOps implementation (which returned "OperationUnsupported" for group creation and port addition with in-memory group tracking via McGroupData, following the tofino_stub pattern. Group membership is used by the table translation layer to build port bitmaps for sidecar-lite's Replicate extern. Ports >= 128 are rejected at add time to match sidecar-lite's 128-bit bitmap width. ## Table translation (asic/src/softnpu/table.rs) We map sidecar.p4 table names to sidecar-lite equivalents and translate action parameters where the designs differ for emulation. All multicast action arms are gated with #[cfg(feature = "multicast")]. ## References - [softnpu #183](oxidecomputer/softnpu#183) - [propolis #1093](oxidecomputer/propolis#1093) - [p4rs #240](oxidecomputer/p4#240) - [sidecar-lite #152](oxidecomputer/sidecar-lite#152) - tokio: 1.50 (due to softnpu) - oxide-tokio-rt: 0.1.3 (following-up from tokio's move to 1.50)
This wires up the softnpu ASIC backend to support multicast end-to-end by translating DPD's sidecar.p4 table operations into sidecar-lite.p4's simplified P4 pipeline. ## AsicMulticastOps We replace the stubbed AsicMulticastOps implementation (which returned "OperationUnsupported" for group creation and port addition with in-memory group tracking via McGroupData, following the tofino_stub pattern. Group membership is used by the table translation layer to build port bitmaps for sidecar-lite's Replicate extern. Ports >= 128 are rejected at add time to match sidecar-lite's 128-bit bitmap width. ## Table translation (asic/src/softnpu/table.rs) We map sidecar.p4 table names to sidecar-lite equivalents and translate action parameters where the designs differ for emulation. All multicast action arms are gated with #[cfg(feature = "multicast")]. ## References - [softnpu #183](oxidecomputer/softnpu#183) - [propolis #1093](oxidecomputer/propolis#1093) - [p4rs #240](oxidecomputer/p4#240) - [sidecar-lite #152](oxidecomputer/sidecar-lite#152) - tokio: 1.50 (due to softnpu) - oxide-tokio-rt: 0.1.3 (following-up from tokio's move to 1.50)
bad9d65 to
784de5e
Compare
This wires up the softnpu ASIC backend to support multicast end-to-end by translating DPD's sidecar.p4 table operations into sidecar-lite.p4's simplified P4 pipeline. ## AsicMulticastOps We replace the stubbed AsicMulticastOps implementation (which returned "OperationUnsupported" for group creation and port addition with in-memory group tracking via McGroupData, following the tofino_stub pattern. Group membership is used by the table translation layer to build port bitmaps for sidecar-lite's Replicate extern. Ports >= 128 are rejected at add time to match sidecar-lite's 128-bit bitmap width. ## Table translation (asic/src/softnpu/table.rs) We map sidecar.p4 table names to sidecar-lite equivalents and translate action parameters where the designs differ for emulation. ## Fix update semantics We fix an issue with stale in-memory sources after update with absent source list(s). `modify_group_external` wrote back `new.sources.or(existing)` to the in-memory record while `update_external_tables` in the same call canonicalized `new.sources` directly to rewrite the P4 source-filter entries. A `None` update therefore cleared the P4 table but left a stale `Some([...])` in memory. All multicast action arms are gated with #[cfg(feature = "multicast")]. ## References and Deps - [softnpu #183](oxidecomputer/softnpu#183) - [propolis #1093](oxidecomputer/propolis#1093) - [p4rs #240](oxidecomputer/p4#240) - [sidecar-lite #152](oxidecomputer/sidecar-lite#152) - tokio: 1.50 (due to softnpu) - oxide-tokio-rt: 0.1.3 (following-up from tokio's move to 1.50) - bump oxide-tokio-rt to 0.1.4, tokio to 1.52.1 (matching Omicron)
75518ad to
0de0c5b
Compare
0de0c5b to
a9779dc
Compare
a9779dc to
95c195a
Compare
Pin softnpu and p4rs to their zl/multicast branches for multicast support in the emulated ASIC, and bump oxide-tokio-rt to 0.1.4.
This work extends the minimal `VIRTIO_NET_F_CTRL_RX` support from [1125](#1125) to the full control-queue receive filtering surface, including promiscuity levels and guest-supplied unicast/multicast MAC tables being applied to the in-kernel device via the new [VNA_IOC_SET_MAC_FILTERS](https://code.oxide.computer/c/illumos-gate/+/775) ioctl. Filter state is cleared on device reset and carried across live migration via `VionaStateV1`. The payload is exported unconditionally, matching the import behavior on master since #1125, which takes the payload without consulting negotiated features. Import still tolerates a missing payload from sources predating VionaStateV1. The device narrows to classified delivery only once the guest installs a non-empty multicast table. An empty table holds the device at its all-multicast default, where some drivers (illumos vioif among them) negotiate `CTRL_RX` but never manage a filter table, and narrowing on an empty table would silently drop their multicast. The virtio spec permits the resulting unrequested traffic, but not the loss. On falcon links, a host without ALL_VLAN support now falls back to full promiscuity rather than all-multicast, since classified unicast delivery would drop transit frames for the emulated fabric. When viona interface version 7 (kernel-side MAC filter table support in st.louis) is available as per https://code.oxide.computer/c/illumos-gate/+/775 (in review), then this is fully lit up.
Every port worker and the guest Tx path serialize on the pipeline mutex. Previously, the guard was held across both the dlpi and virtio egress, meaning that one worker blocked in egress I/O stalled the others. This is especially costly for multicast, where a single input frame fans out to several ports for replication. This change makes the lock scoped to pipeline evaluation only. The egress packets borrow from the input frame rather than the pipeline, so they remain valid after the guard is dropped. Lock acquisition moves from the callers into `process_external_packet` and `process_guest_packet`, which now take the mutex directly. The latter function returns a bool so the guest read loop can stop early when no P4 program is loaded or the lock is poisoned, preserving the prior break semantics.
95c195a to
cd0e261
Compare
FelixMcFelix
left a comment
There was a problem hiding this comment.
Thanks Zeeshan; hopefully these comments makes sense, and thanks for the extra tests around the actual handling of control queue messages. I am partly looking at this from the angle of minimising the amount of things we need to do in viona/MAC, and from keeping as many (guest ⨉ test environment) pairs out of promiscuous mode as is doable.
Here are the follow-ups from review of the CTRL_RX/CTRL_MAC receive filtering work: - Withhold classified delivery until the first accepted MAC_TABLE_SET since reset. illumos vioif negotiates CTRL_RX but never programs the multicast table, so the all-multicast lower bound holds until the driver demonstrates that it can manage its own filtering. - We now carry that marker across migration as an optional payload field (multicast_table_managed). Older sources omit it and the target infers from non-empty tables, while older targets ignore it. - Track table installation independently of promiscuity (mac_filters_dirty), so guest promiscuity toggles no longer unplumb and replumb the MAC client's classified flows. Failed installs and clearings stay dirty and are reissued on a later application. - We scope the falcon promiscuity pin to softnpu-attached links rather than forcing every falcon VM promiscuous. - Return `Result<(), NoP4Program>` from softnpu's `process_guest_packet` instead of just a bool. - Write MAC filter entries directly into `vmf_mcast` rather than collecting them into an intermediate vector. - Bump HEADER_CHECK_REF to gerrit patchset 4.
We now advertise `VIRTIO_NET_F_CTRL_MAC_ADDR` when the kernel offers the V7 MAC filtering ioctls, and implement `VIRTIO_NET_CTRL_MAC_ADDR_SET` by swapping the active unicast address through `VNA_IOC_SET_MAC_ADDR`. Driver writes to the config-space `mac` field remain ignored because only the control-queue command is honored. The kernel removes the current address (and the installed multicast filters) before installing the replacement, so that the swap runs under full promiscuity (or the falcon pin), with `apply_rx_config` reconciling the level and the filter table afterward. Failure outcomes are decoded from the semantic codes copied out through `vioc_mac_addr`: - a partial table restore (`VMA_ERR_MCAST_RESTORE`) that keeps the new address and reissues the table. - a failed swap with the previous address still installed, which fails the command and reissues the table (the kernel may shed entries while restoring it). - a client left with no unicast address, which demands a device reset. DTrace probes report the copied-out error state of failed filter and swap ioctls. The override installed by the guest driver feeds config-space MAC reads and unicast filter classification, yields to the nominal address on device reset, and is carried in the migration payload. Targets install it before reapplying the Rx configuration and refuse the import without kernel support. This work also includes: - mirroring the [775, patchset-6](https://code.oxide.computer/c/illumos-gate/+/775) `vioc_mac_filters` layout (out-of-band table through `vmf_addrs`) in viona-api. - passing a null pointer for an empty table. - bumping the header-check ref to patchset 6. - tracing the new ioctls in scripts/viona.d. - testing replacement, reset, and migration of the override against a live V7 kernel, asserting the kernel-visible address and filter table through test-only GET ioctls.
1fb9a90 to
6885fe0
Compare
6885fe0 to
2fb6852
Compare
2fb6852 to
603194b
Compare
603194b to
3c07d60
Compare
FelixMcFelix
left a comment
There was a problem hiding this comment.
Thanks Zeeshan. I think I'm generally happy with the actual plumbing of the filter tables, as well as the logic to set/unset various promisc levels in response to the error conditions aside from maybe one or two points of confusion. It looks good on the functionality front, though I haven't yet tested it myself and made sure that everything looks right in mdb for various guests yet.
I'd need to defer to @iximeow or @papertigers on the VirtioDevice changes, as well as the accuracy of the new tests which are actually driving the rings via send_ctrl_cmd.
| // An empty table is never installed. Classified delivery with | ||
| // no table covers the device MAC and broadcast, which is | ||
| // exactly what an empty table requests. |
There was a problem hiding this comment.
Guests do often start with an empty table from what I recall, then add the pile of all-routers, etc., related MACs. These tables are assumed to refer to MACs in addition to the config space MAC and broadcast. I think it's just a wording thing for this comment, more than anything?
| // SET_PAIRS can fail while restoring the kernel's receive callbacks, | ||
| // and SET_USEPAIRS can fail independently. Attempt both restorations, | ||
| // and on failure surface NEEDS_RESET to the guest rather than | ||
| // panicking. | ||
| let use_pairs = self.set_use_pairs(1); | ||
| let pairs = self.hdl.set_pairs(1); | ||
| if use_pairs.is_err() || pairs.is_err() { | ||
| self.virtio_state.set_needs_reset(self); | ||
| } |
There was a problem hiding this comment.
If we're at this point we are resetting the NIC, so I'm unsure that signalling needs_reset will be of much use. What exactly is the failure condition for these ioctls, now?
Leaving aside the addition/split of VirtioDevice::device_reset for now, why are we resetting the queuepair counts in a separate spot from the rest of the device logic? #1093 (comment) makes it seem as though this logic should live there too, if the guest forces a reset.
There was a problem hiding this comment.
Both points make sense.
On the failure conditions: VNA_IOC_SET_USEPAIRS(1) reaches viona_ioc_link_usepairs. The request is within the valid range, so the validation failure paths do not apply here. VNA_IOC_SET_PAIRS(1) reaches viona_ioc_link_setpairs, which can fail if the requested count is below l_usepairs, if a ring is not in VRS_RESET, or if viona_rx_set fails while restoring receive callbacks.
The first two conditions are not reachable here: we lower l_usepairs first, and virtio_reset has already reset every ring before invoking device_reset. The viona_rx_set failure is recoverable, however, as it drops the active mode to VIONA_PROMISC_NONE while retaining the request in l_promisc, so the promiscuity restore below retries callback installation.
This makes needs_reset both unnecessary and misleading honestly, so it is now gone. The promiscuity restore in device_reset is the retry for that callback-restoration failure.
On placement, yeah..., it is a bug rather than an inconsistency. A guest writing zero to the status register goes through set_status to virtio_reset and then device_reset, but it does not go through Lifecycle::reset. A driver that negotiated multiqueue and then unloaded therefore left the kernel holding its old pair count.
Resetting the pair count in device_reset fixes that path. The split itself was needed for resetting the VIRTIO_NET_F_CTRL_RX state. Restoring the pair count is an additional benefit of having the device-level reset callback.
Refs:
- https://github.com/oxidecomputer/illumos-gate/blob/e73f2575430340bc2ebdfcb0b5d49ef8fa72d816/usr/src/uts/intel/io/viona/viona_main.c#L1403-L1423
- https://github.com/oxidecomputer/illumos-gate/blob/e73f2575430340bc2ebdfcb0b5d49ef8fa72d816/usr/src/uts/intel/io/viona/viona_main.c#L1426-L1432
- https://github.com/oxidecomputer/illumos-gate/blob/e73f2575430340bc2ebdfcb0b5d49ef8fa72d816/usr/src/uts/intel/io/viona/viona_main.c#L1034-L1066.
| /// Whether the source had accepted a `MAC_TABLE_SET` since its last | ||
| /// reset. | ||
| /// | ||
| /// This is optional for compatibility, as older sources omit it and | ||
| /// older destinations ignore it. | ||
| #[serde(default)] | ||
| pub multicast_table_managed: Option<bool>, | ||
| /// The unicast MAC address installed by the driver in place of the | ||
| /// nominal one the device was created with, if any | ||
| /// (`VIRTIO_NET_CTRL_MAC_ADDR_SET`). | ||
| /// | ||
| /// This is optional for compatibility, as older sources omit it. | ||
| /// | ||
| /// An older destination ignores it and reverts config space to the | ||
| /// nominal address, while the guest continues using the replaced | ||
| /// one, which that host will not classify. | ||
| #[serde(default)] | ||
| pub mac_addr: Option<MacAddr>, |
There was a problem hiding this comment.
My feeling is that this might have to be a V2 payload, because if we do migrate from a propolis with this capability (and which allows guest-driven MAC address config) to an older version, the guest will be left in a very strange state where it believes the host knows about its brand new MAC address but the kernel and PCI cfg-space are not thusly configured.
There was a problem hiding this comment.
Hmm.
I do not think going to V2 buys anything here, because the migration already fails closed. No Viona migration payload has shipped yet, and this PR introduces the first schema. A destination predating it therefore does not ignore the payload: it never claims it, and the import driver treats unconsumed offers as an error and fails the migration. The state you describe, where the guest believes in a MAC that the target kernel and config space do not have, is unreachable for that specific pairing.
What is actually wrong is the documentation. My bad. Both fields claimed that older sources omit them and older destinations ignore them, but no such peer exists. This is now fixed. multicast_table_managed is also now a plain bool, since nothing can send a V1 payload without it, and the inference fallback on import was dead code anyway. mac_addr remains an Option<MacAddr>, because None genuinely means that no override is installed. Finally, the DataMissing branch remains, since old source to new destination is the direction that does work.
The mismatched-policy pairing is already handled as well, as import fails outright when a carried override arrives at a host with allow_guest_mac_change turned off.
viona: CTRL_RX and CTRL_MAC table receive filtering
This work extends the minimal
VIRTIO_NET_F_CTRL_RXsupport from #1125 to coverCTRL_RXpromiscuity/all-multicast controls andCTRL_MACtableset handling. Propolis records the guest-supplied unicast and multicast MAC tables and installs the multicast table on the in-kernel device via the newVNA_IOC_SET_MAC_FILTERSioctl. The unicast table is used to decide when the device must fall back to full promiscuity, since the kernel MAC client already carries the primary unicast address and does not install additional unicast filters.Filter state is cleared on device reset and carried across live migration via
VionaStateV1. The payload is exported unconditionally, matching the import behavior on master since #1125, which takes the payload without consulting negotiated features. Import still tolerates a missing payload from sources predatingVionaStateV1. The device narrows to classified delivery once the driver's firstMAC_TABLE_SETsince reset is accepted (empty or not). Until then, the all-multicast lower bound holds, because some drivers, including illumosvioif, negotiateCTRL_RXbut never actually manage a filter table. That marker is carried across migration as an optionalmulticast_table_managedfield, where older sources omit it (the target infers it from non-empty tables) and older targets ignore it. Table installation is tracked independently of promiscuity, so guest promiscuity toggles leave installed classified flows in place rather than unplumbing and replumbing them accordingly.On falcon links attached to softnpu, promiscuity is pinned at construction rather than derived from guest filter state. A host without
ALL_VLANsupport pins to full promiscuity, because classified unicast delivery would drop transit frames for the emulated fabric. Other falcon VMs are no longer forced promiscuous.viona: guest MAC replacement via CTRL_MAC_ADDR
This adds
VIRTIO_NET_F_CTRL_MAC_ADDRsupport, letting a driver replace the device's active unicast MAC address viaVIRTIO_NET_CTRL_MAC_ADDR_SET. Propolis installs the replacement on the in-kernel device through the newVNA_IOC_SET_MAC_ADDRioctl (viona interface version 7) and reflects it in the device configuration space the driver reads the MAC from. The kernel removes the current address and, with it, the installed multicast filters, before installing the replacement. Therefore, the device raises full promiscuity (or the falcon superset pin) to cover delivery across the swap, and then reconciles promiscuity and the filter table with the resulting state.The override is carried across live migration in
VionaStateV1as an optionalmac_addrfield, and device reset swaps the nominal address back in.Advertising the feature requires an explicit
allow_guest_mac_changepolicy grant (default off), since viona performs the swap as directed and leaves validating the requested address against host policy to its consumer. propolis-server keeps the grant off until Nexus supplies an allowed-MAC policy, while propolis-standalone accepts it as a device option. A destination without the grant rejects migration imports that carry the negotiated bit or an installed override.softnpu: release pipeline lock before egress I/O
Every port worker and the guest Tx path serialize on the pipeline mutex. Previously, the guard was held across both the dlpi and virtio egress, meaning that one worker blocked in egress I/O stalled the others. This is especially costly for multicast, where a single input frame fans out to several ports for replication.
This change scopes the lock to pipeline evaluation (only). The egress packets borrow from the input frame rather than the pipeline, so they remain valid after the guard is dropped. Lock acquisition moves from the callers into
process_external_packetandprocess_guest_packet, which now take the mutex directly.deps and callouts
softnpuis pinned to itszl/multicastbranch pending upstream.oxide-tokio-rtis bumped to match omicron.