mctpd: accept set endpoint ID as endpoint #96
Conversation
khangng-ampere
force-pushed
the
khangng/push-rquoknsoxuun
branch
from
e842126 to
521f10c
Compare
Currently, mctpd handles Set EID message as a bus owner, which means it assumes it has at least one local EID and rejects all Set Endpoint ID requests. This commit handles the case where mctpd runs on an endpoint and it has no EID set yet. Signed-off-by: Khang D Nguyen <[email protected]>
khangng-ampere
force-pushed
the
khangng/push-rquoknsoxuun
branch
from
521f10c to
e6c85c3
Compare
| #define SET_MCTP_EID_ALLOCATION_STATUS(field, status) \ | ||
| ((field) |= (((status) & MCTP_EID_ALLOCATION_STATUS_MASK) \ | ||
| << MCTP_EID_ALLOCATION_STATUS_SHIFT)) |
There was a problem hiding this comment.
I'm not a huge fan of macros that alter their (scalar, non-pointer) arguments. Could you instead do a:
#define SET_MCTP_EID_ALLOCATION_STATUS(status) \
(((status) & MCTP_EID_ALLOCATION_STATUS_MASK) \
<< MCTP_EID_ALLOCATION_STATUS_SHIFT))and then leave it to the caller to or multiple fields together?
Also applies to SET_EID_OPERATION above too, but that is currently unused anyway.
(apologies for not noticing this on the similar ASSIGNMENT_STATUS macro earlier)
There was a problem hiding this comment.
Ah yeah, I followed SET_MCTP_EID_ASSIGNMENT_STATUS. Let me also change that while I'm at it
| free(addrs); | ||
| } | ||
|
|
||
| // Remove all peers on this interface |
There was a problem hiding this comment.
mainly out of curiosity, but do you have any thoughts on what semantics we would want for our remote peer set in endpoint mode?
There was a problem hiding this comment.
Currently I'm removing all the peers just for simplicity. My preferred solution is to mark all peers as Degraded and perform some sort of revalidation - whether it is via the Get Endpoint ID for each fine-grained endpoint revalidation, or we can do a batch Get Routing Table for a coarse-grained revalidation, and get the new endpoints list. Interface-wise, D-Bus clients should still be able to use the existing recovery interface for all peers, unmanaged or not.
| // reject if we are bus owner | ||
| if (link_data->role == ENDPOINT_ROLE_BUS_OWNER) { | ||
| warnx("Rejected set EID %d because we are the bus owner", | ||
| req->eid); |
There was a problem hiding this comment.
Would be good to include the source EID / address of the message here, if that's feasible. I suspect that this would be helpful to debug such an unexpected case.
("which peer incorrectly thinks it is also a bus owner??")
| // sharing a single EID for multiple interfaces. We will need to: | ||
| // - track the first bus assigned the EID. | ||
| // - policy for propagating EID to other interfaces (see bridge EID options in | ||
| // function comment above) |
There was a problem hiding this comment.
I assume the fallthrough is intentional, but could you add a // fallthrough comment here to be explicit?
| MCTP_SET_EID_POOL_NONE); | ||
| resp->eid_set = req->eid; | ||
| resp->eid_pool_size = 0; | ||
| warnx("Accepted set eid %d\n", req->eid); |
There was a problem hiding this comment.
Also probably not a warning :)
However, I'd suggest we still want this printed unconditionally, as it's a fairly interesting event to log.
(and no newline on warnx, but you'll want to keep this if you change to a fprintf)
| // function comment above) | ||
| case MCTP_SET_EID_FORCE: | ||
|
|
||
| warnx("setting EID to %d", req->eid); |
There was a problem hiding this comment.
More of a debug than a warning, given we would print out the Accepted message below too.
There was a problem hiding this comment.
I was sure warnx() is the same as fprintf(stderr, ...), like so: https://git.musl-libc.org/cgit/musl/tree/src/legacy/err.c
I could convert all of those into explicitly using fprintf(stderr, ...) if you prefer that.
|
|
||
| // When we are assigned a new EID, assume our world view of the network | ||
| // reachable from this interface has been stale. Reset everything. | ||
| clear_interface_addrs(ctx, addr->smctp_ifindex); |
There was a problem hiding this comment.
I figure this ties in to the peer semantics mentioned above. The peers may well still be reachable, but a full reset seems reasonable too.
| assert rsp.hex(' ') == '00 02 00 00 02 00' | ||
|
|
||
| # set EID = 42 | ||
| rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x01, bytes([0x00, 0x42]))) |
There was a problem hiding this comment.
maybe make the addresses (0x42 / 0x66) (const?) vars, so we're checking against the same symbolic values below.
There was a problem hiding this comment.
They are probably better as non-hex, given that's the usual convention for EIDs. Currently there's a decimal/hex mismatch between the comment and value.
| req->eid); | ||
| resp->completion_code = MCTP_CTRL_CC_ERROR_UNSUPPORTED_CMD; | ||
| resp_len = | ||
| sizeof(resp->ctrl_hdr) + sizeof(resp->completion_code); |
There was a problem hiding this comment.
You could use a struct mctp_ctrl_resp for these error cases, if that simplifies the responding.
This change takes a little long for me to push out, mainly because I am not sure how the specification interacts with our MCTP in-kernel infrastructure, where a single EID address can be assigned on multiple interfaces.
I think I mostly know how this maps to the spec now. The spec suggests:
This PR address the endpoint case, where it will accept the EID from every interfaces it is connected to.
Also, this PR also fixes the response for Bus Owner. Instead of rejecting with
0b01 EID assignment rejected, this PR rejects withMCTP_CTRL_CC_ERROR_UNSUPPORTED_CMD. Rejecting EID assignment is reserved for the case where a bridge is rejecting subsequent EID assignments from other bus owners it is connected to.