Skip to content

Mctp bridge support #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Mctp bridge support #71

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b274f0d
Add mctp ALLOCATE_ENDPOINT_ID message structure
faizana-nvidia Apr 22, 2025
3f9ec21
Add Bridge Endpoint Support
faizana-nvidia Apr 23, 2025
686395a
Add Allocate Endpoint ID support for MCTP Bridge
faizana-nvidia Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 169 additions & 0 deletions src/mctpd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
#define CC_MCTP_DBUS_IFACE_INTERFACE "au.com.codeconstruct.MCTP.Interface1"
#define CC_MCTP_DBUS_NETWORK_INTERFACE "au.com.codeconstruct.MCTP.Network1"

#define BRIDGE_SETTLE_DELAY_SEC 4
// an arbitrary constant for use with sd_id128_get_machine_app_specific()
static const char* mctpd_appid = "67369c05-4b97-4b7e-be72-65cfd8639f10";

Expand Down Expand Up @@ -243,6 +244,7 @@ static int add_local_eid(ctx *ctx, int net, int eid);
static int del_local_eid(ctx *ctx, int net, int eid);
static int add_net(ctx *ctx, int net);
static int add_interface(ctx *ctx, int ifindex);
static int endpoint_allocate_eid(peer *peer);

mctp_eid_t local_addr(const ctx *ctx, int ifindex) {
mctp_eid_t *eids, ret = 0;
Expand Down Expand Up @@ -2107,6 +2109,16 @@ static int method_assign_endpoint(sd_bus_message *call, void *data, sd_bus_error
goto err;
dfree(peer_path);

/* MCTP Bridge Support */
/* Allocate Endopint EID while looking for pool start eid
* Update pool endpoints to dbus */

if(peer->pool_size > 0) {
/* new start eid will be assigned before MCTP Allocate eid control command */
peer->pool_start = peer->eid + 1;
rc = endpoint_allocate_eid(peer);
}

return sd_bus_reply_method_return(call, "yisb",
peer->eid, peer->net, peer_path, 1);
err:
Expand Down Expand Up @@ -2185,8 +2197,11 @@ static int method_assign_endpoint_static(sd_bus_message *call, void *data,
dfree(peer_path);

/* MCTP Bridge Support */
/* Allocate Endopint EID
* Update pool endpoints to dbus */
if(peer->pool_size > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: formatting here should be:

    if (peer->pool_size > 0) {

(with the space after the if)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, since we are on this, what clang-format are we following here? Any steps on how I could run it locally. Would come in handy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a full .clang-format definition, but we're roughly doing kernel format here. If it helps, I can put an actual config together, but that then we'd need a lot of churn in applying it.

So, that's probably better something done just before release, to avoid to much merge collisions.

Or, if you're OK with a bit of churn, I can do that beforehand?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sold for before release part :)

peer->pool_start = start_eid;
rc = endpoint_allocate_eid(peer);
}

return sd_bus_reply_method_return(call, "yisb",
Expand Down Expand Up @@ -4494,6 +4509,160 @@ static void setup_config_defaults(ctx *ctx)
ctx->default_role = ENDPOINT_ROLE_BUS_OWNER;
}

static int endpoint_send_allocate_endpoint_id(peer *peer,
mctp_eid_t eid_start, uint8_t eid_pool_size, mctp_ctrl_cmd_alloc_eid_op oper,
uint8_t *allocated_pool_size, mctp_eid_t *allocated_pool_start)
{
struct sockaddr_mctp_ext addr;
struct mctp_ctrl_cmd_alloc_eid req = {0};
struct mctp_ctrl_resp_alloc_eid *resp = NULL;
uint8_t *buf = NULL;
size_t buf_size;
uint8_t iid, stat;
int rc;

iid = mctp_next_iid(peer->ctx);
req.ctrl_hdr.rq_dgram_inst = RQDI_REQ | iid;
req.ctrl_hdr.command_code = MCTP_CTRL_CMD_ALLOCATE_ENDPOINT_IDS;
req.alloc_eid_op = alloc_eid;
req.pool_size = eid_pool_size;
req.start_eid = eid_start;
rc = endpoint_query_peer(peer, MCTP_CTRL_HDR_MSG_TYPE, &req, sizeof(req),
&buf, &buf_size, &addr);
if (rc < 0)
goto out;

rc = mctp_ctrl_validate_response(buf, buf_size, sizeof(*resp),
peer_tostr_short(peer), iid,
MCTP_CTRL_CMD_ALLOCATE_ENDPOINT_IDS);

if (rc)
goto out;

resp = (void *)buf;
if (!resp) {
warnx("%s Invalid response Buffer\n", __func__);
return -ENOMEM;
}

stat = resp->status & 0x03;
if (stat == 0x00) {
if(peer->ctx->verbose) {
fprintf(stderr, "%s Allocation Accepted \n", __func__);
}
}
else if (stat == 0x1) {
warnx("%s Allocation was rejected as it was Allocated by other bus \n", __func__);
}

*allocated_pool_size = resp->eid_pool_size;
*allocated_pool_start = resp->eid_set;
if(peer->ctx->verbose) {
fprintf(stderr, "%s Allocated size of %d, starting from EID %d\n", __func__,
resp->eid_pool_size, resp->eid_set);
}

return 0;
out:
free(buf);
return rc;
}
static int cb_populate_pool_eids(sd_event_source *s, uint64_t t, void* data)
{
peer *peer = data;
int net = peer->net;
struct peer *allocated_peer = NULL;
dest_phys dest = peer->phys;
int rc;

fprintf (stderr, "Bridge Time expired, set pool eids\n");
for (mctp_eid_t eid = peer->pool_start;
eid < peer->pool_start + peer->pool_size; eid++) {
rc = add_peer(peer->ctx, &dest, eid, net, &allocated_peer);
if (rc < 0) {
warnx("%s failed to find peer for allocated eid %d\n",
__func__, eid);
continue;
}

rc = setup_added_peer(allocated_peer);
if (rc < 0) {
warnx("%s failed to setup peer for allocated eid %d\n",
__func__, eid);
continue;
}
}

return 0;
}

static mctp_eid_t get_pool_start (peer *peer, mctp_eid_t eid_start, uint8_t pool_size)
{
uint8_t count = 0;
mctp_eid_t pool_start = eid_alloc_max;
net_det *n = lookup_net(peer->ctx, peer->net);

if (!n) {
warnx("BUG: Unknown net %d : failed to get pool start\n", peer->net);
return eid_alloc_max;
}

for (mctp_eid_t e = eid_start; e <= eid_alloc_max; e++) {
if (n->peeridx[e] == -1) {
if(pool_start == eid_alloc_max) {
pool_start = e;
}
count++;
if (count == pool_size) return pool_start;
} else {
pool_start = eid_alloc_max;
count = 0;
}
}

return eid_alloc_max;
}

static int endpoint_allocate_eid(peer* peer)
{
uint8_t allocated_pool_size = 0;
mctp_eid_t allocated_pool_start = 0;

/* Find pool sized contiguous unused eids to allocate on the bridge. */
peer->pool_start = get_pool_start(peer, peer->pool_start, peer->pool_size);
if(peer->pool_start == eid_alloc_max) {
warnx("%s failed to find contiguous EIDs of required size", __func__);
return 0;
} else {
if(peer->ctx->verbose)
fprintf(stderr, "%s Asking for contiguous EIDs for pool with start eid : %d\n", __func__, peer->pool_start);
}

int rc = endpoint_send_allocate_endpoint_id(peer, peer->pool_start, peer->pool_size,
alloc_eid, &allocated_pool_size, &allocated_pool_start);
if (rc) {
warnx("%s failed to allocate endpoints, returned %s %d\n",
__func__, strerror(-rc), rc);
} else {
peer->pool_size = allocated_pool_size;
peer->pool_start = allocated_pool_start;

//delay for settling MCTP Bridge after allocation of downstream eid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that we issue the Allocate Endpoint IDs immediately. If the bridge needs some time to settle, we can implement that with a retry.

For your device, what do you see when you attempt to do the Allocate immediately? Does the command fail, or something else?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me highlight the issue better, it's not that Allocate Endpoint ID is getting delayed (we are sending it as part of sequence right after bridge eid is assigned), but post allocate endpoint id, referring to my setup, I see no response from the downstream EIDs, basically timeout for GET_MESSG_TYPE and GET_UUID mctp commands on downstream eids.

On topic of retry mechanism, in my setup based on observational data, find that it takes around 4 sec for all eids to be in proper state after receiving ALLOCATE_EID but this also depends on the bus topology and how many downstream endpoints are there.

That being said it could easily be something which could become a common issue for Bridge's owing to their network congestion, resource congestion etc.
A retry mechanism would then be needed as general part of design for messages like GET_MESSG_TYPE and GET_UUID, which is currently missing implementation.

Would like to explore any other suggestion beyond retry mechanism, if possible :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me highlight the issue better, it's not that Allocate Endpoint ID is getting delayed (we are sending it as part of sequence right after bridge eid is assigned), but post allocate endpoint id, referring to my setup, I see no response from the downstream EIDs, basically timeout for GET_MESSG_TYPE and GET_UUID mctp commands on downstream eids.

ah, gotchya!

Yeah, this isn't a trivial problem, and I've been thinking of potential approaches for a while.

My current leaning is that once an endpoint pool is assigned, mctpd would perform some sort of polling to determine which of the pool's EIDs are active (ie, have been assigned to a device, and that device is responding).

We could potentially do this via a Get Routing Table Entries command to the bridge, but that's a bit of a mis-use of the command, and isn't necessarily returning the data we want (the bridge may have routing information to a device, but the device may not be present).

So, I think the most reliable approach is that the pool's EIDs are periodically polled using a Get Endpoint ID command - which is the spec-recommended method of detecting endpoint presence, and is what we use for the existing recovery process. We may be able to share some code with the recover infrastructure for this too.

We would schedule those polls when the pool is allocated, and then repeat until we detect a device - at which point we would perform full enumeration. If no device is ever present, mctpd would end up polling forever, but I don't think there's an alternative there.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about doing a GetRoutingTableEntries, the bridge should return ERROR_NOT_READY if it is still assigning EIDs downstream from the previous AllocateEndpointID. Once we get a successful response, we can start querying EIDs downstream for UUIDs and message types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bridge should return ERROR_NOT_READY if it is still assigning EIDs downstream from the previous AllocateEndpointID

that's not necessarily what the bridge will do though - there's no concept of the routing state being "complete" at any point (eg, for a downstream port that may have an enpoint hotplugged at some point in the future (or perhaps never!))

For that reason, I don't think we'll be able to avoid polling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ending up utilizing the eids from the pool

Not sure I understand what you mean by "utilizing the eids" here, could you elaborate?

Take RoutingTable approach makes us definitive about what eids needs to be consumed from the pool

All allocated EIDs will need to be consumed, we have no choice about that.

It's what endpoints we try to interact with that may vary, depending on the approach.

My concern with using Get Routing Table Entries is that we're on the edges of what the spec describes this command for, whereas Get Endpoint ID is mentioned as a mechanism for this. An endpoint being described in Get Routing Table Entries does not mean it is available, so we would have to fall back to querying/polling it with Get Endpoint ID anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand what you mean by "utilizing the eids" here, could you elaborate?

Sorry I should have explained better. May be I'm overthinking it here but here's what I meant

For instance, if we have a network comprising of multiple Bridge's where we have to assign all Dynamic Eids to the downstream, as per our current assumption, we'll have to keep aside all Eids which bridge needs based on its pool size (Bridge eid : 8, pool size :15 thus 9- 23 is consumed from our Dynamic Pool even though there's possibility of not having devices downstream for 9-23 Eids, since we carve eids from our maintained pool via (code below) and then add_peer(consumed here) later update setup_peer()

/* Find an unused EID */ for (e = eid_alloc_min; e <= eid_alloc_max; e++) { if (n->peeridx[e] == -1) { rc = add_peer(ctx, dest, e, net, &peer); if (rc < 0) return rc; break; } }
this could end up consuming EIDs for the device which may not be there.

Also to mention based on the allocation of EIDs needs to be contiguous, this would create another issue if there happens to be other Bridges which has been allocated EID statically which kind of makes find a contiguous set of EIDs for the downstream endpoints of Dynamic Allocated Bridge difficult and where need of next contiguous set of EIDs rises.

Spec : DSP0236 Version: 1.3.0

9.1.9 Consolidating routing table entries
MCTP requires that when an EID pool is allocated to a device, the range of EIDs is contiguous and
follows the EID for the bridge itself. Thus, a bridge can elect to consolidate routing table information into
one entry when it recognizes that it has received an EID or EID range that is contiguous with an existing
entry for the same physical address and bus. (The reason that EID allocation and routing information
updates are not done as one range using the same command is because of the possibility that a device
may have already received an allocation from a different bus owner.)

In such contingency with limited EIDs availability, keeping EIDs for non available device is what I fear. Though I agree with your point too Routing Table may not be the best approach to detect Endpoint presence in the network especially if the endpoint is hot pluggable.

My concern with using Get Routing Table Entries is that we're on the edges of what the spec describes this command for, whereas Get Endpoint ID is mentioned as a mechanism for this. An endpoint being described in Get Routing Table Entries does not mean it is available, so we would have to fall back to querying/polling it with Get Endpoint ID anyway.

But then again in such case Spec tells us about commands like Discovery Notify which should be initiated by the Bridge to notify any change in current network topology under the bridge. Please feel free to correct me in my assumptions :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll have to keep aside all Eids which bridge needs based on its pool size (Bridge eid : 8, pool size :15 thus 9- 23 is consumed from our Dynamic Pool even though there's possibility of not having devices downstream for 9-23 Eids
[...]
this could end up consuming EIDs for the device which may not be there.

Yes, absolutely. If we have done an Allocate Endpoint IDs that gives a range of EIDs to a bridge, then mctpd no longer has ownership of any of those EIDs, and cannot hand out any subset of that range elsewhere.

In such contingency with limited EIDs availability, keeping EIDs for non available device is what I fear.

We have no choice about that - if we allocate a range to a bridge, the entire range belongs to the bridge, regardless of whether the EIDs in that range are then allocated to downstream devices, or whether those downstream devices are active.

But then again in such case Spec tells us about commands like Discovery Notify which should be initiated by the Bridge to notify any change in current network topology under the bridge.

That's not my interpretation of the spec for Discovery Notify; this is more to announce endpoint presence, and is only used on specific transport types (ie, those that do not have their own presence detection mechanism).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understood, Thank you so much for clearing this out, I'm gonna integrate the polling logic via GET_ENDPOINT_ID similar to recovery.

Since we dont have kernel implementation for using the bridge as a gateway for that range can we go with sub-optimal way of thing ? i.e adding routes and neighbours to these discovered device via Bridge phys address till the time your changes are getting ready?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we don't add the workaround at present. I'll take a look at getting the kernel changes done to support the proper gateway and range routes.

uint64_t now_usec, timer_usec;
// Get the current time in microseconds
if (sd_event_now(peer->ctx->event, CLOCK_MONOTONIC, &now_usec) < 0) {
warnx("Failed to get current time");
return -1;
}
timer_usec = now_usec + BRIDGE_SETTLE_DELAY_SEC * 1000000ULL;
rc = sd_event_add_time(peer->ctx->event, NULL,
CLOCK_MONOTONIC, timer_usec, 0,
cb_populate_pool_eids, peer);
}

return 0;
}

int main(int argc, char **argv)
{
int rc;
Expand Down
22 changes: 17 additions & 5 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,12 @@ async def handle_mctp_control(self, sock, addr, data):
data = bytes(hdr + [0x00, len(types)] + types)
await sock.send(raddr, data)

elif opcode == 8:
# Allocate Enpoint IDs
(_, pool_st) = data[3:]
data = bytes(hdr + [0x00, 0x00, self.pool_size, pool_st])
await sock.send(raddr, data)

else:
await sock.send(raddr, bytes(hdr + [0x05])) # unsupported command

Expand Down Expand Up @@ -348,9 +354,9 @@ def __init__(self):
def add_endpoint(self, endpoint):
self.endpoints.append(endpoint)

def lookup_endpoint(self, iface, lladdr):
def lookup_endpoint(self, iface, lladdr, eid):
for ep in self.endpoints:
if ep.iface == iface and ep.lladdr == lladdr:
if ep.iface == iface and ep.lladdr == lladdr and (eid == ep.eid or not eid):
return ep
return None

Expand Down Expand Up @@ -570,7 +576,7 @@ async def handle_send(self, addr, data):
if phys is None:
return

ep = self.network.lookup_endpoint(*phys)
ep = self.network.lookup_endpoint(*phys, a.eid)
if ep is None:
return

Expand Down Expand Up @@ -1031,8 +1037,8 @@ def name_owner_changed(name, new_owner, old_owner):

"""Simple system & network.

Contains one interface (lladdr 0x10, local EID 8), and one endpoint (lladdr
0x1d), that reports support for MCTP control and PLDM.
Contains two interface (lladdr 0x10, local EID 8), (lladdr 0x11, local EID 10) with one endpoint (lladdr
0x1d), and MCTP bridge (lladdr 0x1e, pool size 2), that reports support for MCTP control and PLDM.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't alter the default sysnet unless it's needed by a significant number of tests (which in this case, it is not). Just set up the test fixtures default as needed for your new test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so better to update the current interface (lladdr 0x10, local EID 8) with pool size and update pool size numbered eids to the network simulating a bridge rather than creating a new one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Just update the interface/endpoints/etc in the individual test case.

"""
@pytest.fixture
async def sysnet():
Expand All @@ -1044,6 +1050,12 @@ async def sysnet():
network = Network()
network.add_endpoint(Endpoint(iface, bytes([0x1d]), types = [0, 1]))

# new interface for MCTP Bridge device
bridge_iface = System.Interface('mctp1', 2, 2, bytes([0x11]), 68, 254, True)
await system.add_interface(bridge_iface)
await system.add_address(System.Address(bridge_iface, 10))
network.add_endpoint(Endpoint(bridge_iface, bytes([0x1e]), types=[0, 1], pool_size= 0x2))

return Sysnet(system, network)

@pytest.fixture
Expand Down
49 changes: 48 additions & 1 deletion tests/test_mctpd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ def ep_removed(ep_path, interfaces):
# Delete the endpoint from the network so its recovery will fail after
# timeout. Note we delete at the split index as the network was already
# populated with the default endpoint
del mctpd.network.endpoints[split]
del mctpd.network.endpoints[split + 1]

# Begin recovery for the endpoint ...
ep_ep = await ep.get_interface(MCTPD_ENDPOINT_I)
Expand All @@ -555,3 +555,50 @@ def ep_removed(ep_path, interfaces):
with trio.move_on_after(2 * MCTPD_TRECLAIM) as expected:
await removed.acquire()
assert not expected.cancelled_caught

""" Test that we allocate Eids to MCTP Bridge Endpoints"""
async def test_endpoint_allocate_eid(dbus, mctpd):
bridge = mctpd.network.endpoints[1]
ep_types = [0, 1, 5]
bridge.types = ep_types
static_eid = 35
start_eid = 36

# mimicing MCTP Bridge by adding Bridg's Endpoints to same network and physcial address
for eid in range(start_eid, start_eid + bridge.pool_size + 1):
mctpd.network.add_endpoint(Endpoint(bridge.iface, bridge.lladdr, types = [0,1,2,3], eid= eid))

# first assign enpoint to MCTP Bridge and later involk allocate endpoint ids
# this will add Brige's enpoints routes and neigh and update dbus
mctp = await mctpd_mctp_iface_obj(dbus, bridge.iface)
(eid, _, path, _) = await mctp.call_assign_endpoint_static(
bridge.lladdr,
static_eid,
start_eid
)

# non blocking sleep for Allocate Eid timer expiry
await trio.sleep(5)
assert eid == static_eid
# check if networks neighbours are reflecting the mctpd added bridge's neighbours
assert len(mctpd.system.neighbours) == (1 + bridge.pool_size)

neigh = mctpd.system.neighbours[0]
assert neigh.lladdr == bridge.lladdr
assert neigh.eid == static_eid

neigh = mctpd.system.neighbours[1]
assert neigh.lladdr == bridge.lladdr
assert neigh.eid == start_eid

neigh = mctpd.system.neighbours[2]
assert neigh.lladdr == bridge.lladdr
assert neigh.eid == start_eid + 1

# check one of Bridge's endpoint types per dbus property
path = path.rsplit('/endpoints', 1)[0]
path = path + f"/endpoints/{mctpd.network.endpoints[2].eid}"
ep = await mctpd_mctp_endpoint_common_obj(dbus, path)
query_types = list(await ep.get_supported_message_types())
print(mctpd.network.endpoints[2].types)
assert (query_types == mctpd.network.endpoints[2].types)