Skip to content
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

mctpd: Send Discovery Notify on Endpoint role set #15

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+93 −8
Open

mctpd: Send Discovery Notify on Endpoint role set #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64f4f53
mctp-netlink: Store interface physical address in linkmap
Jul 15, 2024
73e2567
mctpd: Send Discovery Notify on Endpoint role set
Jul 15, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
5. mctpd: New test infrastructure for control procotol messaging to mctpd
6. mctpd: Add a configuration file facility, defaulting to /etc/mctpd.conf.
7. mctpd: Add mctp/interfaces/<name> D-Bus object
8. mctpd: Send Discovery Notify on Endpoint role set

### Changed

Expand Down
40 changes: 34 additions & 6 deletions src/mctp-netlink.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
struct linkmap_entry {
int ifindex;
char ifname[IFNAMSIZ+1];
uint8_t ifaddr[MAX_ADDR_LEN];
size_t ifaddr_len;
int net;
bool up;

Expand Down Expand Up @@ -53,8 +55,9 @@ static int fill_local_addrs(mctp_nl *nl);
static int fill_linkmap(mctp_nl *nl);
static void sort_linkmap(mctp_nl *nl);
static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info,
const char *ifname, size_t ifname_len, int net,
bool up);
const char *ifname, size_t ifname_len,
uint8_t *ifaddr, size_t ifaddr_len, int net,
bool up);
static struct linkmap_entry *entry_byindex(const mctp_nl *nl,
int index);

Expand Down Expand Up @@ -679,8 +682,9 @@ static int parse_getlink_dump(mctp_nl *nl, struct nlmsghdr *nlh, uint32_t len)

for (; NLMSG_OK(nlh, len); nlh = NLMSG_NEXT(nlh, len)) {
struct rtattr *rta, *rt_nest, *rt_mctp;
uint8_t *ifaddr;
char *ifname;
size_t ifname_len, rlen, nlen, mlen;
size_t ifname_len, ifaddr_len, rlen, nlen, mlen;
uint32_t net;
bool up;

Expand Down Expand Up @@ -722,8 +726,13 @@ static int parse_getlink_dump(mctp_nl *nl, struct nlmsghdr *nlh, uint32_t len)
continue;
}
ifname_len = strnlen(ifname, ifname_len);

ifaddr = mctp_get_rtnlmsg_attr(IFLA_ADDRESS, rta, rlen,
&ifaddr_len);

up = info->ifi_flags & IFF_UP;
linkmap_add_entry(nl, info, ifname, ifname_len, net, up);
linkmap_add_entry(nl, info, ifname, ifname_len, ifaddr,
ifaddr_len, net, up);
}
// Not done.
return 1;
Expand Down Expand Up @@ -927,6 +936,16 @@ const char* mctp_nl_if_byindex(const mctp_nl *nl, int index)
return NULL;
}

uint8_t *mctp_nl_ifaddr_byindex(const mctp_nl *nl, int index, size_t *ret_len)
{
struct linkmap_entry *entry = entry_byindex(nl, index);
if (entry) {
*ret_len = entry->ifaddr_len;
return entry->ifaddr;
}
return NULL;
}

int mctp_nl_net_byindex(const mctp_nl *nl, int index)
{
struct linkmap_entry *entry = entry_byindex(nl, index);
Expand Down Expand Up @@ -1054,8 +1073,9 @@ int *mctp_nl_if_list(const mctp_nl *nl, size_t *ret_num_ifs)
}

static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info,
const char *ifname, size_t ifname_len, int net,
bool up)
const char *ifname, size_t ifname_len,
uint8_t *ifaddr, size_t ifaddr_len, int net,
bool up)
{
struct linkmap_entry *entry;
size_t newsz;
Expand All @@ -1067,6 +1087,12 @@ static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info,
return -1;
}

if (ifaddr_len > MAX_ADDR_LEN) {
warnx("linkmap, too long ifaddr (%zu bytes long, expected max %d bytes)",
ifaddr_len, MAX_ADDR_LEN);
return -1;
}

if (net <= 0) {
warnx("Bad network ID %d for %*s", net, (int)ifname_len, ifname);
return -1;
Expand All @@ -1088,6 +1114,8 @@ static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info,
entry = &nl->linkmap[idx];
memset(entry, 0, sizeof(*entry));
snprintf(entry->ifname, IFNAMSIZ, "%*s", (int)ifname_len, ifname);
memcpy(entry->ifaddr, ifaddr, ifaddr_len);
entry->ifaddr_len = ifaddr_len;
entry->ifindex = info->ifi_index;
entry->net = net;
entry->up = up;
Expand Down
1 change: 1 addition & 0 deletions src/mctp-netlink.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ int mctp_nl_recv_all(mctp_nl *nl, int sd,
/* Lookup MCTP interfaces */
int mctp_nl_ifindex_byname(const mctp_nl *nl, const char *ifname);
const char* mctp_nl_if_byindex(const mctp_nl *nl, int index);
uint8_t *mctp_nl_ifaddr_byindex(const mctp_nl *nl, int index, size_t *ret_len);
int mctp_nl_net_byindex(const mctp_nl *nl, int index);
bool mctp_nl_up_byindex(const mctp_nl *nl, int index);
/* Caller to free */
Expand Down
59 changes: 57 additions & 2 deletions src/mctpd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2895,6 +2895,48 @@ static int bus_link_get_prop(sd_bus *bus,
return rc;
}

static int notify_discovery(ctx *ctx, int ifindex)
{
int rc = 0;
dest_phys desti = { 0 }, *dest = &desti;
struct mctp_ctrl_cmd_discovery_notify req = { 0 };
struct mctp_ctrl_resp_discovery_notify *resp;
uint8_t *buf;
size_t buf_size;
struct sockaddr_mctp_ext resp_addr;

dest->ifindex = ifindex;
mctp_nl_ifaddr_byindex(ctx->nl, dest->ifindex, &dest->hwaddr_len);
memset(dest->hwaddr, 0, sizeof dest->hwaddr);
Copy link
Member

Choose a reason for hiding this comment

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

A zero hwaddr is specific to PCIe, right? Do we also need to know the physical broadcast address for this link type?

Copy link
Author

@khangng-ampere khangng-ampere Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jk-ozlabs The MCTP base spec does not specify that, so I think this depends on the link. (And it is the "default" route to the bus owner, not specifically "broadcast").

As for PCIe, you actually need to specify a routing type before sending it to the bus (Route-to-Root-Complex, Broadcast-from-Root-Complex, Route-by-ID), which I don't find a way to put that in the sockaddr, so I pick the following addressing convention:

  • 0x00:00 for "default route", which corresponds to Route-to-Root-Complex,
  • 0xFF:FF for "broadcast route", which corresponds to Broadcast-from-Root-Complex,
  • The rest corresponds to Route-by-ID.

(Only Route-by-ID routing type will actually use the address, the formers will simply ignore the value per the PCIe or DSP0238 spec)

Copy link
Member

Choose a reason for hiding this comment

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

which I don't find a way to put that in the sockaddr

phys addresses don't belong in the struct sockaddr_mctp, they would go into the smctp_haddr of the struct sockaddr_mctp_ext, which is just bytes. If you need the routing type represented in there as well, that's entirely possible too.

But this sounds like review details on the driver itself, so probably not the best forum here :)

We do need to ensure we're using values that would work for any transport in future though, and an all-zero dest phys may not be suitable for those...


req.ctrl_hdr.command_code = MCTP_CTRL_CMD_DISCOVERY_NOTIFY;
req.ctrl_hdr.rq_dgram_inst = RQDI_REQ;

rc = endpoint_query_phys(ctx, dest, MCTP_CTRL_HDR_MSG_TYPE, &req,
sizeof(req), &buf, &buf_size, &resp_addr);
Copy link
Member

Choose a reason for hiding this comment

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

endpoint_query_phys will wait for a response, but Discovery Notify does not send a response. We'll probably need a different helper for datagram-style messages.

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(dest));
rc = -ENOMSG;
goto free_buf;
}
resp = (void *)buf;

if (resp->completion_code != 0) {
// TODO: make this a debug message?
warnx("Failure completion code 0x%02x from %s",
resp->completion_code, dest_phys_tostr(dest));
rc = -ECONNREFUSED;
goto free_buf;
}
free_buf:
free(buf);
return rc;
}

static int bus_link_set_prop(sd_bus *bus,
const char *path, const char *interface, const char *property,
sd_bus_message *value, void *userdata, sd_bus_error *berr)
Expand All @@ -2906,6 +2948,7 @@ static int bus_link_set_prop(sd_bus *bus,
link_userdata *lmUserData;
int rc;
struct role role;
int ifindex;

if (!is_interfaces_path(path)) {
sd_bus_error_setf(berr, SD_BUS_ERROR_INVALID_ARGS,
Expand All @@ -2921,11 +2964,13 @@ static int bus_link_set_prop(sd_bus *bus,
goto out;
}

lmUserData = mctp_nl_get_link_userdata_byname(ctx->nl, link_name);
if (!lmUserData) {
ifindex = mctp_nl_ifindex_byname(ctx->nl, link_name);
if (!ifindex) {
rc = -ENOENT;
goto out;
}
lmUserData = mctp_nl_get_link_userdata(ctx->nl, ifindex);
assert(lmUserData);

if (strcmp(property, "Role") != 0) {
printf("Unknown property '%s' for %s iface %s\n", property, path, interface);
Expand Down Expand Up @@ -2956,6 +3001,16 @@ static int bus_link_set_prop(sd_bus *bus,
}
lmUserData->role = role.role;

// Announce on the bus we are endpoint, print warning and ignore error if failed
if (lmUserData->role == ENDPOINT_ROLE_ENDPOINT) {
rc = notify_discovery(ctx, ifindex);
Copy link
Member

Choose a reason for hiding this comment

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

This is done unconditionally, but Discovery Notify messages should only be sent on transport types that require them. We may need a way to query the transport type?

Copy link
Author

Choose a reason for hiding this comment

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

@jk-ozlabs It would be great if we have that now (we need them to get control message timing information for the recovery mechanism on each interface anyway).

I would propose adding such information to when a binding driver register itself to the MCTP networking subsystem inside the kernel (mctp_netdev_ops), like so:

static const struct mctp_netdev_ops mctp_i2c_mctp_ops = {
	.transport = MCTP_TRANSPORT_SMBUS,
	.release_flow = mctp_i2c_release_flow,
};

And then exposing them to userspace through netlink.

I don't know how long it would take to merge the patch to linux, but in the meanwhile, we may temporarily provide such information in mctpd by basing on the interface name i think. Everything is already somewhat following the convention: mctpi2c%d, mctpserial%d, ...

I could probably open a PR for this if you think this is ok.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we'd need the kernel to provide the physical medium identifier. It doesn't belong in the ops struct, but there are certainly ways we can provide that to the struct mctp_dev in the kernel (and then on to the netlink interface as a new attribute; say IFLA_MCTP_PHYS_MEDIUM).

Given that:

  1. the PCIe VDM transport is the only one that requires a Discovery Notify
  2. that transport driver is not yet upstream
  3. none of the currently-upstream transport drivers provide bindings that require Discovery Notify messages

- then I would suggest that we add the infrastructure for IFLA_MCTP_PHYS_MEDIUM, and have your driver provide the attribute. Then, mctpd will only send Discovery Notify messages attribute is present (ie., we know the medium type) and matches the media types that need a Discovery Notify.

Since the Discovery Notify is only required for the non-upstream transport, you'll need kernel patches anyway.

we may temporarily provide such information in mctpd by basing on the interface name i think. Everything is already somewhat following the convention: mctpi2c%d, mctpserial%d, ...

Interface names are not a stable API that we can rely on; they can be renamed arbitrarily.

I'm happy to do the IFLA_MCTP_PHYS_MEDIUM support; drivers will just need a minor change to populate the type value.

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 happy to do the IFLA_MCTP_PHYS_MEDIUM support; drivers will just need a minor change to populate the type value.

That would be great 😀 Thank you!

if (rc) {
warnx("Warning: discovery notify on interface '%s' failed: %s", link_name,
strerror(-rc));
rc = 0;
}
}

out:
set_berr(ctx, rc, berr);
return rc;
Expand Down