Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport

From: Adam Young
Date: Fri Nov 01 2024 - 17:20:53 EST



On 11/1/24 04:55, Jeremy Kerr wrote:
[EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.]


Hi Adam,

Thanks for the quick response. I think the dev lladdr is the main thing
to work out here, and it's not something we can change that post-merge.
I'm not yet convinced on your current approach, but a few
comments/queries that may help us get a consensus there, one way or the
other:

Thanks so much for your review, and yes, this is one part of the code that commits us to a course of action, as it defines the userland  interface.


We need a hardware address to create a socket without an EID in order
to know where we are sending the packets.
Just to clarify that: for physical (ie, null-EID) addressing, you don't
need the hardware address, you need:

1) the outgoing interface's ifindex; and
2) the hardware address of the *remote* endpoint, in whatever
format is appropriate for link type

In cases where there is no hardware addressing in the tx packet (which
looks to apply to PCC), (2) is empty.

I understand that you're needing some mechanism for finding the correct
ifindex, but I don't think using the device lladdr is the correct
approach.

We have this model already for mctp-over-serial, which is another
point-to-point link type. MCTP-over-serial devices have no hardware
address, as there is no hardware addressing in the packet format. In
EID-less routing, it's up to the application to determine the ifindex,
using whatever existing device-identification mechanism is suitable.

I'd like to avoid having a custom mechanism to find the right interface.  Agreed that this is really find 1) above: selecting the outgoing interface.  There is already an example of using the HW address in the interface:  the loopback has an address in it, for some reason.  Probably because it is inherited from the Ethernet loopback.


The Hardware addressing is needed to be able to use the device in
point-to-point mode. It is possible to have multiple devices at the
hardware level, and also to not use EID based routing. Thus, the
kernel needs to expose which device is which.
Yes, that's totally fine to expect find a specific device - but the
device's hardware address is not the conventional place to do that.

True.  Typically, the hardware interface is linked to a physical device, and the operator know a-priori which network is plugged into that device.  Really, device selection is a collection of heuristics.

In our use case, we expect there to be two MCTP-PCC links available on a 2 Socket System, one per socket.  The end user needs a way to know which device talks to which socket.  In the case of a single socket system, there should only be one.

However, there is no telling how this mechanism will be used in the future, and there may be MCTP-PCC enabled devices that are not bound to a CPU.


The Essential piece of information is the outbox, which identifies
which channel the message will be sent on. The inbox is in the
hardware address as well as a confirmation of on which channel the
messages are expected to return.
Those are the indices of the shared memory regions used for the
transfer, right?

Correct.  And, strictly speaking, only the outbox index is in the message, but it is in there.

Technically we get the signature field in the first four bytes of the PCC Generic Comunications channel Shared memory region:

https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#generic-communications-channel-shared-memory-region

"The PCC signature. The signature of a subspace is computed by a bitwise-or of the value 0x50434300 with the subspace ID. For example, subspace 3 has the signature 0x50434303."

So we could use that.  And it is sufficient to let the receiver know which channel sent the message, if there are  multiples:


In the future, it is possible to reuse channels and IRQs in
constrained situations, and the driver would then be able to deduce
from the packet which remote device sent it.
The spec mentions:

A single PCC instance shall serve as a communication channel
between at most two MCTP capable entities

so how is it ambiguous which remote device has sent a packet? Am I
misinterpreting "channel" there?

Yes, the spec does  say that a single channel is only ever used for a single pair of communicators.  However, we have seen cases where interrupts  are used for more than just a single channel, and thus I don't want to assume that it will stay that way for ever:  pub-sub mechanisms are fairly common.  Thus, this address does not tell the receiver where it came from, and, more importantly where to send responses to.  Hence a push for a better addressing scheme.  There really is no reason a single channel cannot be used by multiple publishers.

In which case, how does the driver RX path do that deduction? There is
no hardware addressing information in the DSP0292 packet format.

Instead, there is a portion of it on outgoing packets, and a
different portion on incoming packets.

The hardware address format is in an upcoming version of the spec not
yet published.
I can't really comment on non-published specs, but that looks more like
identifiers for the tx/rx channel pair (ie., maps to a device
identifier) rather than physical packet addressing data (ie., an
interface lladdr). Happy to be corrected on that though!

In this case,  they really are the same thing:  the index of the channel is  used  to look up the rest of the information.  And the index of the outbox is  the address to send  the packet to, the index of the inbox is where the packet will be received.

One possibility is to do another revision that uses  the SIGNATURE as the HW address, with an understanding that if the signature changes, there will be a corresponding change in the HW address, and thus userland and kernel space will be kept in sync. This is an ugly format. The format suggested above is easier to parse and work with.



Cheers,


Jeremy