Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

Adam Young
2025-12-18T19:31:19Z

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.