mctpd: Add VendorDefinedMessageTypes property by msnidhin · Pull Request #136 · CodeConstruct/mctp

msnidhin · 2026-01-15T07:14:24Z

Implement Get VDM Support (0x06) control command to query vendor defined message capabilities from peers. Expose via D-Bus property a(yvu) containing format, vendor_id (PCIe 16-bit or IANA 32-bit), and command set. Includes positive test for both formats and negative tests for invalid responses (wrong lengths, invalid format, unsupported command).

jk-ozlabs

Nice! This looks good, just a few changes/comments inline.

jk-ozlabs · 2026-01-15T23:44:30Z

src/mctpd.c

+			goto out;
+
+		resp = (void *)buf;
+		if (resp->vendor_id_format !=


Minor: Since we re-use resp->vendor_id_format a few times, and the length causes a lot of wrapping, I'd suggest using a temporary (say, fmt) for this.

jk-ozlabs · 2026-01-15T23:48:38Z

src/mctpd.c

+			goto out;
+
+		/* Check for minimum length of PCIe VDM*/
+		expect_size = sizeof(*resp);


This is a bit fragile given the somewhat quirky definition of struct mctp_ctrl_resp_get_vdm_support. We're really including spce for the IANA-format response (because the union will be the max size possible), but then relying on the later fields being absent from the struct, which coincidentally aligns with the correct size for a PCIe format.

We should fix that definition to not include format-specific data at all. In which case, a fixed value here might be more appropriate.

jk-ozlabs · 2026-01-15T23:56:08Z

src/mctpd.c

+		peer->num_vdm_types++;
+
+		/* Use the next selector from the response. 0xFF indicates no more entries */
+		req.vendor_id_set_selector = resp->vendor_id_set_selector;


Might be cleaner to do the 0xff check here, and rc = 0; break if it matches. Then your while loop can be unconditional, and the error cases can just break, rather than needing a goto between scopes.

jk-ozlabs · 2026-01-15T23:57:37Z

src/mctpd.c

+	free(buf);
+	if (rc < 0) {
+		free(peer->vdm_types);
+		peer->vdm_types = NULL;


May be better to store these temporarily, and only update the peer-> members on success?

jk-ozlabs · 2026-01-15T23:59:03Z

src/mctpd.c

+		}
+	}
+
+	for (unsigned int i = 0; supports_vdm && i < max_retries; i++) {


The intent would be more obvious with supports_vdm as an explicit conditional, rather than hiding it in the loop conditional.

jk-ozlabs · 2026-01-15T23:59:49Z

src/mctpd.c

+			"a(yvu)",
+			bus_endpoint_get_prop,
+			0,
+			SD_BUS_VTABLE_PROPERTY_CONST),


Please add to the dbus API spec (in docs/mctpd.md) too.

Hi @jk-ozlabs
I see only

These objects host the interface xyz.openbmc_project.MCTP.Endpoint, as per [OpenBMC documentation](https://github.com/openbmc/phosphor-dbus-interfaces/tree/master/yaml/xyz/openbmc_project/MCTP).

in docs/mctpd.md

Do I need to add only this new property ?

Ah, true. Don't worry about this then, I'll add docs for all properties in a separate change.

jk-ozlabs · 2026-01-16T00:01:10Z

tests/test_mctpd.py

+    iface = mctpd.system.interfaces[0]
+    mctp = await mctpd_mctp_iface_obj(dbus, iface)
+
+    # Test 1: Invalid PCIe length


If these are different tests, please make them separate test functions. You can share the endpoint classes if that makes things easier.

jk-ozlabs · 2026-01-16T00:02:41Z

tests/test_mctpd.py

+""" Test that VendorDefinedMessageTypes property is queried and populated correctly """
+async def test_query_vdm_types(dbus, mctpd):
+    class VDMEndpoint(Endpoint):
+        async def handle_mctp_control(self, sock, addr, data):


This could go in the main Endpoint object, like the general type support, using a vdm_msg_types member as the data source.

msnidhin · 2026-01-18T05:38:15Z

Waiting for https://gerrit.openbmc.org/c/openbmc/phosphor-dbus-interfaces/+/86652 to finalize

jk-ozlabs

Looking good for the new spec, but a couple of things inline.

jk-ozlabs · 2026-02-19T08:05:18Z

tests/test_mctpd.py

    assert ep_types == query_types


+""" Test that VendorDefinedMessageTypes property is queried and populated correctly """


Docstrings should go inside the function definition, as the first line.

Please check out the spacing requirements in PEP257 too - no space immediately within the quotes, and """ on its own line for multi-line docstrings.

(this is all part of the new checks, which had landed since your last push)

jk-ozlabs · 2026-02-19T08:06:21Z

tests/test_mctpd.py

+    assert len(vdm_types) == 0
+
+
+""" Test VDM query with invalid PCIe length """


We can probably deduce this from the function name. If you add a docstring, make it a little more explanatory (or drop if not needed)

jk-ozlabs · 2026-02-19T08:07:40Z

tests/mctpenv/__init__.py

+                    resp = resp + list(cur_vdm[1].to_bytes(4, 'big'))
+                else:
+                    await sock.send(raddr, bytes(hdr + [0x02]))
+                    return


I'd suggest handing this error case before constructing the response.

However, a value other than 0 or 1 would indicate an issue in the test suite, no?

jk-ozlabs · 2026-02-19T08:10:15Z

src/mctpd.c

+			if (rc < 0)
+				return rc;
+
+			rc = sd_bus_message_append(reply, "u",


The new spec defines this as q, no?

jk-ozlabs · 2026-02-19T08:13:28Z

src/mctpd.c

 			0,
 			SD_BUS_VTABLE_PROPERTY_CONST),
+	SD_BUS_PROPERTY("VendorDefinedMessageTypes",
+			"a(yvu)",


jk-ozlabs · 2026-02-19T08:16:22Z

src/mctpd.c

+	if (supports_vdm) {
+		for (unsigned int i = 0; i < max_retries; i++) {
+			rc = query_get_peer_vdm_types(peer);
+			// Success


Probably don't need this comment.

jk-ozlabs · 2026-02-19T08:20:48Z

src/mctpd.c

-				htobe16(cur_vdm->vendor_id.pcie);
+			vdm_pcie = (void *)(resp + 1);
+			resp_len = sizeof(*resp) +
+				   sizeof(struct mctp_vdm_pcie_data);


Does this need wrapping? there seems to be a bit of short-wrapping in this patch.

/usr/bin/clang-format --dry-run --Werror --color=1 -style=file -i /home/runner/work/mctp/mctp/src/mctpd.c
/home/runner/work/mctp/mctp/src/mctpd.c:1112:30: error: code should be clang-formatted [-Wclang-format-violations]
resp_len = sizeof(*resp) + sizeof(struct mctp_vdm_pcie_data);
^
FAILED: [code=1] meson-internal__clang-format-check

formatter gives wrapped line

jk-ozlabs · 2026-02-19T08:22:38Z

src/mctpd.c

+			break;
+
+		/* Check for minimum length of PCIe VDM*/
+		expect_size = sizeof(*resp) + sizeof(struct mctp_vdm_pcie_data);


Maybe just check for the base here, then apply the full expect_size check in the same place you are doing for PCIe (and that will allow you to handle the !PCIE_FORMAT && !IANA_FORMAT as an else-case there)

Implement Get VDM Support (0x06) control command to query vendor defined message capabilities from peers. Expose via D-Bus property a(yvq) containing format, vendor_id (PCIe 16-bit or IANA 32-bit), and command set. Includes positive test for both formats and negative tests for invalid responses (wrong lengths, invalid format, unsupported command). Signed-off-by: Nidhin MS <nidhin.ms@intel.com>

msnidhin force-pushed the vdm_types branch 3 times, most recently from fa05dfb to dad0d6c Compare January 15, 2026 07:51

jk-ozlabs requested changes Jan 16, 2026

View reviewed changes

msnidhin force-pushed the vdm_types branch 3 times, most recently from 171d7fd to a964b55 Compare February 19, 2026 06:13

jk-ozlabs requested changes Feb 19, 2026

View reviewed changes

msnidhin force-pushed the vdm_types branch 2 times, most recently from a7e5ece to cb182be Compare February 20, 2026 04:55

msnidhin requested a review from jk-ozlabs February 20, 2026 04:57

		assert ep_types == query_types


		""" Test that VendorDefinedMessageTypes property is queried and populated correctly """

		assert len(vdm_types) == 0


		""" Test VDM query with invalid PCIe length """

Conversation

msnidhin commented Jan 15, 2026

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!

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!

msnidhin commented Jan 18, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments