mctpd: support discovery requests

[khangng-ampere]

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]

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]

@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]

@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]

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.

[jk-ozlabs]

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

[khangng-ampere]

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]

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

[jk-ozlabs]

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]

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]

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]

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]

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]

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

[jk-ozlabs]

Couple of minor things inline, but looks good!

[jk-ozlabs]

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?

[khangng-ampere]

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

[jk-ozlabs]

nice!

[jk-ozlabs]

Thanks!