mctpd: Support MCTP Discovery Notify command - #165
Conversation
4bee8eb to
9274ea2
Compare
In MCTP networks, endpoints broadcast or send a Discovery Notify control request (0x0D) to inform the Bus Owner when they boot up, reset, or are hot-plugged. Currently, mctpd relies on active bus scanning or static configurations, leaving newly online endpoints undiscovered until the next poll cycle. This change adds support for handling Discovery Notify requests when mctpd is in the Bus Owner role: - Immediately acknowledges Discovery Notify requests over the physical socket with MCTP_CTRL_CC_SUCCESS inside handle_control_discovery_notify() to prevent hardware endpoints (e.g. PCIe VDMs / SMBus sequencers) from dropping subsequent Set Endpoint ID requests while waiting for an ACK. - Asynchronously defers EID assignment and route programming (RTM_NEWROUTE) to the systemd event loop (sd_event_add_defer) to prevent thread blocking. - Safely handles event deferral failures by dropping the request and returning -ENOMEM rather than executing synchronously in receive context, eliminating re-entrance and event loop corruption risks. - Scopes request deduplication to (ifindex, dest_phys) composite keys to prevent cross-bus address collisions on multi-bus platforms. - Enforces per-interface rate-limiting (maximum 5 discovery requests per second) to protect against discovery request floods. - Correctly supports Bridge Endpoints by reserving EID pools and invoking endpoint_allocate_eids() for downstream pool allocation and gateway routing. - Includes comprehensive unit test coverage in test_mctpd.py and test_mctpd_endpoint.py. Assisted-by: Antigravity:Gemini-Next Signed-off-by: Jasmine Cha <chajasmine@google.com>
9274ea2 to
f6fbc3b
Compare
|
I see there's been a few updates; let me know when this is stable and you'd like a review. |
The code is ready for review, please take a look at this. Thanks. |
jk-ozlabs
left a comment
jk-ozlabs
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I have a few comments.
On the commit message: it reads like a marketing pitch rather than an explanation of the design, or rationale for the implementation. While you don't need to describe the individual changes, I would appreciate some background on the approach, and non-obvious parts of the implementation (like, why the new NLM_F flags?)
In MCTP networks, endpoints broadcast or send a Discovery Notify control
request (0x0D) to inform the Bus Owner when they boot up, reset, or are
hot-plugged. Currently, mctpd relies on active bus scanning or static
configurations,
or primarily: hot-plug events, where the transport provides them
leaving newly online endpoints undiscovered until the next poll cycle.
What poll cycle?
| return -1; | ||
| } | ||
| msg.nh.nlmsg_type = RTM_NEWROUTE; | ||
| msg.nh.nlmsg_flags |= NLM_F_CREATE | NLM_F_REPLACE; |
There was a problem hiding this comment.
I can't see why we need different flags for the route creation for the discovery notify path. If this is needed, could you explain this in the commit message, or in a comment here?
| struct { | ||
| sd_event_source **sources; | ||
| } bridge_ep_poll; | ||
| uint32_t flags; |
There was a problem hiding this comment.
Why a new flag set for just one flag?
| /* | ||
| * Rate limiting is evaluated per-link (interface) rather than per-physical-MAC address | ||
| * for performance and simplicity. On shared multi-drop buses (e.g. I2C/SMBus), a noisy | ||
| * endpoint could temporarily consume the 5 req/sec quota; healthy adjacent endpoints will | ||
| * naturally retransmit Discovery Notify upon timeout. | ||
| */ |
There was a problem hiding this comment.
This comment is more of an argument against a per-link rate-limit. Can you detail why per-link instead of per-endpoint? If it's just for simplicity of the implementation, then say so.
But is it more difficult to track per physaddr anyway?
|
|
||
| if (link_data->discovery_count >= 5) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Minor, but please try to be consistent with your coding style (comparing to the if block on line 1367)
I have a slight preference for no-braces on single-statement blocks.
| bool assign_bridge; | ||
| int rc; | ||
|
|
||
| (void)s; /* Floating event source unref is managed automatically by sd_event */ |
| rc = endpoint_assign_eid(dctx->ctx, NULL, &dctx->phys, &peer, | ||
| target_eid, assign_bridge); |
There was a problem hiding this comment.
Is this sufficient? Are we sure that we should be re-assigning the old EID here?
As above: if we receive a Discovery Notify for a pre-existing peer, it seems that we have lost state synchronisation with that peer, which may be more like a recovery operation rather than a simple EID assignment. Your change doesn't really explain what we intend to do in that case.
| dest_phys_tostr(&dctx->phys), strerror(-rc)); | ||
| } | ||
|
|
||
| /* Keep pending context linked until execution completes to prevent deduplication races */ |
There was a problem hiding this comment.
The wording of comment doesn't make sense here. Perhaps something like:
/* We have kept the context linked until this point, to prevent deduplication races, but
* we are safe to unlink now.
*/
| dctx->next = ctx->pending_discoveries; | ||
| ctx->pending_discoveries = dctx; | ||
|
|
||
| if (sd_event_add_defer(ctx->event, NULL, deferred_assign_eid_cb, dctx) < |
There was a problem hiding this comment.
Your implementation keeps a list of deferred discovery operations, but then schedules a new callback event for each. Would it make more sense just to have one callback queued?
| if (!allow_bridged && is_eid_in_bridge_pool(n, ctx, eid)) | ||
| return -EEXIST; | ||
|
|
||
| /* Re-use existing physical peer mapping safely via change_peer_eid */ |
There was a problem hiding this comment.
How is this related to the discovery implementation?
| assert rsp.hex(' ') == '01 0d 00' | ||
|
|
||
| # Allow deferred EID assignment event to execute | ||
| await trio.sleep(0.1) |
There was a problem hiding this comment.
This looks like a source of future unreliability, and will be difficult to debug. Is there a better way to synchronise rather than relying on delays?
2 participants
Implement support for the MCTP Discovery Notify control command in mctpd. When an MCTP endpoint issues a Discovery Notify control request to the Bus Owner, mctpd immediately acknowledges the request over the physical socket and defers EID assignment to the main systemd event loop.
This avoids blocking the event thread during control message processing and safely handles EID re-assignments via change_peer_eid(), keeping D-Bus object paths and netlink kernel routing tables synchronized. Also include unit test coverage for Discovery Notify in the test suite.
Assisted-by: Antigravity:Gemini-Next