Message-ID: <c69f83fa-a4e2-48fc-8c1a-553724828d70@amperemail.onmicrosoft.com>
Date: Fri, 1 Nov 2024 17:19:59 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Jeremy Kerr <jk@...econstruct.com.au>, admiyo@...amperecomputing.com,
 Matt Johnston <matt@...econstruct.com.au>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Sudeep Holla <sudeep.holla@....com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH v6 2/2] mctp pcc: Implement MCTP over PCC Transport


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

Powered by blists - more mailing lists