Skip to content

Support LocaEids property in au.com.codeconstruct.MCTP.Network1 #54

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

Closed
ThuBaNguyen wants to merge 1 commit into from
Closed

Support LocaEids property in au.com.codeconstruct.MCTP.Network1 #54

Show file tree
Hide file tree
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
1. New debug tool, `mctp-bench`, for sending and receiving a stream of MCTP
messages between two processes.

2. mctpd: Add `au.com.codeconstruct.MCTP.Network1` interface

## [2.1] - 2024-12-16

### Fixed
Expand Down
21 changes: 21 additions & 0 deletions docs/mctpd.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,27 @@ Like SetupEndpoint but will not assign EIDs, will only query endpoints for a
current EID. The `new` return value is set to `false` for an already known
endpoint, or `true` when an endpoint's EID is newly discovered.

## Network objects: `/au/com/codeconstruct/networks/<net>`

These objects represent MCTP networks which have been added use `mctp link`
commands. These will be 1:1 with the MCTP networks on the system.

These objects host the interface `au.com.codeconstruct.MCTP.Network1`.

### MCTP network interface: `au.com.codeconstruct.MCTP.Network1`

All MCTP networks objects host the `au.com.codeconstruct.MCTP.Network1` dbus
interface:

```
NAME TYPE SIGNATURE RESULT/VALUE FLAGS
au.com.codeconstruct.MCTP.Network1 interface - - -
.LocalEIDs property ay 1 8 const
```

The D-Bus interface includes the `LocalEIDs` property which reports BMC local EIDs
in the network.

## Endpoint objects: `/au/com/codeconstruct/networks/<net>/endpoints/<eid>`

These objects represent MCTP endpoints that `mctpd` has either discovered
Expand Down
115 changes: 115 additions & 0 deletions src/mctpd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
#define MCTP_DBUS_IFACE_ENDPOINT "xyz.openbmc_project.MCTP.Endpoint"
#define OPENBMC_IFACE_COMMON_UUID "xyz.openbmc_project.Common.UUID"
#define CC_MCTP_DBUS_IFACE_INTERFACE "au.com.codeconstruct.MCTP.Interface1"
#define CC_MCTP_DBUS_NETWORK_INTERFACE "au.com.codeconstruct.MCTP.Network1"

// 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 @@ -288,6 +289,31 @@ static peer * find_peer_by_addr(ctx *ctx, mctp_eid_t eid, int net)
return NULL;
}

static int find_local_eids_by_net(ctx *ctx, uint32_t net,
size_t* local_eid_cnt,
mctp_eid_t *ret_eids)
{
size_t local_count = 0;
net_det *n = lookup_net(ctx, net);
peer *peer;

*local_eid_cnt = 0;
if (!n)
return -EINVAL;

for (size_t t = 0; t < 256; t++) {
if (n->peeridx[t] < 0)
continue;

peer = &ctx->peers[n->peeridx[t]];
if (peer && (peer->state == LOCAL))
ret_eids[local_count++] = t;
}
*local_eid_cnt = local_count;

return 0;
}
Copy link
Member

@jk-ozlabs jk-ozlabs Dec 20, 2024

Choose a reason for hiding this comment

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

You have essentially the same logic repeated here. Just return the count and array from the one function.

Copy link
Member

@jk-ozlabs jk-ozlabs Jan 7, 2025

Choose a reason for hiding this comment

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

This has been re-introduced in the recent changes; does this iteration code need to appear twice?

Copy link
Contributor Author

@ThuBaNguyen ThuBaNguyen Jan 7, 2025

Choose a reason for hiding this comment

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

Yes.

We need to know the number of localEIDs first, then allocate ret_eids with number of localEIDs.
In the commit 17adff2, I tried to change combine find_local_eids_count_by_net and find_local_eids_by_net but eids is null event num is 1 in sd_bus_message_append_array(reply, 'y', eids, num);. That why I back to use two functions.

Copy link
Contributor Author

@ThuBaNguyen ThuBaNguyen Jan 7, 2025

Choose a reason for hiding this comment

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

Using two functions also makes the code is clearer.

Copy link
Member

@jk-ozlabs jk-ozlabs Jan 7, 2025

Choose a reason for hiding this comment

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

Using two functions also makes the code is clearer.

I disagree. You already have divergence between the two implementations (code formatting), as well as a bug duplicated in both (&ctx->peers[x] can never be null)

The number of local EIDs is fairly well constrained; does this even need to be calculated in advance? just malloc the whole 256 bytes, and iterate once.

but eids is null event num is 1 in sd_bus_message_append_array(reply, 'y', eids, num);

not sure I understand this, can you rephrase?

Copy link
Contributor Author

@ThuBaNguyen ThuBaNguyen Jan 7, 2025

Choose a reason for hiding this comment

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

The number of local EIDs is fairly well constrained; does this even need to be calculated in advance? just malloc the whole 256 bytes, and iterate once.

I updated the latest code by malloc the localEIDs with 256 bytes as your suggest.
edc3f5f
Only need one functions and iterate once.


/* Returns a deferred free pointer */
static const char* dest_phys_tostr(const dest_phys *dest)
{
Expand Down Expand Up @@ -2915,6 +2941,26 @@ static bool is_endpoint_path(const char *path)
return true;
}

static bool get_networkid_from_path(const char *path, uint32_t* netid)
{
char *netstr = NULL;
int rc;

rc = sd_bus_path_decode_many(path,
MCTP_DBUS_PATH "/networks/%",
&netstr);

if (rc <= 0)
return false;

dfree(netstr);

if (parse_uint32(netstr, netid) < 0)
return false;

return true;
}

static bool is_interfaces_path(const char *path)
{
char *intfName = NULL;
Expand Down Expand Up @@ -2966,6 +3012,33 @@ static int bus_endpoint_get_prop(sd_bus *bus,
return rc;
}

static int bus_network_get_prop(sd_bus *bus,
const char *path, const char *interface, const char *property,
sd_bus_message *reply, void *userdata, sd_bus_error *berr)
{
ctx *ctx = userdata;
int rc = 0;
uint32_t netid;
mctp_eid_t *eids = (mctp_eid_t *)malloc(256);
size_t num;

if (!get_networkid_from_path(path, &netid)) {
return -ENOENT;
}

if (strcmp(property, "LocalEIDs") == 0) {
rc = find_local_eids_by_net(ctx, netid, &num, eids);
if (rc < 0) {
return -ENOENT;
}

dfree(eids);
rc = sd_bus_message_append_array(reply, 'y', eids, num);
}

return rc;
}

static int bus_link_get_prop(sd_bus *bus,
const char *path, const char *interface, const char *property,
sd_bus_message *reply, void *userdata, sd_bus_error *berr)
Expand Down Expand Up @@ -3165,6 +3238,16 @@ static const sd_bus_vtable bus_endpoint_link_vtable[] = {
SD_BUS_VTABLE_END
};

static const sd_bus_vtable bus_network_vtable[] = {
SD_BUS_VTABLE_START(0),
SD_BUS_PROPERTY("LocalEIDs",
"ay",
bus_network_get_prop,
0,
SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_VTABLE_END
};

static const sd_bus_vtable bus_endpoint_cc_vtable[] = {
SD_BUS_VTABLE_START(0),
SD_BUS_METHOD_WITH_ARGS("SetMTU",
Expand Down Expand Up @@ -3218,6 +3301,26 @@ static int bus_endpoint_find(sd_bus *bus, const char *path,
return 0;
}

static int bus_mctp_network_find(sd_bus *bus, const char *path,
const char *interface, void *userdata, void **ret_found,
sd_bus_error *ret_error)
{
ctx *ctx = userdata;
uint32_t netid;

if (!get_networkid_from_path(path, &netid)) {
return 0;
}

net_det *n = lookup_net(ctx, netid);
if (n) {
*ret_found = ctx;
return 1;
}

return 0;
}

/* Common.UUID interface is only added for peers that have a UUID. */
static int bus_endpoint_find_uuid(sd_bus *bus, const char *path,
const char *interface, void *userdata, void **ret_found,
Expand Down Expand Up @@ -3710,6 +3813,18 @@ static int setup_bus(ctx *ctx)
warnx("Failed creating D-Bus object");
goto out;
}

rc = sd_bus_add_fallback_vtable(ctx->bus, NULL,
MCTP_DBUS_PATH,
CC_MCTP_DBUS_NETWORK_INTERFACE,
bus_network_vtable,
bus_mctp_network_find,
ctx);
if (rc < 0) {
warnx("Failed adding Network D-Bus interface: %s", strerror(-rc));
goto out;
}

rc = sd_bus_add_object_manager(ctx->bus, NULL, MCTP_DBUS_PATH);
if (rc < 0) {
warnx("Adding object manager failed: %s", strerror(-rc));
Expand Down
Loading