From 19e21041dad3e4fa9181b4625c278c794777c0e0 Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Mon, 11 Aug 2025 09:37:32 +0700 Subject: [PATCH 1/2] mctpd: remove assignments from SET_* macros Preparing the values and letting the caller OR multiple fields together should be preferred over assigning to their arguments. Signed-off-by: Khang D Nguyen --- src/mctp-control-spec.h | 36 ++++++++++++++++++------------------ src/mctpd.c | 8 +++++--- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/mctp-control-spec.h b/src/mctp-control-spec.h index f691c5f..45f3b5b 100644 --- a/src/mctp-control-spec.h +++ b/src/mctp-control-spec.h @@ -238,9 +238,9 @@ struct mctp_ctrl_resp_resolve_endpoint_id { #define MCTP_EID_ASSIGNMENT_STATUS_SHIFT 0x4 #define MCTP_EID_ASSIGNMENT_STATUS_MASK 0x3 -#define SET_MCTP_EID_ASSIGNMENT_STATUS(field, status) \ - ((field) |= (((status) & MCTP_EID_ASSIGNMENT_STATUS_MASK) \ - << MCTP_EID_ASSIGNMENT_STATUS_SHIFT)) +#define SET_MCTP_EID_ASSIGNMENT_STATUS(status) \ + (((status) & MCTP_EID_ASSIGNMENT_STATUS_MASK) \ + << MCTP_EID_ASSIGNMENT_STATUS_SHIFT) #define MCTP_SET_EID_ACCEPTED 0x0 #define MCTP_SET_EID_REJECTED 0x1 @@ -263,26 +263,26 @@ struct mctp_ctrl_resp_resolve_endpoint_id { #define MCTP_ENDPOINT_TYPE_MASK 0x3 #define MCTP_SIMPLE_ENDPOINT 0 #define MCTP_BUS_OWNER_BRIDGE 1 -#define SET_ENDPOINT_TYPE(field, type) \ - ((field) |= \ - (((type) & MCTP_ENDPOINT_TYPE_MASK) << MCTP_ENDPOINT_TYPE_SHIFT)) +#define SET_ENDPOINT_TYPE(type) \ + (((type) & MCTP_ENDPOINT_TYPE_MASK) << MCTP_ENDPOINT_TYPE_SHIFT) #define MCTP_ENDPOINT_ID_TYPE_SHIFT 0 #define MCTP_ENDPOINT_ID_TYPE_MASK 0x3 #define MCTP_DYNAMIC_EID 0 #define MCTP_STATIC_EID 1 -#define SET_ENDPOINT_ID_TYPE(field, type) \ - ((field) |= (((type) & MCTP_ENDPOINT_ID_TYPE_MASK) \ - << MCTP_ENDPOINT_ID_TYPE_SHIFT)) +#define MCTP_STATIC_EID_MATCHING_PRESENT 2 +#define MCTP_STATIC_EID_NOT_MATCHING_PRESENT 3 +#define SET_ENDPOINT_ID_TYPE(type) \ + (((type) & MCTP_ENDPOINT_ID_TYPE_MASK) << MCTP_ENDPOINT_ID_TYPE_SHIFT) /* MCTP Routing Table entry types * See DSP0236 v1.3.0 Table 27. */ #define MCTP_ROUTING_ENTRY_PORT_SHIFT 0 #define MCTP_ROUTING_ENTRY_PORT_MASK 0x1F -#define SET_ROUTING_ENTRY_PORT(field, port) \ - ((field) |= (((port) & MCTP_ROUTING_ENTRY_PORT_MASK) \ - << MCTP_ROUTING_ENTRY_PORT_SHIFT)) +#define SET_ROUTING_ENTRY_PORT(port) \ + (((port) & MCTP_ROUTING_ENTRY_PORT_MASK) \ + << MCTP_ROUTING_ENTRY_PORT_SHIFT) #define GET_ROUTING_ENTRY_PORT(field) \ (((field) >> MCTP_ROUTING_ENTRY_PORT_SHIFT) & \ MCTP_ROUTING_ENTRY_PORT_MASK) @@ -291,9 +291,9 @@ struct mctp_ctrl_resp_resolve_endpoint_id { #define MCTP_ROUTING_ENTRY_ASSIGNMENT_TYPE_MASK 0x1 #define MCTP_DYNAMIC_ASSIGNMENT 0 #define MCTP_STATIC_ASSIGNMENT 1 -#define SET_ROUTING_ENTRY_ASSIGNMENT_TYPE(field, type) \ - ((field) |= (((type) & MCTP_ROUTING_ENTRY_ASSIGNMENT_TYPE_MASK) \ - << MCTP_ROUTING_ENTRY_ASSIGNMENT_TYPE_SHIFT)) +#define SET_ROUTING_ENTRY_ASSIGNMENT_TYPE(type) \ + (((type) & MCTP_ROUTING_ENTRY_ASSIGNMENT_TYPE_MASK) \ + << MCTP_ROUTING_ENTRY_ASSIGNMENT_TYPE_SHIFT) #define GET_ROUTING_ENTRY_ASSIGNMENT_TYPE(field) \ (((field) >> MCTP_ROUTING_ENTRY_ASSIGNMENT_TYPE_SHIFT) & \ MCTP_ROUTING_ENTRY_ASSIGNMENT_TYPE_MASK) @@ -304,9 +304,9 @@ struct mctp_ctrl_resp_resolve_endpoint_id { #define MCTP_ROUTING_ENTRY_BRIDGE_AND_ENDPOINTS 0x01 #define MCTP_ROUTING_ENTRY_BRIDGE 0x02 #define MCTP_ROUTING_ENTRY_ENDPOINTS 0x03 -#define SET_ROUTING_ENTRY_TYPE(field, type) \ - ((field) |= (((type) & MCTP_ROUTING_ENTRY_TYPE_MASK) \ - << MCTP_ROUTING_ENTRY_TYPE_SHIFT)) +#define SET_ROUTING_ENTRY_TYPE(type) \ + (((type) & MCTP_ROUTING_ENTRY_TYPE_MASK) \ + << MCTP_ROUTING_ENTRY_TYPE_SHIFT) #define GET_ROUTING_ENTRY_TYPE(field) \ (((field) >> MCTP_ROUTING_ENTRY_TYPE_SHIFT) & \ MCTP_ROUTING_ENTRY_TYPE_MASK) diff --git a/src/mctpd.c b/src/mctpd.c index 2ba0a59..7b1377c 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -719,10 +719,12 @@ static int handle_control_get_endpoint_id(struct ctx *ctx, int sd, mctp_ctrl_msg_hdr_init_resp(&resp->ctrl_hdr, req->ctrl_hdr); resp->eid = local_addr(ctx, addr->smctp_ifindex); + + resp->eid_type = 0; if (ctx->default_role == ENDPOINT_ROLE_BUS_OWNER) - SET_ENDPOINT_TYPE(resp->eid_type, MCTP_BUS_OWNER_BRIDGE); - // 10b = 2 = static EID supported, matches currently assigned. - SET_ENDPOINT_ID_TYPE(resp->eid_type, 2); + resp->eid_type |= SET_ENDPOINT_TYPE(MCTP_BUS_OWNER_BRIDGE); + resp->eid_type |= + SET_ENDPOINT_ID_TYPE(MCTP_STATIC_EID_MATCHING_PRESENT); // TODO: medium specific information // Get Endpoint ID is typically send and reply using physical addressing. From 8b906b4369c1ba83dbd65447f9119e92fb9c73b0 Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Mon, 24 Mar 2025 02:58:13 +0000 Subject: [PATCH 2/2] mctpd: accept set endpoint ID as endpoint 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 --- src/mctp-control-spec.h | 24 ++++- src/mctpd.c | 164 +++++++++++++++++++++++++++++++++-- tests/test_mctpd_endpoint.py | 66 ++++++++++++++ 3 files changed, 246 insertions(+), 8 deletions(-) diff --git a/src/mctp-control-spec.h b/src/mctp-control-spec.h index 45f3b5b..e3f2f62 100644 --- a/src/mctp-control-spec.h +++ b/src/mctp-control-spec.h @@ -232,10 +232,23 @@ struct mctp_ctrl_resp_resolve_endpoint_id { #define MCTP_CTRL_CC_GET_MCTP_VER_SUPPORT_UNSUPPORTED_TYPE 0x80 -/* MCTP Set Endpoint ID response fields +/* MCTP Set Endpoint ID request and response fields * See DSP0236 v1.3.0 Table 14. */ +#define MCTP_SET_EID_OPERATION_SHIFT 0x0 +#define MCTP_SET_EID_OPERATION_MASK 0x3 +#define GET_MCTP_SET_EID_OPERATION(field) \ + (((field) >> MCTP_SET_EID_OPERATION_SHIFT) & \ + MCTP_SET_EID_OPERATION_MASK) +#define SET_MCTP_SET_EID_OPERATION(status) \ + (((status) & MCTP_SET_EID_OPERATION_MASK) \ + << MCTP_SET_EID_OPERATION_SHIFT) +#define MCTP_SET_EID_SET 0x0 +#define MCTP_SET_EID_FORCE 0x1 +#define MCTP_SET_EID_RESET 0x2 +#define MCTP_SET_EID_DISCOVERED 0x3 + #define MCTP_EID_ASSIGNMENT_STATUS_SHIFT 0x4 #define MCTP_EID_ASSIGNMENT_STATUS_MASK 0x3 #define SET_MCTP_EID_ASSIGNMENT_STATUS(status) \ @@ -244,6 +257,15 @@ struct mctp_ctrl_resp_resolve_endpoint_id { #define MCTP_SET_EID_ACCEPTED 0x0 #define MCTP_SET_EID_REJECTED 0x1 +#define MCTP_EID_ALLOCATION_STATUS_SHIFT 0x0 +#define MCTP_EID_ALLOCATION_STATUS_MASK 0x3 +#define SET_MCTP_EID_ALLOCATION_STATUS(status) \ + (((status) & MCTP_EID_ALLOCATION_STATUS_MASK) \ + << MCTP_EID_ALLOCATION_STATUS_SHIFT) +#define MCTP_SET_EID_POOL_NONE 0x0 +#define MCTP_SET_EID_POOL_REQUIRED 0x1 +#define MCTP_SET_EID_POOL_RECEIVED 0x2 + /* MCTP Physical Transport Binding identifiers * See DSP0239 v1.7.0 Table 3. */ diff --git a/src/mctpd.c b/src/mctpd.c index 7b1377c..bb5efbf 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -231,6 +231,12 @@ static int emit_interface_added(struct link *link); static int emit_interface_removed(struct link *link); static int emit_net_added(struct ctx *ctx, struct net *net); static int emit_net_removed(struct ctx *ctx, struct net *net); +static int add_peer(struct ctx *ctx, const dest_phys *dest, mctp_eid_t eid, + uint32_t net, struct peer **ret_peer); +static int add_peer_from_addr(struct ctx *ctx, + const struct sockaddr_mctp_ext *addr, + struct peer **ret_peer); +static int remove_peer(struct peer *peer); static int query_peer_properties(struct peer *peer); static int setup_added_peer(struct peer *peer); static void add_peer_route(struct peer *peer); @@ -631,7 +637,64 @@ static int reply_message(struct ctx *ctx, int sd, const void *resp, return 0; } -// Handles new Incoming Set Endpoint ID request +/// Clear interface local addresses and remote cached peers +static void clear_interface_addrs(struct ctx *ctx, int ifindex) +{ + mctp_eid_t *addrs; + size_t addrs_num; + size_t i; + int rc; + + // Remove all addresses on this interface + addrs = mctp_nl_addrs_byindex(ctx->nl, ifindex, &addrs_num); + if (addrs) { + for (i = 0; i < addrs_num; i++) { + rc = mctp_nl_addr_del(ctx->nl, addrs[i], ifindex); + if (rc < 0) { + errx(rc, + "ERR: cannot remove local eid %d ifindex %d", + addrs[i], ifindex); + } + } + free(addrs); + } + + // Remove all peers on this interface + for (i = 0; i < ctx->num_peers; i++) { + struct peer *p = ctx->peers[i]; + if (p->state == REMOTE && p->phys.ifindex == ifindex) { + remove_peer(p); + } + } +} + +/// Handles new Incoming Set Endpoint ID request +/// +/// This currently handles two cases: Top-most bus owner and Endpoint. No bridge +/// support yet. +/// +/// +/// # References +/// +/// The DSP0236 1.3.3 specification describes Set Endpoint ID in the following +/// sections: +/// +/// - 8.18 Endpoint ID assignment and endpoint ID pools +/// +/// > A non-bridge device that is connected to multiple different buses +/// > will have one EID for each bus it is attached to. +/// +/// - 9.1.3 EID options for MCTP bridge +/// +/// > There are three general options: +/// > - The bridge uses a single MCTP endpoint +/// > - The bridge uses an MCTP endpoint for each bus that connects to a bus owner +/// > - The bridge uses an MCTP endpoint for every bus to which it connects +/// +/// - 12.4 Set Endpoint ID +/// +/// [the whole section] +/// static int handle_control_set_endpoint_id(struct ctx *ctx, int sd, struct sockaddr_mctp_ext *addr, const uint8_t *buf, @@ -639,24 +702,97 @@ static int handle_control_set_endpoint_id(struct ctx *ctx, int sd, { struct mctp_ctrl_cmd_set_eid *req = NULL; struct mctp_ctrl_resp_set_eid respi = { 0 }, *resp = &respi; + struct link *link_data; + struct peer *peer; size_t resp_len; + int rc; if (buf_size < sizeof(*req)) { - warnx("short Set Endpoint ID message"); + bug_warn("short Set Endpoint ID message"); return -ENOMSG; } req = (void *)buf; + link_data = mctp_nl_get_link_userdata(ctx->nl, addr->smctp_ifindex); + if (!link_data) { + bug_warn("unconfigured interface %d", addr->smctp_ifindex); + return -ENOENT; + } + mctp_ctrl_msg_hdr_init_resp(&respi.ctrl_hdr, req->ctrl_hdr); resp->completion_code = MCTP_CTRL_CC_SUCCESS; - resp->status = 0x01 << 4; // Already assigned, TODO - resp->eid_set = local_addr(ctx, addr->smctp_ifindex); - resp->eid_pool_size = 0; resp_len = sizeof(struct mctp_ctrl_resp_set_eid); - // TODO: learn busowner route and neigh + // reject if we are bus owner + if (link_data->role == ENDPOINT_ROLE_BUS_OWNER) { + warnx("Rejected set EID %d request from (%s) because we are the bus owner", + req->eid, ext_addr_tostr(addr)); + resp->completion_code = MCTP_CTRL_CC_ERROR_UNSUPPORTED_CMD; + resp_len = sizeof(struct mctp_ctrl_resp); + return reply_message(ctx, sd, resp, resp_len, addr); + } - return reply_message(ctx, sd, resp, resp_len, addr); + // error if EID is invalid + if (req->eid < 0x08 || req->eid == 0xFF) { + warnx("Rejected invalid EID %d", req->eid); + resp->completion_code = MCTP_CTRL_CC_ERROR_INVALID_DATA; + resp_len = sizeof(struct mctp_ctrl_resp); + return reply_message(ctx, sd, resp, resp_len, addr); + } + + switch (GET_MCTP_SET_EID_OPERATION(req->operation)) { + case MCTP_SET_EID_SET: + // TODO: for bridges, only accept EIDs from originator bus + // + // We currently only support endpoints, which require separate EIDs on + // interfaces (see function comment). For bridges, we might need to support + // 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) + + // fallthrough + case MCTP_SET_EID_FORCE: + + fprintf(stderr, "setting EID to %d\n", req->eid); + + // 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); + + rc = mctp_nl_addr_add(ctx->nl, req->eid, addr->smctp_ifindex); + if (rc < 0) { + warnx("ERR: cannot add local eid %d to ifindex %d", + req->eid, addr->smctp_ifindex); + resp->completion_code = MCTP_CTRL_CC_ERROR_NOT_READY; + } + + rc = add_peer_from_addr(ctx, addr, &peer); + if (rc == 0) { + rc = setup_added_peer(peer); + } + if (rc < 0) { + warnx("ERR: cannot add bus owner to object lists"); + } + + resp->status = + SET_MCTP_EID_ASSIGNMENT_STATUS(MCTP_SET_EID_ACCEPTED) | + SET_MCTP_EID_ALLOCATION_STATUS(MCTP_SET_EID_POOL_NONE); + resp->eid_set = req->eid; + resp->eid_pool_size = 0; + fprintf(stderr, "Accepted set eid %d\n", req->eid); + return reply_message(ctx, sd, resp, resp_len, addr); + + case MCTP_SET_EID_DISCOVERED: + case MCTP_SET_EID_RESET: + // unsupported + resp->completion_code = MCTP_CTRL_CC_ERROR_INVALID_DATA; + return reply_message(ctx, sd, resp, resp_len, addr); + + default: + bug_warn("unreachable Set EID operation code"); + return -EINVAL; + } } static int @@ -1478,6 +1614,20 @@ static int add_peer(struct ctx *ctx, const dest_phys *dest, mctp_eid_t eid, return 0; } +static int add_peer_from_addr(struct ctx *ctx, + const struct sockaddr_mctp_ext *addr, + struct peer **ret_peer) +{ + struct dest_phys phys; + + phys.ifindex = addr->smctp_ifindex; + memcpy(phys.hwaddr, addr->smctp_haddr, addr->smctp_halen); + phys.hwaddr_len = addr->smctp_halen; + + return add_peer(ctx, &phys, addr->smctp_base.smctp_addr.s_addr, + addr->smctp_base.smctp_network, ret_peer); +} + static int check_peer_struct(const struct peer *peer, const struct net *n) { if (n->net != peer->net) { diff --git a/tests/test_mctpd_endpoint.py b/tests/test_mctpd_endpoint.py index 2ce97b9..5c2b125 100644 --- a/tests/test_mctpd_endpoint.py +++ b/tests/test_mctpd_endpoint.py @@ -1,7 +1,14 @@ import pytest +import asyncdbus from mctp_test_utils import * from mctpenv import * +"""Simple endpoint setup. + +Contains one interface (lladdr 0x1d), and one bus-owner (lladdr 0x1d, eid 8), +that reports support for MCTP control and PLDM. +""" + @pytest.fixture def config(): return """ @@ -35,3 +42,62 @@ async def test_respond_get_eid_with_no_eid(dbus, mctpd): cmd = MCTPControlCommand(True, 0, 0x02) rsp = await bo.send_control(mctpd.network.mctp_socket, cmd) assert rsp.hex(' ') == '00 02 00 00 02 00' + + +""" Test if mctpd accepts Set EID when no EID """ +async def test_accept_set_eid(dbus, mctpd): + bo = mctpd.network.endpoints[0] + + assert len(mctpd.system.addresses) == 0 + + # no EID yet + rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x02)) + 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]))) + assert rsp.hex(' ') == '00 01 00 00 42 00' + + # get EID, expect receive 42 back + rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x02)) + assert rsp.hex(' ') == '00 02 00 42 02 00' + + +async def test_accept_multiple_set_eids_for_single_interface(dbus, mctpd): + bo = mctpd.network.endpoints[0] + + assert len(mctpd.system.addresses) == 0 + + # if we are only reachable through one interfaces, + # accept all Set EIDs + assert len(mctpd.system.interfaces) == 1 + + # no EID yet + rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x02)) + assert rsp.hex(' ') == '00 02 00 00 02 00' + + # set EID = 42 + first_eid = 42 + rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x01, bytes([0x00, first_eid]))) + assert rsp.hex(' ') == f'00 01 00 00 {first_eid:02x} 00' + + # get EID, expect receive 42 back + rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x02)) + assert rsp.hex(' ') == f'00 02 00 {first_eid:02x} 02 00' + + # set EID = 66 + second_eid = 66 + rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x01, bytes([0x00, second_eid]))) + assert rsp.hex(' ') == f'00 01 00 00 {second_eid:02x} 00' + + # get EID, expect receive 66 back + rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x02)) + assert rsp.hex(' ') == f'00 02 00 {second_eid:02x} 02 00' + + # expect previous EID removed on D-Bus + with pytest.raises(asyncdbus.errors.DBusError) as ex: + await mctpd_mctp_endpoint_control_obj(dbus, f"/au/com/codeconstruct/mctp1/networks/1/endpoints/{first_eid}") + assert str(ex.value) == f"Unknown object '/au/com/codeconstruct/mctp1/networks/1/endpoints/{first_eid}'." + + # expect new EID on D-Bus + assert await mctpd_mctp_endpoint_control_obj(dbus, f"/au/com/codeconstruct/mctp1/networks/1/endpoints/{second_eid}")