Skip to content

mctpd: support discovery requests #16

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

Jump to bottom
Merged
merged 2 commits into from
Aug 25, 2025
Merged

mctpd: support discovery requests #16

merged 2 commits into from
Aug 25, 2025

Conversation

khangng-ampere
Copy link
Contributor

@khangng-ampere khangng-ampere commented Oct 6, 2023 •
edited
Loading

This PR adds support for replying to discovery messages, when mctpd runs on an endpoint:

  • Prepare for Endpoint Discovery
  • Endpoint Discovery
  • Set Endpoint ID (updated to set assigned EID)

@jk-ozlabs
Copy link
Member

All look great, thanks.

We should probably include some top-level configuration on whether mctpd is running as an endpoint vs bus owner (and/or do we need to distingush a TMBO?). Even a command-line argument would be sufficient there.

For example, this would affect the Set Endpoint ID handling; in bus-owner mode, we would reject all Set Endpoint ID commands; rather than being conditional on whether an EID is already assigned. In endpoint mode, it should be totally fine for the bus owner to reassign EIDs.

Is that something you'd like to do? If not, I can implement as a base change.

@khangng-ampere
Copy link
Contributor Author

khangng-ampere commented Jan 22, 2024 •
edited
Loading

@jk-ozlabs Sorry for the late reply. I made a MR to add the BO configuration at #14, if that is what you meant.

@jk-ozlabs
Copy link
Member

@jk-ozlabs Sorry for the late reply. I made a MR to add the BO configuration at #14, if that is what you meant.

Ah! Thank you. I should have started there.

@khangng-ampere khangng-ampere marked this pull request as draft July 28, 2025 05:33
@khangng-ampere khangng-ampere marked this pull request as ready for review July 28, 2025 06:01
@khangng-ampere
Copy link
Contributor Author

I figured it might be a good time to refresh those patches :) Added a basic test case to exercise the new path, and check for Bus Owner as discussed above.

@khangng-ampere khangng-ampere changed the title mctpd: Handle discovery messages mctpd: support discovery requests Jul 28, 2025
@jk-ozlabs
Copy link
Member

How does this relate to #16? should I check that one out first, or this one?

@khangng-ampere
Copy link
Contributor Author

khangng-ampere commented Jul 28, 2025 •
edited
Loading

How does this relate to #16? should I check that one out first, or this one?

@jk-ozlabs Do you mean #96 (set endpoint ID)? This one is based on top of Set Endpoint ID so you should review that one first.

Discovery process contains setting EID, so I think basing this on Set Endpoint ID makes more sense. Set Endpoint ID also works standalone, and not every transport requires discovery

@jk-ozlabs
Copy link
Member

Sorry, yes, #96. OK, will start there.

@jk-ozlabs
Copy link
Member

The DSP0236 implies that we should only support discovery on certain link types:

Note that Discovered flag is only used for some physical transport
bindings. An ERROR_INVALID_DATA completion code shall be returned
if this operation is selected and the particular transport binding does not
support a Discovered flag.

any thoughts on making this selective?

@khangng-ampere
Copy link
Contributor Author

The DSP0236 implies that we should only support discovery on certain link types:

Note that Discovered flag is only used for some physical transport
bindings. An ERROR_INVALID_DATA completion code shall be returned
if this operation is selected and the particular transport binding does not
support a Discovered flag.

any thoughts on making this selective?

I think one option is to make link->discovered tri-state: unsupported, undiscovered and discovered. That helps decouple the state initialization to when we add the link, where we can base on the link medium type to decide if it is supported or not. Other parts of mctpd can check if this flag is supported. What do you think?

@khangng-ampere
Copy link
Contributor Author

I suppose it would look something like this. There is still no link medium check, but at least bus owner mode should not be impacted

@jk-ozlabs
Copy link
Member

I think that looks reasonable; we would just set up the unsupported/undiscovered state depending on link type.

My main concern is not breaking things when we (eventually) use the medium type to detect this as some transports will no longer support discovery at that point. A couple of options:

  • we just do the transport detection now
  • we add this, but all links come up as DISCOVERY_UNSUPPORTED for now (but that may make testing somewhat difficult)
  • we accept the future breakage
  • we only enable this with a config option
  • something else?

@khangng-ampere
Copy link
Contributor Author

I think that looks reasonable; we would just set up the unsupported/undiscovered state depending on link type.

My main concern is not breaking things when we (eventually) use the medium type to detect this as some transports will no longer support discovery at that point. A couple of options:

  • we just do the transport detection now
  • we add this, but all links come up as DISCOVERY_UNSUPPORTED for now (but that may make testing somewhat difficult)
  • we accept the future breakage
  • we only enable this with a config option
  • something else?

I think I should just support the transport detection now, since I will have to do that sooner or later anyway. Let me update the PR

@khangng-ampere
Copy link
Contributor Author

I added support for the physical binding attribute, please have a look. Thanks!

jk-ozlabs
jk-ozlabs reviewed Aug 22, 2025
View reviewed changes
Copy link
Member

@jk-ozlabs jk-ozlabs left a comment

Choose a reason for hiding this comment

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

Couple of minor things inline, but looks good!

@khangng-ampere
netlink: support physical binding attribute
350a374 
Newer Linux kernel supports reporting interface physical binding
attribute via IFLA_MCTP_PHYS_BINDING [1]. The value of this attributes
is defined in DSP0239 [2].

Add support for this attribute.

[1] torvalds/linux@580db51
[2] https://www.dmtf.org/sites/default/files/standards/documents/DSP0239_1.11.1.pdf

Signed-off-by: Khang D Nguyen <[email protected]>
jk-ozlabs
jk-ozlabs reviewed Aug 25, 2025
View reviewed changes
tests/test_mctpd_endpoint.py Outdated
@@ -18,7 +18,7 @@ def config():
@pytest.fixture
async def sysnet():
system = System()
iface = System.Interface("mctp0", 1, 1, bytes([0x1D]), 68, 254, True)
iface = System.Interface("mctp0", 1, 1, bytes([0x1D]), 68, 254, True, PhysicalBinding.PCIE_VDM)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little wary of making this the default, as there is no upstream support for the PCIe binding type - we want to make sure that if there are any cases we might hit with common scenarios, that we are likely to cover those in tests.

Is it possible to set this in the test cases where required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is reasonable yeah. I scoped those test in classes and used pytest fixtures to override the interface binding types

Copy link
Member

Choose a reason for hiding this comment

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

nice!

@khangng-ampere
mctpd: support discovery requests
67f8f16 
This adds support for the Discovered flag and discovery requests,
as described in DSP0236
12.14 Prepare for Endpoint Discovery and
12.15 Endpoint Discovery.

Signed-off-by: Khang D Nguyen <[email protected]>
@jk-ozlabs jk-ozlabs merged commit 6744e5d into CodeConstruct:main Aug 25, 2025
3 checks passed
@jk-ozlabs
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jk-ozlabs jk-ozlabs jk-ozlabs left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@khangng-ampere @jk-ozlabs