Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data
From: Adam Young
Date: Thu Dec 18 2025 - 14:31:19 EST
On 10/24/25 09:50, Sudeep Holla wrote:
On Tue, Oct 21, 2025 at 01:20:50PM -0400, Adam Young wrote:
[...]
Why ? How does sk_buff interpret that as PCC header. Something doesn't alignBecause the PCC spec is wonky.No you have misread this part.
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.
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.
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.