Support get static eid config from entity manager by BonnieLo · Pull Request #17 · CodeConstruct/mctp

BonnieLo · 2023-10-17T07:05:40Z

The mctpd will try to find the configuration interface "xyz.openbmc_project.Configuration.MctpDevice" and parse the data. Then it will send get EID command to target device. If we the get the response, we publish the EID on the D-Bus.

Test log:

reboot and check MCTP D-Bus path root@bmc:~# reboot

root@bmc:# busctl tree xyz.openbmc_project.MCTP
- /xyz - /xyz/openbmc_project - /xyz/openbmc_project/mctp - /xyz/openbmc_project/mctp/1 |- /xyz/openbmc_project/mctp/1/60 `- /xyz/openbmc_project/mctp/1/8 root@bmc:# pldmtool base GetTID -m 60
{
"Response": 134
}

The mctpd will try to find the configuration interface "xyz.openbmc_project.Configuration.MCTPEndpoint" and parse the data. Then it will send get EID command to target device. If we the get the response, we publish the EID on the D-Bus. Test log: - reboot and check MCTP D-Bus path root@bmc:~# reboot root@bmc:~# busctl tree xyz.openbmc_project.MCTP `- /xyz `- /xyz/openbmc_project `- /xyz/openbmc_project/mctp `- /xyz/openbmc_project/mctp/1 |- /xyz/openbmc_project/mctp/1/60 `- /xyz/openbmc_project/mctp/1/8 root@bmc:~# pldmtool base GetTID -m 60 { "Response": 134 } upstream link: CodeConstruct#17

williamspatrick · 2023-11-14T22:40:53Z

@jk-ozlabs @mkj - Is there any feedback on this PR? It has been open for a month with no action.

jk-ozlabs · 2023-11-14T23:53:03Z

@jk-ozlabs @mkj - Is there any feedback on this PR? It has been open for a month with no action.

So there was a discussion in OpenBMC gerrit, at https://gerrit.openbmc.org/c/openbmc/entity-manager/+/66750, you mentioned:

I really don't like that we have undocumented dbus interfaces now spanning even outside the OpenBMC organization.
Should we start documenting these EM config types in phosphor-dbus-interfaces? (@edtanous - opinions?)

And I replied:

Yep, these should be tracked with the rest of the config structure.

Alternatively, we could make a policy that the EM config types don't leak out to other projects (or at least those we have input on), and provide a more "generic" configuration mechanism for mctpd.

So: this is pending a decision on whether we want to expose the EM interfaces outside of OpenBMC.

If not, we'll need a bit of bridge code, within OpenBMC, to perform configuration on non-openbmc code (this change is essentially a shortcut for calling the SetupEndpoint method based on EM config, which could justifiable be done elsewhere)

If so, we should be fine with this as-is.

Let me know what your preference with the interface.

PeterHo-wiwynn · 2023-12-15T08:22:06Z

Hi @jk-ozlabs,
Since the patch https://gerrit.openbmc.org/c/openbmc/entity-manager/+/66750 has been merged, would you mind take a look at this PR? I did added some changes to this PR, so it is a little different from the origin one. The commit message lists all the changes in this PR.

jk-ozlabs · 2023-12-18T09:16:41Z

Hi @jk-ozlabs, Since the patch https://gerrit.openbmc.org/c/openbmc/entity-manager/+/66750 has been merged, would you mind take a look at this PR?

Sure thing! I'll get that done this week.

jk-ozlabs

Hi!

Thanks for the change, overall I'm happy with the strategy here.

There are a few comments on the code though. Most are super minor, except for the InterfacesAdded / InterfacesRemoved handling - if possible, we probably want to filter those at the match level too, so we're not getting floods of signals.

jk-ozlabs · 2023-12-22T06:50:29Z

+		rc = sd_bus_message_read(m, "s", &iface);
+		if (rc == 0)
+			break;
+		if(strcmp(iface,OPENBMC_MCTP_CONFIG_IFACE) == 0) {


You have two different spacing styles here:

if (...)

vs

if(...)

The former is more consistent with existing code.

(Super minor, only worth changing if you're re-working this commit otherwise. However, there are several other instances of this in the patch)

jk-ozlabs · 2023-12-22T06:50:42Z

+		}
+	}
+
+	if (update == true) {


if (update)

You may also want to return early here, and avoid the indent below.

jk-ozlabs · 2023-12-22T06:51:10Z

+		char *ret = strrchr(temp, '/');
+		*ret = '\0';
+
+		for(;;) {


you seem to be using a for to define an infinite loop, but then working over an iterator. How about:

for (peer = find_peer_by_association(ctx, temp); peer; peer = find_peer_by_association(ctx, temp)) { remove_peer(peer) }

the repetition isn't ideal, so I'm also okay with:

for (;;) { peer = find_peer_by_association(ctx, temp); if (!peer) break; remove_peer(peer) }

(also, spacing again around the for)

jk-ozlabs · 2023-12-22T07:06:56Z

+		sd_bus_message_read(message, "t", &data);
+
+		size = sizeof(uint8_t);
+		memset(dest->hwaddr, 0x0, MAX_ADDR_LEN);


This will already be zeroed above.

jk-ozlabs · 2023-12-22T07:10:47Z

+
+		rc = add_peer(ctx, dest, (mctp_eid_t)eid, net, &peer);
+		if (rc < 0) {
+			warnx("Failed to add peer for EID %d", (mctp_eid_t)eid);


maybe do the cast once after extracting from the sb_bus_message_read(), instead of every subsequent use of eid.

jk-ozlabs · 2023-12-22T07:12:04Z

+		char object_path[100];
+		strcpy(object_path, s[i]);
+		char *ret = strrchr(object_path, '/');
+		*ret = '\0';


This could be a NULL pointer.

jk-ozlabs · 2023-12-22T07:13:01Z

+		char *ret = strrchr(object_path, '/');
+		*ret = '\0';
+
+		peer->association = malloc(sizeof(struct association));


We could have this allocated as part of the peer, and then just use a boolean flag to indicate no association.

Done. I added this to add_peer.

jk-ozlabs

I'll take a look at options for filtering InterfacesAdded, but I think we'll just go with what you have here. A few minor changes included as comments, but looks good so far.

jk-ozlabs · 2024-01-02T14:51:15Z

+		if (strcmp(iface, OPENBMC_MCTP_CONFIG_IFACE) != 0) {
+			sd_bus_message_skip(m, "a{sv}");
+		}
+		else {


some unusual indenting here; just keep that on the same line:

} else {

jk-ozlabs · 2024-01-02T14:52:13Z

+	return rc;
+}
+
+int on_interface_removed(sd_bus_message *m, void *userdata, sd_bus_error *ret_error)


jk-ozlabs · 2024-01-02T14:52:15Z

+	}
+	sd_bus_message_exit_container(m);
+	//TODO: parse message from signal instead of recanning
+	if (update == true)


if (update)

jk-ozlabs · 2024-01-02T14:52:31Z

 	return rc;
 }

+int on_interface_added(sd_bus_message *m, void *userdata, sd_bus_error *ret_error)


jk-ozlabs · 2024-01-02T15:14:44Z

+			&error, &message, "t");
+		if (rc < 0) {
+			warnx("Failed to get property: %s", error.message);
+			goto out;


You're also generating the same error message as above, so there will be no way to tell which path failed.

Done. I also revised the message.

jk-ozlabs · 2024-01-02T15:15:39Z

 		rc = sd_bus_message_append(reply, "s", s);
+	} else if (strcmp(property, "Associations") == 0 && peer->association) {
+		rc = sd_bus_message_append(reply, "a(sss)",
+			1, peer->association->forward, peer->association->reverse, peer->association->object_path);


super minor, but can you wrap at 80 here?

jk-ozlabs · 2024-01-02T15:16:35Z

+
+	rc = peer_from_path(ctx, path, &peer);
+	if (rc >= 0 && peer->published) {
+		if (peer->association) {


just include this in the conditional above maybe?

jk-ozlabs · 2024-01-02T15:16:46Z

+		sd_bus_message_exit_container(m);
+	}
+	sd_bus_message_exit_container(m);
+	//TODO: parse message from signal instead of recanning


What do you mean by 'recanning' here?

Sorry for typo. It should be 'rescanning'.

jk-ozlabs · 2024-01-02T15:17:47Z

+	if (update == true)
+		setup_static_eid(ctx);
+
+out:


do you really need this label? there's no cleanup code, you could just return directly from the error cases.

Done. Removed it.

jk-ozlabs · 2024-01-03T09:45:40Z

+
+	rc = peer_from_path(ctx, path, &peer);
+	if (rc >= 0 && peer->published && peer->association) {
+			*ret_found = peer;


only minor, but indentation is off here.

jk-ozlabs · 2024-01-03T09:46:31Z

+	}
+
+	char temp[100];
+	strcpy(temp, object_path);


Same potential overflow here

I also made it check the length here.

Same off-by-one as above

jk-ozlabs

Looking good now - just have a couple of potential stack overflows to resolve. If you'd prefer to do those in a separate change, let me know, and I'll merge this and address them separately.

jk-ozlabs · 2024-01-03T09:57:20Z

And one last thing: can you add a Signed-off-by line, to indicate that you agree to the DCO: https://developercertificate.org/

ThuBaNguyen · 2024-01-04T00:02:47Z

In which condition the Static Endpoint object path will be created? Because the https://gerrit.openbmc.org/c/openbmc/entity-manager/+/66750 creates the Association without whenever the configuration is existing.
I suggest to check the available of Endpoint in the Bus before adding them to the MCTP Interface. Adding them without checking available can cause the failure in Discovery and Init Agent steps in PLDM service. Which can cause there is no sensors for the Endpoints.
We can check if the Endpoint is available by checking existing of an I2C Device in I2C bus by reading Address 0 of device. The successful of reading can understand as device is ready.

Summary: Updated latest patch for mctpd. CodeConstruct/mctp#17 # Description Please include a summary of the change and which issue is fixed. # Motivation Please include an explanation of why you these changes are necessary X-link: https://github.com/facebookexternal/openbmc.wiwynn/pull/2938 Test Plan: Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Reviewed By: williamspatrick Differential Revision: D52501259 fbshipit-source-id: 35015fa16ed6b2d312a26c1f2cf0ef4a8e1ecf8c

PeterHo-wiwynn · 2024-01-04T05:31:27Z

Hi @ThuBaNguyen,

We made it sent the MCTP Get Endpoint ID before publishing EID on D-Bus. It only publishes the device which has response.

amboar · 2024-01-31T02:31:22Z

Okay, here's a proof-of-concept:

https://gerrit.openbmc.org/q/topic:%22mctpreactor%22

It doesn't yet handle associations between the EM config and the endpoint objects exposed by mctpd. However, it does handle configuring mctpd with exposed EM configs, and also re-establishing the setup of endpoints objects that are removed by mctpd while their EM configs remain exposed.

Issues that need to be addressed:

Associations as noted above
The EM MCTPEndpoint config schema needs to specify Interface rather than Bus as a member
The PoC code needs to support the Interface property
The PoC code needs to be generalised beyond SmbusMctpdDevice

Made it get the inventory path from association created by `mctpd` or configuration in static endpoint table. The inventory path is for platform-mc terminus. Tested with: CodeConstruct/mctp#17 https://gerrit.openbmc.org/c/openbmc/entity-manager/+/66750 which gets static EID from entity-manager and creates association definition D-Bus interface. Change-Id: Ic9d0b6cd602d71a6fa51a73bd45964c2a0dac6aa Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>

The inventory name was set as PLDM_DEVICE_TID, but we might need more specific name to identify inventory item. This change made it get inventory path found by MctpDiscovery. It keeps the original path name by default if it didn't get the path from MctpDiscovery. Tested with: CodeConstruct/mctp#17 https://gerrit.openbmc.org/c/openbmc/entity-manager/+/66750 which gets static EID from entity-manager and creates association definition D-Bus interface. Change-Id: I17b6c56722df180defa1f769a54fd4b10df22d71 Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>

Made it get the inventory path from association created by `mctpd` or configuration in static endpoint table. The inventory path is for platform-mc terminus. Tested with: CodeConstruct/mctp#17 https://gerrit.openbmc.org/c/openbmc/entity-manager/+/66750 which gets static EID from entity-manager and creates association definition D-Bus interface. Change-Id: Ic9d0b6cd602d71a6fa51a73bd45964c2a0dac6aa Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>

The inventory name was set as PLDM_DEVICE_TID, but we might need more specific name to identify inventory item. This change made it get inventory path found by MctpDiscovery. It keeps the original path name by default if it didn't get the path from MctpDiscovery. Tested with: CodeConstruct/mctp#17 https://gerrit.openbmc.org/c/openbmc/entity-manager/+/66750 which gets static EID from entity-manager and creates association definition D-Bus interface. Change-Id: I17b6c56722df180defa1f769a54fd4b10df22d71 Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>

amboar · 2024-02-09T03:42:20Z

Okay, I've been chipping away at improvements to the mctpreactor proof-of-concept. As of patchset 8 in Gerrit it adds and removes the (proxy) associations as necessary, ticking off the first todo above. It also supplies a systemd service unit which should make it a bit more convenient.

Next thing that needs attention is fixing up the schema in EM.

Regarding the proxy associations, here's an example busctl invocation that will allow an application to traverse back from an mctpd endpoint object to the inventory associated with it by way of the association hosted by mctpreactor:

root@cc-nvme-mi:~# busctl call \
> xyz.openbmc_project.ObjectMapper \
> /xyz/openbmc_project/object_mapper \
> xyz.openbmc_project.ObjectMapper \
> GetAssociatedSubTree ooias \
> /xyz/openbmc_project/mctp/1/9/chassis \
> / \
> 0 \
> 1 xyz.openbmc_project.Configuration.MCTPEndpoint
a{sa{sas}} 1 "/xyz/openbmc_project/inventory/system/nvme/NVMe_1/NVMe_1_Temp" 1 "xyz.openbmc_project.EntityManager" 1 "xyz.openbmc_project.Configuration.MCTPEndpoint"

I think the association forward and reverse names probably need some bikeshedding, but for now I've just copied what was created by the patches in this PR.

While mctpd[1] may see heavy use in projects such as OpenBMC, it implements generic functionality necessary to operate MCTP as a protocol. It therefore should be easy to use in other contexts, and so it feels unwise to embed OpenBMC-specific details in its implementation. Conversely, entity-manager's scope is to expose inventory and board configuration. It externalises all other responsibilities for the sake of stability and maintenance. While entity-manager is central to OpenBMC's implementation and has little use in other contexts, embedding details of how to configure mctpd in entity-manager exceeds its scope. Thus we reach the design point of mctpreactor, an intermediary process that encapsulates OpenBMC-specific and mctpd-specific behaviours to constrain their dispersion in either direction. The design-point was reached via discussion at [2]. mctpreactor is implemented in terms of two new expose schemas in entity-manager: - MCTPInterface - MCTPDevice mctpreactor tracks instances of both appearing as a result of inventory changes, and uses the provided information to dynamically configure the endpoints via mctpd. This dynamic configuration may include assignment of static endpoint IDs to the devices as they appear. The lifecycle of an MCTP device can be quite dynamic - mctpd provides behaviours to recover[3] or remove endpoints from the network. Their presence cannot be assumed. mctpreactor handles these events: If a device is removed at the MCTP layer (as it may be unresponsive), mctpreactor will periodically attempt to re-establish it as an endpoint so long as the associated configuration on the entity-manager inventory object remains exposed. [1]: https://github.com/CodeConstruct/mctp/ [2]: CodeConstruct/mctp#17 [3]: https://github.com/CodeConstruct/mctp/blob/7ec2f8daa3a8948066390aee621d6afa03f6ecd9/docs/endpoint-recovery.md Change-Id: I5e362cf6e5ce80ce282bab48d912a1038003e236 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

While mctpd[1] may see heavy use in projects such as OpenBMC, it implements generic functionality necessary to operate MCTP as a protocol. It therefore should be easy to use in other contexts, and so it feels unwise to embed OpenBMC-specific details in its implementation. Conversely, entity-manager's scope is to expose inventory and board configuration. It externalises all other responsibilities for the sake of stability and maintenance. While entity-manager is central to OpenBMC's implementation and has little use in other contexts, embedding details of how to configure mctpd in entity-manager exceeds its scope. Thus we reach the design point of mctpreactor, an intermediary process that encapsulates OpenBMC-specific and mctpd-specific behaviors to constrain their dispersion in either direction. The design-point was reached via discussion at [2]. mctpreactor is implemented in terms of two new expose schemas in entity-manager: - MCTPInterface - MCTPDevice mctpreactor tracks instances of both appearing as a result of inventory changes, and uses the provided information to dynamically configure the endpoints via mctpd. This dynamic configuration may include assignment of static endpoint IDs to the devices as they appear. The lifecycle of an MCTP device can be quite dynamic - mctpd provides behaviors to recover[3] or remove endpoints from the network. Their presence cannot be assumed. mctpreactor handles these events: If a device is removed at the MCTP layer (as it may be unresponsive), mctpreactor will periodically attempt to re-establish it as an endpoint so long as the associated configuration on the entity-manager inventory object remains exposed. [1]: https://github.com/CodeConstruct/mctp/ [2]: CodeConstruct/mctp#17 [3]: https://github.com/CodeConstruct/mctp/blob/7ec2f8daa3a8948066390aee621d6afa03f6ecd9/docs/endpoint-recovery.md Change-Id: I5e362cf6e5ce80ce282bab48d912a1038003e236 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

amboar · 2024-02-26T01:55:46Z

alright, I think the mctpreactor bits are in decent enough shape for people to review:

I've done a conversion of the Yosemit4 configs to use the new schemas, but I'm not sure the config as-specified was well formed:

configurations: Convert Yosemite4 to MCTPDevice schema

While mctpd[1] may see heavy use in projects such as OpenBMC, it implements generic functionality necessary to operate MCTP as a protocol. It therefore should be easy to use in other contexts, and so it feels unwise to embed OpenBMC-specific details in its implementation. Conversely, entity-manager's scope is to expose inventory and board configuration. It externalises all other responsibilities for the sake of stability and maintenance. While entity-manager is central to OpenBMC's implementation and has little use in other contexts, embedding details of how to configure mctpd in entity-manager exceeds its scope. Thus we reach the design point of mctpreactor, an intermediary process that encapsulates OpenBMC-specific and mctpd-specific behaviors to constrain their dispersion in either direction. The design-point was reached via discussion at [2]. mctpreactor is implemented in terms of two new expose schemas in entity-manager: - MCTPInterface - MCTPDevice mctpreactor tracks instances of both appearing as a result of inventory changes, and uses the provided information to dynamically configure the endpoints via mctpd. This dynamic configuration may include assignment of static endpoint IDs to the devices as they appear. The lifecycle of an MCTP device can be quite dynamic - mctpd provides behaviors to recover[3] or remove endpoints from the network. Their presence cannot be assumed. mctpreactor handles these events: If a device is removed at the MCTP layer (as it may be unresponsive), mctpreactor will periodically attempt to re-establish it as an endpoint so long as the associated configuration on the entity-manager inventory object remains exposed. [1]: https://github.com/CodeConstruct/mctp/ [2]: CodeConstruct/mctp#17 [3]: https://github.com/CodeConstruct/mctp/blob/7ec2f8daa3a8948066390aee621d6afa03f6ecd9/docs/endpoint-recovery.md Change-Id: I5e362cf6e5ce80ce282bab48d912a1038003e236 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Summary: # Description: Update mctp patch for parsing data in InterfaceAdded signal. The mctpd will try to add corresponding board to EID table instead of rescanning whole EM config on every InterfaceAdded signal. The code had been update to CodeConstruct/mctp#17. # Motivation: Rescanning whole EM config on every InterfaceAdded signal cause performance issue. Chenge to parse signal data instead. X-link: https://github.com/facebookexternal/openbmc.wiwynn/pull/3000 Test Plan: Check if mctpd spends less time getting EID on dbus. - pass Reviewed By: wangx6f Differential Revision: D54584520 fbshipit-source-id: 379f17efa52177dd9fc050aeada44b4432be5805

While mctpd[1] may see heavy use in projects such as OpenBMC, it implements generic functionality necessary to operate MCTP as a protocol. It therefore should be easy to use in other contexts, and so it feels unwise to embed OpenBMC-specific details in its implementation. Conversely, entity-manager's scope is to expose inventory and board configuration. It externalises all other responsibilities for the sake of stability and maintenance. While entity-manager is central to OpenBMC's implementation and has little use in other contexts, embedding details of how to configure mctpd in entity-manager exceeds its scope. Thus we reach the design point of mctpreactor, an intermediary process that encapsulates OpenBMC-specific and mctpd-specific behaviors to constrain their dispersion in either direction. The design-point was reached via discussion at [2]. mctpreactor tracks instances of transport-specific MCTP device configurations appearing as a result of inventory changes, and uses them to assign endpoint IDs via mctpd. The lifecycle of an MCTP device can be quite dynamic - mctpd provides behaviors to recover[3] or remove endpoints from the network. Their presence cannot be assumed. mctpreactor handles these events: If a device is removed at the MCTP layer (as it may be unresponsive), mctpreactor will periodically attempt to re-establish it as an endpoint so long as the associated configuration on the entity-manager inventory object remains exposed. [1]: https://github.com/CodeConstruct/mctp/ [2]: CodeConstruct/mctp#17 [3]: https://github.com/CodeConstruct/mctp/blob/7ec2f8daa3a8948066390aee621d6afa03f6ecd9/docs/endpoint-recovery.md Change-Id: I5e362cf6e5ce80ce282bab48d912a1038003e236 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

The mctpd will try to find the configuration interface "xyz.openbmc_project.Configuration.MCTPEndpoint" and parse the data. Then it will send get EID command to target device. If we the get the response, we publish the EID on the D-Bus. Changes: - Scan D-Bus for static EIDs config. - Add a method that rescans entity-manager config for EIDs. - Add matches for monitoring InterfaceAdded and InterfaceRemoved from entity-manger. - Add association definition interface for EIDs that came from entity-manager config. Test log: - reboot and check MCTP D-Bus path root@bmc:~# reboot root@bmc:~# busctl tree xyz.openbmc_project.MCTP `- /xyz `- /xyz/openbmc_project `- /xyz/openbmc_project/mctp `- /xyz/openbmc_project/mctp/1 |- /xyz/openbmc_project/mctp/1/60 `- /xyz/openbmc_project/mctp/1/8 root@bmc:~# pldmtool base GetTID -m 60 { "Response": 134 } Signed-off-by: PeterHo-wiwynn <Peter_MH_Ho@wiwynn.com>

While mctpd[1] may see heavy use in projects such as OpenBMC, it implements generic functionality necessary to operate MCTP as a protocol. It therefore should be easy to use in other contexts, and so it feels unwise to embed OpenBMC-specific details in its implementation. Conversely, entity-manager's scope is to expose inventory and board configuration. It externalises all other responsibilities for the sake of stability and maintenance. While entity-manager is central to OpenBMC's implementation and has little use in other contexts, embedding details of how to configure mctpd in entity-manager exceeds its scope. Thus we reach the design point of mctpreactor, an intermediary process that encapsulates OpenBMC-specific and mctpd-specific behaviors to constrain their dispersion in either direction. The design-point was reached via discussion at [2]. mctpreactor tracks instances of transport-specific MCTP device configurations appearing as a result of inventory changes, and uses them to assign endpoint IDs via mctpd. The lifecycle of an MCTP device can be quite dynamic - mctpd provides behaviors to recover[3] or remove endpoints from the network. Their presence cannot be assumed. mctpreactor handles these events: If a device is removed at the MCTP layer (as it may be unresponsive), mctpreactor will periodically attempt to re-establish it as an endpoint so long as the associated configuration on the entity-manager inventory object remains exposed. [1]: https://github.com/CodeConstruct/mctp/ [2]: CodeConstruct/mctp#17 [3]: https://github.com/CodeConstruct/mctp/blob/7ec2f8daa3a8948066390aee621d6afa03f6ecd9/docs/endpoint-recovery.md Change-Id: I5e362cf6e5ce80ce282bab48d912a1038003e236 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

While mctpd[1] may see heavy use in projects such as OpenBMC, it implements generic functionality necessary to operate MCTP as a protocol. It therefore should be easy to use in other contexts, and so it feels unwise to embed OpenBMC-specific details in its implementation. Conversely, entity-manager's scope is to expose inventory and board configuration. It externalises all other responsibilities for the sake of stability and maintenance. While entity-manager is central to OpenBMC's implementation and has little use in other contexts, embedding details of how to configure mctpd in entity-manager exceeds its scope. Thus we reach the design point of mctpreactor, an intermediary process that encapsulates OpenBMC-specific and mctpd-specific behaviors to constrain their dispersion in either direction. The design-point was reached via discussion at [2]. mctpreactor tracks instances of transport-specific MCTP device configurations[3] appearing as a result of inventory changes, and uses them to assign endpoint IDs via mctpd. The lifecycle of an MCTP device can be quite dynamic - mctpd provides behaviors to recover[4] or remove endpoints from the network. Their presence cannot be assumed. mctpreactor handles these events: If a device is removed at the MCTP layer (as it may be unresponsive), mctpreactor will periodically attempt to re-establish it as an endpoint so long as the associated configuration on the entity-manager inventory object remains exposed. [1]: https://github.com/CodeConstruct/mctp/ [2]: CodeConstruct/mctp#17 [3]: https://gerrit.openbmc.org/c/openbmc/entity-manager/+/70628 [4]: https://github.com/CodeConstruct/mctp/blob/7ec2f8daa3a8948066390aee621d6afa03f6ecd9/docs/endpoint-recovery.md Change-Id: I5e362cf6e5ce80ce282bab48d912a1038003e236 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

jk-ozlabs · 2025-01-06T04:11:17Z

With the mctpreactor changes, it seems like we would no longer need this implementation, as the configuration is handled externally. We would need further work for static EID allocations, but that's now in the EM domain.

So, I'll close this now, but @BonnieLo : let me know if I'm incorrect with the above and we can revisit/reopen.

yikaitsaiww · 2025-01-06T08:16:52Z

Hi @jk-ozlabs,

In our project, we have numerous devices on different I3C buses that use the same I3C address. As a temporary solution, we have been using the SetupEndpointByConfigPath implementation introduced in this PR to address this issue.

As I know, mctpreactor calls AssignEndpoint which cannot support multi devices with the same I3C address. For such specific scenarios, additional handling or configuration might be necessary. Do you have any other suggestions on how we can achieve a similar functionality within entity-manager or other recpies?

Thank you for your time and consideration.

jk-ozlabs · 2025-01-06T08:20:56Z

which cannot support multi devices with the same I3C address

Can you expand on this a little? I assume these are on separate i3c busses, right?

jk-ozlabs · 2025-01-06T08:22:14Z

Ah, you have mentioned that they are separate busses, right.

In which case, AssignEndpoint is fine; it is specific to a MCTP interface device.

yikaitsaiww · 2025-01-06T08:26:36Z

Thank you for your prompt response! We will double-check this and create a new issue if we have any further questions.

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch 2 times, most recently from 155f95b to 729dab7 Compare November 8, 2023 06:43

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch 2 times, most recently from b24373a to 55d015c Compare November 14, 2023 02:37

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch 3 times, most recently from 198de48 to 45156e1 Compare December 1, 2023 08:59

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch 2 times, most recently from eed15c8 to 4fc8bc9 Compare December 15, 2023 07:36

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch from 4fc8bc9 to 30b0b9f Compare December 20, 2023 09:09

jk-ozlabs requested changes Dec 22, 2023

View reviewed changes

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch from 30b0b9f to 1851ecf Compare January 2, 2024 07:26

jk-ozlabs requested changes Jan 2, 2024

View reviewed changes

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch 2 times, most recently from 7b1baed to c59e5fe Compare January 3, 2024 06:42

jk-ozlabs reviewed Jan 3, 2024

View reviewed changes

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch 3 times, most recently from 1261237 to 4571f0c Compare January 4, 2024 05:18

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch from 7a6a164 to 8de7934 Compare February 22, 2024 02:55

PeterHo-wiwynn force-pushed the Peter/static-eid-config branch from 8de7934 to 417e77c Compare April 11, 2024 00:31

yikaitsaiww mentioned this pull request Jan 6, 2025

mctpd: AllocateEndpoints implementation #46

Closed

Conversation

BonnieLo commented Oct 17, 2023

Uh oh!

williamspatrick commented Nov 14, 2023

Uh oh!

jk-ozlabs commented Nov 14, 2023

Uh oh!

PeterHo-wiwynn commented Dec 15, 2023

Uh oh!

jk-ozlabs commented Dec 18, 2023

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ThuBaNguyen commented Jan 4, 2024 •

edited

Loading