Skip to content

mctpd: Send Discovery Notify on Endpoint role set #15

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 1 commit into
base: main
Choose a base branch
from
+231 −14
Open

mctpd: Send Discovery Notify on Endpoint role set #15

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
148 changes: 136 additions & 12 deletions src/mctpd.c
View file
Original file line number Diff line number Diff line change
@@ -126,7 +126,6 @@ enum discovery_state {
};

struct link {
enum discovery_state discovered;
bool published;
int ifindex;
enum endpoint_role role;
@@ -135,6 +134,14 @@ struct link {
sd_bus_slot *slot_iface;
sd_bus_slot *slot_busowner;

struct {
enum discovery_state flag;
Copy link
Member

@jk-ozlabs jk-ozlabs Dec 3, 2025

Choose a reason for hiding this comment

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

Super minor, but 'flag' implies a boolean, and we're not quite aligned with the "discovered flag" concept in DSP0236. How about just state for this?

Copy link
Contributor Author

@khangng-ampere khangng-ampere Dec 3, 2025

Choose a reason for hiding this comment

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

Acknowledged

sd_event_source *notify_source;
dest_phys notify_dest;
uint64_t notify_retry_delay;
uint8_t notify_tries_left;
} discovery;

struct ctx *ctx;
};

@@ -805,8 +812,8 @@ static int handle_control_set_endpoint_id(struct ctx *ctx, int sd,
warnx("ERR: cannot add bus owner to object lists");
}

if (link_data->discovered != DISCOVERY_UNSUPPORTED) {
link_data->discovered = DISCOVERY_DISCOVERED;
if (link_data->discovery.flag != DISCOVERY_UNSUPPORTED) {
link_data->discovery.flag = DISCOVERY_DISCOVERED;
}
resp->status =
SET_MCTP_EID_ASSIGNMENT_STATUS(MCTP_SET_EID_ACCEPTED) |
@@ -817,13 +824,13 @@ static int handle_control_set_endpoint_id(struct ctx *ctx, int sd,
return reply_message(ctx, sd, resp, resp_len, addr);

case MCTP_SET_EID_DISCOVERED:
if (link_data->discovered == DISCOVERY_UNSUPPORTED) {
if (link_data->discovery.flag == DISCOVERY_UNSUPPORTED) {
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);
}

link_data->discovered = DISCOVERY_DISCOVERED;
link_data->discovery.flag = DISCOVERY_DISCOVERED;
resp->status =
SET_MCTP_EID_ASSIGNMENT_STATUS(MCTP_SET_EID_REJECTED) |
SET_MCTP_EID_ALLOCATION_STATUS(MCTP_SET_EID_POOL_NONE);
@@ -1061,16 +1068,16 @@ static int handle_control_prepare_endpoint_discovery(
resp = (void *)resp;
mctp_ctrl_msg_hdr_init_resp(&resp->ctrl_hdr, *req);

if (link_data->discovered == DISCOVERY_UNSUPPORTED) {
if (link_data->discovery.flag == DISCOVERY_UNSUPPORTED) {
warnx("received prepare for discovery request to unsupported interface %d",
addr->smctp_ifindex);
resp->completion_code = MCTP_CTRL_CC_ERROR_UNSUPPORTED_CMD;
return reply_message_phys(ctx, sd, resp,
sizeof(struct mctp_ctrl_resp), addr);
}

if (link_data->discovered == DISCOVERY_DISCOVERED) {
link_data->discovered = DISCOVERY_UNDISCOVERED;
if (link_data->discovery.flag == DISCOVERY_DISCOVERED) {
link_data->discovery.flag = DISCOVERY_UNDISCOVERED;
warnx("clear discovered flag of interface %d",
addr->smctp_ifindex);
}
@@ -1105,13 +1112,13 @@ handle_control_endpoint_discovery(struct ctx *ctx, int sd,
return 0;
}

if (link_data->discovered == DISCOVERY_UNSUPPORTED) {
if (link_data->discovery.flag == DISCOVERY_UNSUPPORTED) {
resp->completion_code = MCTP_CTRL_CC_ERROR_INVALID_DATA;
return reply_message(ctx, sd, resp,
sizeof(struct mctp_ctrl_resp), addr);
}

if (link_data->discovered == DISCOVERY_DISCOVERED) {
if (link_data->discovery.flag == DISCOVERY_DISCOVERED) {
// if we are already discovered (i.e, assigned an EID), then no reply
return 0;
}
@@ -3659,6 +3666,88 @@ static int bus_link_get_prop(sd_bus *bus, const char *path,
return rc;
}

static int query_discovery_notify(struct link *link)
{
struct mctp_ctrl_cmd_discovery_notify req = { 0 };
struct mctp_ctrl_resp_discovery_notify *resp;
struct sockaddr_mctp_ext resp_addr;
size_t buf_size;
uint8_t *buf;
int rc;

mctp_ctrl_msg_hdr_init_req(&req.ctrl_hdr, mctp_next_iid(link->ctx),
MCTP_CTRL_CMD_DISCOVERY_NOTIFY);

rc = endpoint_query_phys(link->ctx, &link->discovery.notify_dest,
MCTP_CTRL_HDR_MSG_TYPE, &req, sizeof(req),
&buf, &buf_size, &resp_addr);
if (rc < 0)
goto free_buf;

if (buf_size != sizeof(*resp)) {
warnx("%s: wrong reply length %zu bytes. dest %s", __func__,
buf_size, dest_phys_tostr(&link->discovery.notify_dest));
rc = -ENOMSG;
goto free_buf;
}

resp = (void *)buf;
if (resp->completion_code != 0) {
warnx("Failure completion code 0x%02x from %s",
resp->completion_code,
dest_phys_tostr(&link->discovery.notify_dest));
rc = -ECONNREFUSED;
goto free_buf;
}

free_buf:
free(buf);
return rc;
}

static int link_discovery_notify_callback(sd_event_source *source,
uint64_t time, void *userdata)
{
struct link *link = userdata;
struct ctx *ctx = link->ctx;
int rc;

// sanity check
assert(link->discovery.notify_source == source);

// Discovery notify succeeded
if (link->discovery.flag == DISCOVERY_DISCOVERED)
goto disarm;

rc = query_discovery_notify(link);
if (rc < 0) {
if (ctx->verbose) {
warnx("failed to send discovery notify at retry %d: %s",
link->discovery.notify_tries_left, strerror(-rc));
}
}

link->discovery.notify_tries_left -= 1;
if (link->discovery.notify_tries_left == 0) {
warnx("failed to send discovery notify after all retries");
goto disarm;
}

rc = mctp_ops.sd_event.source_set_time_relative(
source, link->discovery.notify_retry_delay);
if (rc < 0) {
warnx("failed to rearm discovery notify timer");
goto disarm;
}

return 0;

disarm:
sd_event_source_disable_unref(source);
link->discovery.notify_source = NULL;
return 0;
}

static int bus_link_set_prop(sd_bus *bus, const char *path,
const char *interface, const char *property,
sd_bus_message *value, void *userdata,
@@ -4496,7 +4585,7 @@ static int add_interface(struct ctx *ctx, int ifindex)
if (!link)
return -ENOMEM;

link->discovered = DISCOVERY_UNSUPPORTED;
link->discovery.flag = DISCOVERY_UNSUPPORTED;
link->published = false;
link->ifindex = ifindex;
link->ctx = ctx;
@@ -4526,7 +4615,42 @@ static int add_interface(struct ctx *ctx, int ifindex)
}

if (phys_binding == MCTP_PHYS_BINDING_PCIE_VDM) {
link->discovered = DISCOVERY_UNDISCOVERED;
link->discovery.flag = DISCOVERY_UNDISCOVERED;
Copy link
Member

@jk-ozlabs jk-ozlabs Dec 3, 2025

Choose a reason for hiding this comment

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

We want the discovery process (below) to work for any binding type that supports Discovery Notify, not just VDM, right?

Should we move that out to be conditional on link->discovery.flag == DISCOVERY_UNDISCOVERED ?

(there may be some binding-specific addressing info needed, but the rest should be common?)

Copy link
Contributor Author

@khangng-ampere khangng-ampere Dec 3, 2025

Choose a reason for hiding this comment

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

I see, I will split in to two blocks: one for setting the flag to DISCOVERY_UNDISCOVERED, and one for conditional on the flag to enable the discovery process

// TODO: These numbers are respectively MN1 and MT4, specified in DSP0239
// control message timing.
//
// Might need to extract these to macros like MCTP_I2C_TSYM_* in this file,
// or a commit to actually centralize those timing at one place, now that
// we have support for detecting link binding type.
link->discovery.notify_tries_left = 3;
link->discovery.notify_retry_delay = 5000000;

// For PCIe-VDM, we want an all zeroes address for Route-to-Root-Complex.
Copy link
Member

@jk-ozlabs jk-ozlabs Dec 3, 2025

Choose a reason for hiding this comment

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

What is defining these address semantics?

Given there is no convention upstream for the format of VDM addresses, how do we know this is correct?

Copy link
Contributor Author

@khangng-ampere khangng-ampere Dec 3, 2025

Choose a reason for hiding this comment

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

There is indeed no VDM supports yet in upstream, and the addresses format has only been discussed in email threads and non-upstreamed drivers...

Do you have any suggestions to resolve or move this forward? It would be great if this could be resolved independently from the kernel changes.

Would changing this into using IFLA_BROADCAST for broadcast hardware address (and only support discovery for MCTP bindings that 1) requires discovery in the spec and 2) support this attribute in the binding driver) be more sensible? IFLA_BROADCAST seems to be the most correct place to get this info from.

Copy link
Contributor Author

@khangng-ampere khangng-ampere Dec 3, 2025

Choose a reason for hiding this comment

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

Oh wait, I got it wrong - Route-to-root-complex is not broadcast (that is Broadcast-from-Root-Complex), but rather default / "gateway"-ish address... Not sure if there is a Linux netlink attribute for that...

Copy link
Member

@jk-ozlabs jk-ozlabs Dec 3, 2025

Choose a reason for hiding this comment

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

There is indeed no VDM supports yet in upstream, and the addresses format has only been discussed in email threads and non-upstreamed drivers...

My concern is that we end up committing to an addressing representation that has not been reviewed. I am aware of some prior discussions there, but nothing that seemed like a definite plan at this stage.

Ideally, we'd at least have some indication of an addressing format upstream (or at least proposed upstream), so that we have some confidence that this is correct.

Would changing this into using IFLA_BROADCAST

That only covers the Broadcast From RC addressing type though? We're after Route to RC here, no?

rc = mctp_nl_hwaddr_len_byindex(
ctx->nl, ifindex,
&link->discovery.notify_dest.hwaddr_len);
if (rc < 0) {
warnx("Can't find hwaddr_len by index %d", ifindex);
return -ENOENT;
}

memset(link->discovery.notify_dest.hwaddr, 0,
link->discovery.notify_dest.hwaddr_len);
link->discovery.notify_dest.ifindex = ifindex;

rc = mctp_ops.sd_event.add_time_relative(
ctx->event, &link->discovery.notify_source,
CLOCK_MONOTONIC, 0, 0, link_discovery_notify_callback,
link);
if (rc >= 0) {
rc = sd_event_source_set_enabled(
link->discovery.notify_source, SD_EVENT_ON);
}
if (rc < 0) {
warnx("Failed to arm discovery notify timer");
sd_event_source_disable_unref(
link->discovery.notify_source);
}
}

link->published = true;
97 changes: 95 additions & 2 deletions tests/test_mctpd_endpoint.py
View file
Original file line number Diff line number Diff line change
@@ -22,11 +22,16 @@ async def iface():


@pytest.fixture
async def sysnet(iface):
async def bo(iface):
return Endpoint(iface, bytes([0x10]), eid=8)


@pytest.fixture
async def sysnet(iface, bo):
system = System()
await system.add_interface(iface)
network = Network()
network.add_endpoint(Endpoint(iface, bytes([0x10]), eid=8))
network.add_endpoint(bo)
return Sysnet(system, network)


@@ -113,12 +118,100 @@ class TestDiscovery:
async def iface(self):
return System.Interface("mctp0", 1, 1, bytes([0x1D]), 68, 254, True, PhysicalBinding.PCIE_VDM)

@pytest.fixture
async def bo(self, iface):
return TestDiscovery.BusOwnerEndpoint(iface, bytes([0x00]), eid=8)


class BusOwnerEndpoint(Endpoint):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.sem = trio.Semaphore(initial_value=0)

async def handle_mctp_control(self, sock, addr, data):
print(addr, data)
flags, opcode = data[0:2]
if opcode != 0x0D:
return await super().handle_mctp_control(sock, addr, data)
dst_addr = MCTPSockAddr.for_ep_resp(self, addr, sock.addr_ext)
await sock.send(dst_addr, bytes([flags & 0x1F, opcode, 0x00]))
self.sem.release()


""" Test simple Discovery sequence """
async def test_simple_discovery_sequence(self, dbus, mctpd):
bo = mctpd.network.endpoints[0]

assert len(mctpd.system.addresses) == 0

# BMC should send a Discovery Notify message
with trio.move_on_after(5) as expected:
await bo.sem.acquire()
assert not expected.cancelled_caught

# 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'

# BMC response to Prepare for Discovery
rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x0B))
assert rsp.hex(' ') == '00 0b 00'

# BMC response to Endpoint Discovery
rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x0C))
assert rsp.hex(' ') == '00 0c 00'

# set EID = 42
eid = 42
rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x01, bytes([0x00, eid])))
assert rsp.hex(' ') == f'00 01 00 00 {eid:02x} 00'

# BMC should contains two object paths: bus owner and itself
assert await mctpd_mctp_endpoint_control_obj(dbus, f"/au/com/codeconstruct/mctp1/networks/1/endpoints/{bo.eid}")
assert await mctpd_mctp_endpoint_control_obj(dbus, f"/au/com/codeconstruct/mctp1/networks/1/endpoints/{eid}")


class TestDiscoveryRetry:
@pytest.fixture
async def iface(self):
return System.Interface("mctp0", 1, 1, bytes([0x1D]), 68, 254, True, PhysicalBinding.PCIE_VDM)

@pytest.fixture
async def bo(self, iface):
return TestDiscoveryRetry.BusOwnerEndpoint(iface, bytes([0x00]), eid=8)


class BusOwnerEndpoint(Endpoint):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.sem = trio.Semaphore(initial_value=0)
self.retry_left = 1

async def handle_mctp_control(self, sock, src_addr, msg):
flags, opcode = msg[0:2]
if opcode != 0x0D:
return await super().handle_mctp_control(sock, src_addr, msg)

# only reply after 2 retries
if self.retry_left == 0:
dst_addr = MCTPSockAddr.for_ep_resp(self, src_addr, sock.addr_ext)
await sock.send(dst_addr, bytes([flags & 0x1F, opcode, 0x00]))
self.sem.release()
else:
self.retry_left -= 1


""" Test simple Discovery sequence """
async def test_discovery_after_one_retry(self, dbus, mctpd, autojump_clock):
bo = mctpd.network.endpoints[0]

assert len(mctpd.system.addresses) == 0

# BMC should send a Discovery Notify message
with trio.move_on_after(10) as expected:
await bo.sem.acquire()
assert not expected.cancelled_caught

# 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'