Message-ID: <fc6aa01e-d35e-449d-a752-00f91491f16a@amperemail.onmicrosoft.com>
Date: Thu, 18 Dec 2025 14:31:09 -0500
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: Adam Young <admiyo@...amperecomputing.com>,
 Jassi Brar <jassisinghbrar@...il.com>, "Rafael J. Wysocki"
 <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Robert Moore <robert.moore@...el.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Jeremy Kerr <jk@...econstruct.com.au>,
 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>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing
 PCC extended data


On 10/24/25 09:50, Sudeep Holla wrote:
> On Tue, Oct 21, 2025 at 01:20:50PM -0400, Adam Young wrote:
>
> [...]
>
>>>> Because the PCC spec is wonky.
>>>> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#extended-pcc-subspace-shared-memory-region
>>>>
>>>> "Length of payload being transmitted including command field."  Thus in
>>>> order to copy all of the data, including  the PCC header, I need to drop the
>>>> length (- sizeof(u32) ) and then add the entire header. Having all the PCC
>>>> data in the buffer allows us to see it in networking tools. It is also
>>>> parallel with how the messages are sent, where the PCC header is written by
>>>> the driver and then the whole message is mem-copies in one io/read or write.
>>>>
>>> No you have misread this part.
>>> Communication subspace(only part and last entry in shared memory at offset of
>>> 16 bytes) - "Memory region for reading/writing PCC data. The maximum size of
>>> this region is 16 bytes smaller than the size of the shared memory region
>>> (specified in the Master slave Communications Subspace structure). When a
>>> command is sent to or received from the platform, the size of the data in
>>> this space will be Length (expressed above) minus the 4 bytes taken up by
>>> the command."
>>>
>>> The keyword is "this space/region" which refers to only the communication
>>> subspace which is at offset 16 bytes in the shmem.
>>>
>>> It should be just length - sizeof(command) i.e. length - 4
>>
>> I just want to make sure I have this correct.  I want to copy the entire PCC
>> buffer, not just the payload, into the sk_buff.  If I wanted the payload, I
>> would use the length field.  However, I want the PCC header as well, which
>> is the length field, plus sizeof (header).  But that double counts the
>> command field, which is part of the header, and thus I subtract this out.  I
>> think my math is correct. What you wrote would be for the case where I want
>> only the PCC payload.
>>
> Why ? How does sk_buff interpret that as PCC header. Something doesn't align
> well here or I might be missing something.
>
> I started writing some helpers and this comment made me to rethink my
> approach. I don't have any to share and I will be away for a while. I will
> try to review any further changes from you but expect delays.

The sk_buff and socket api allows you to specify the various levels of 
headers for a nested protocol.  Just like udp inside IP inside Ethernet 
is a thing, the packets here are mctp inside pcc. The the networking 
stack can look into the packet and pull out the MCTP information when 
routing the packet is routed to the link device.

Since the mctp over pcc driver is PCC specific, I want to be able to see 
the PCC header in a tool like wireshark.  If we only return the MCTP 
portion, we will lose some tracing information. Admittedly, not a lot.

This is fairly well tested in my code base:  I can send and receive MCTP 
messages through the Linux kernel network stack.

The header has the flags, length, and command blocks.  I think we agree 
that PCC mailbox should not hardcode  the command.  The question, then 
is if the PCC layer should be responsible for the signature and header.  
The alternative is that the read/write functions will handle the PCC 
header information.  I think the only real drawback to this would be if 
the driver wanted to be able to set the flags.  For now the only flag 
that is defined is PCC_CMD_COMPLETION_NOTIFY, but this is a driver 
specific decision, and needs  to be accounted for.

So I would like to keep the signatures of the read/write functions as 
is.  We can inline them if you think that there is benefit to it:  as I 
see it, it exposes more internals but reduces the number of external 
symbols for the PCC Mailbox.  This is your call as the maintainer, and I 
can make it work either way.  I would like to submit an updated driver.

Powered by blists - more mailing lists