[viona, mac-filter, mcast] install guest multicast MAC filter tables via V7 ioctls - #1210
zeeshanlakhani wants to merge 5 commits into
Conversation
This work fixes a [known issue](#1053) where legacy interface writes could arrive after feature negotiation ended or a `DRIVER_OK` flag was set, causing side-effects and unwanted reconfiguration. We fix this with a guard before `set_features` that checks that negotiation has closed first, setting `NEEDS_RESET` (discarding the write) if a different mask is requested. A write that produces the same mask is just a no-op here. For modern, common interfaces, writes after `FEATURES_OK` could mutate the state's `negotiated_features` without calling `set_features` itself, causing the mask to diverge from what the device had already applied. This came up in iximeow's [comment](#1053 (comment)), which proposed `NEEDS_RESET` for feature writes between `FEATURES_OK` and `DRIVER_OK`. To handle this, we apply a similar methodology, but for `CommonConfigReg::DriverFeature` once negotiation is closed. We also reject retried `FEATURES_OK` flag sets while `NEEDS_RESET` status is already set. We need these changes for upcoming viona work involving setting MAC filters.
zeeshanlakhani
changed the title
…via V7 ioctls Guest multicast interest has been driven by `VIONA_PROMISC_MULTI`, delivering multicast frames for the guests to filter. We saw duplication issues at guest Rx when packets reached both the classified and promiscuous callbacks. This led to the viona interface V7 scoped changes in [illumos-gate](https://github.com/oxidecomputer/illumos-gate/blob/5ffff4b86e486e1f9d7860be1368386699a7829a) / [change 775](https://code.oxide.computer/c/illumos-gate/+/775). Given the upstream changes, this work introduces `VNA_IOC_SET_MAC_FILTERS` to install guest multicast tables on the underlay MAC client backing viona. `VNA_IOC_GET_MAC_FILTERS` is used for reading back installed entries from the table. Classified delivery can replace all-multicast mode once the guest supplies a table that fits and no broader receive mode is necessary. All-multicast remains the fallback for guests that haven't sent a table request or have exceeded the kernel's capacity. Most of the work here goes into handling ioctl failures, semantic result codes, and fallback. This includes explicit multicast table management, reset handling, and Falcon-build `AllVlan` logic. Most importantly (most scary), this raises the minimum required host viona API to V7. We're keeping the V1 viona state payload for now, with import / export handling for the multicast table state. This is the first of a few different PRs upcoming, including - [ ] CTRL_RX receive-mode handling - [ ] send notification on `NEEDS_RESET` status
63d156a to
8c7cc6a
Compare
- merge in #1212 when accepted.
iximeow
left a comment
iximeow
left a comment
There was a problem hiding this comment.
I've left a bunch of thoughts spanning a few levels of detail, sorry for the mix going up and down - I kind of read this top to bottom in diff order and had different levels of thought along the way.
since I'd really like to get Propolis to match the stlouis VIONA_CURRENT_INTERFACE_VERSION before we cut the next release, I think what I'll do is a separate PR that bumps the expected interface version, and we can decouple the specifics of using the new MAC filtering ioctls separately. my understanding of the stlouis change is that until we're using those ioctls, the viona behavior is not any different. so the extant behavior around compiling a list of filters into viona promiscuity is fine today, and would be fine to switch away from at some point in the future? I'll tag you on that impending change but please shout here or there (or chat of course) if it sounds like I've misunderstood the kernel side here.
| /// Whether the guest's MAC table management is active. | ||
| /// | ||
| /// An accepted table, including an empty one, allows | ||
| /// [`PciVirtioViona::rx_config`] to release the all-multicast | ||
| /// lower bound. | ||
| /// | ||
| /// This bound exists for illumos vioif, which negotiates | ||
| /// `VIRTIO_NET_F_CTRL_RX` but never programs a multicast | ||
| /// table: <https://www.illumos.org/issues/18280>. |
There was a problem hiding this comment.
this is a bit confusing to me: is this field not necessary for correct operation anywhere else? I think as-is we would need it to correctly describe the device's behavior in the transitory period after the guest has negotiated F_CTRL_RX and before a driver has submitted a filter table. otherwise (for example) if you migrate in that period the destination VM would believe multicast_mac_filters represented the source's viona state and install an empty list, dropping all multicast traffic while the source was still passing it.
I figure that's a pretty small window, but the comment here looks like we're working around guest quirks when the bit is necessary to describe a legal (albeit strange) device state.
| /// Whether a multicast filter table is installed on the in-kernel MAC | ||
| /// client via `VNA_IOC_SET_MAC_FILTERS`. | ||
| mac_filters_installed: bool, | ||
| rx_config_failed: bool, |
There was a problem hiding this comment.
this seems kind of funky too: we're tracking if an rx configuration-related operation failed at some point so we can reject future attempts to change the viona rx configuration? if we've failed to set the configuration and set NEEDS_RESET on the device, should we be standing in the way of future viona rx changes?
my expectation is that viona makes no promises as to the device state if an ioctl fails. so, if an rx configuration errors for any reason, we'll already be in a situation where Propolis' understanding of the guest state may not match viona (e.g. we'll have changed Propolis state just after some if state.rx_config_failed, but possibly not have actually changed viona's configuration to match). i don't think we can avoid the possibility of some skew there either way, because if we wait until an ioctl succeeds to update Propolis' side of things, then an ioctl that fails may have applied a change while we don't update Propolis' side due to the failure. so i think we're just going to have some skew either way.
that means that if a migration happens while rx_config_failed is set, all we know is the thing we're migrating might not be the state of viona that was migrated from. but we do not know if there is actually config skew. so the best this can tell us is "the guest may see different device behavior after migration", which is probably good to know for debugging purposes but we'd probably want to build differently with that purpose as a goal?
namely: if we've set NEEDS_RESET then the guest SHOULD NOT rely on the completion of operations, but i don't think there is a reason we should prevent the guest from trying. if the guest sees a NEEDS_RESET after trying to set a bogus set of filters and the guest "fixes it" by setting a different set of filters, i could imagine that manifesting as user observing "the driver does something weird, then a user changes a NIC setting and the hardware tolerates that", where rx_config_failed would probably just keep the device in a stuck and undesired state.
a guest should not depend on any of that working, but i don't see a reason we should go out of our way to make it not work.
| // A pre-MAC-filter target version restores `promisc`, but does not | ||
| // install the migrated multicast table. | ||
| // | ||
| // We export all-multicast mode when delivery currently depends | ||
| // on that table, so the migration does not drop multicast. | ||
| let promisc = if state.mac_filters_installed | ||
| && state.promisc == PromiscLevel::None | ||
| { | ||
| PromiscLevel::AllMulti | ||
| } else { | ||
| state.promisc | ||
| }; |
There was a problem hiding this comment.
so.. if a guest sets up a virtio-net device and provides one unicast MAC and no multicast MACs, we'll migrate it out at PromiscLevel::AllMulti regardless of what the promiscuity was, and on the other end .. ignore the promiscuity computed here? I don't follow why we're going through this.
also: if state.promisc was AllVlan before migrating, but the guest had set a MAC filter, we'll switch to AllMulti here and the destination's check that if the source promiscuity was AllVlan then the destination's promiscuity must be on import will be bypassed. but then in that case the destination would get to AllVlan after the next rx_config() so i think it would appear to work while all the plumbing points in different directions?
| #[derive(Copy, Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] | ||
| pub enum PromiscMode { | ||
| None, | ||
| AllMulti, | ||
| All, | ||
| AllVlan, | ||
| } | ||
|
|
||
| impl From<PromiscLevel> for PromiscMode { | ||
| fn from(value: PromiscLevel) -> Self { | ||
| match value { | ||
| PromiscLevel::None => Self::None, | ||
| PromiscLevel::AllMulti => Self::AllMulti, | ||
| PromiscLevel::All => Self::All, | ||
| #[cfg(feature = "falcon")] | ||
| PromiscLevel::AllVlan => Self::AllVlan, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
why have this in addition to PromiscLevel? is this to permit migrating state between Falcon and non-Falcon builds of Propolis, where AllVlan would disappear from the wire? I don't think that's something we'd want to go out of our way to support, but I'll be the first to admit I don't really know much about Falcon.
| // Note: `multicast_table_managed` comes after V1 has already shipped. | ||
| // A payload from older sources omits this field, defaulting to false and | ||
| // retaining all-multicast mode. Pre-MAC-filter targets will just ignore it. |
There was a problem hiding this comment.
we still aren't live-migrating, there's not really any need to ensure compatibility for migration payloads. if we were live-migrating, we wouldn't be changing old payloads (the general plan is to follow more along the lines of the HTTP API versioning where old versions are immediately frozen in time forever)
| assert!(!state.mac_table_set); | ||
| assert_eq!(state.promisc, PromiscLevel::AllMulti); | ||
| } | ||
| assert!(driver.status_ok()); |
There was a problem hiding this comment.
probably should assert that get_mac_filters() returns an empty list here too, right?
| #[cfg(not(feature = "falcon"))] | ||
| fn mac_filters_host_promisc_pin(test_ctx: TestCtx) -> TestCtx { | ||
| test_ctx | ||
| } |
There was a problem hiding this comment.
this maybe could be #[cfg(feature = "falcon")] testcase!(mac_filters_host_promisc_pin) on the list below, instead?
| match self.take_schema(T::id()) { | ||
| Some(mut offer) => offer.parse(), | ||
| None => match self.0.iter().find(|offer| offer.kind == T::id().0) { | ||
| Some(offer) => Err(MigrateStateError::UnexpectedPayload( | ||
| offer.kind.into(), | ||
| offer.version, | ||
| )), | ||
| None => Err(MigrateStateError::DataMissing), | ||
| }, | ||
| } |
There was a problem hiding this comment.
i really don't understand what's going on here or in the tests below. this looks like plumbing that's still around from trying to support a VionaStateV2? i'm not super familiar with the device migration payload bits and i'm not sure either of us want to get into that for this change, at least...
| // Until guest MAC table management has been established, we | ||
| // keep all-multicast delivery enabled. | ||
| // | ||
| // illumos vioif negotiates CTRL_RX but never sends a table. | ||
| // Narrowing it to classified delivery after a promiscuity cycle could | ||
| // drop its multicast traffic: <https://www.illumos.org/issues/18280>. | ||
| // | ||
| // Note: Unrequested traffic is permitted by the VirtIO spec. | ||
| let promisc = if need_promisc { | ||
| PromiscLevel::All | ||
| } else if need_mcast { | ||
| } else if need_mcast_promisc || !state.mac_table_set { | ||
| PromiscLevel::AllMulti | ||
| } else { | ||
| PromiscLevel::None | ||
| }; |
There was a problem hiding this comment.
i'm even less familiar with the networking space than virtio-net, which i am very much learning as-needed; we're setting the device up to interpret an empty multicast table in two different ways based on if the guest has ever set one before. but if the guest wants to not get multicast traffic, shouldn't we support CTRL_RX_EXTRA and CTRL_RX_NOMULTI? is there guest driver support for that? or, is it typical to set an empty table and expect that to mean "no traffic"?
i don't really know what the spec suggests for this case. filtering is a "SHOULD", so passing extra traffic is always okay. but "no MAC in the empty list matched, so don't pass the traffic" also seems like a valid reading. do you have a sense of what guest drivers are expecting to work with here, or is this more "it makes illumos consistently work so we'll have some extra ifs in viona.rs"?
| state.unicast_mac_filters = unicast; | ||
| state.multicast_mac_filters = multicast; | ||
| state.mac_table_set = true; |
There was a problem hiding this comment.
since we're using mac_table_set to mean "has the guest ever given us MAC filters" ... maybe we should just have an struct GuestMacFilters { unicast, multicast } and Option<GuestMacFilters>?
|
@iximeow, thanks for the updated review.
I do think it makes sense to cut the V7 version update separately. I mainly wanted to try and begin the work on what approaching this interface would be like, and I'll separately go through your review here for sure. Let me know when you have the bump up, so we can get that in as soon as we can. |
2 participants
Guest multicast interest has been driven by
VIONA_PROMISC_MULTI, delivering multicast frames for the guests to filter. We saw duplication issues at guest Rx when packets reached both the classified and promiscuous callbacks. This led to the viona interface V7 scoped changes in illumos-gate / change 775.Given the upstream changes, this work introduces
VNA_IOC_SET_MAC_FILTERSto install guest multicast tables on the underlay MAC client backing viona.VNA_IOC_GET_MAC_FILTERSis used for reading back installed entries from the table. Classified delivery can replace all-multicast mode once the guest supplies a table that fits and no broader receive mode is necessary. All-multicast remains the fallback for guests that haven't sent a table request or have exceeded the kernel's capacity.Most of the work here goes into handling ioctl failures, semantic result codes, and fallback. This includes explicit multicast table management, reset handling, and Falcon-build
AllVlanlogic.Most importantly (most scary), this raises the minimum required host viona API to V7. We're keeping the V1 viona state payload for now, with import / export handling for the multicast table state.
Note: This work will fail on CI as don't have special handling. Chat with @iximeow for more.
This is the first of a few different PRs upcoming, including
NEEDS_RESETstatus