Re: PCI VPD checksum ambiguity

[Heiner Kallweit <hkallweit1@gmail.com>]

On 01.04.2025 22:55, Bjorn Helgaas wrote:
> Hi,
> 
> The PCIe spec is ambiguous about how the VPD checksum should be
> computed, and resolving this ambiguity might break drivers.
> 
> PCIe r6.0 sec 6.27 says only the VPD-R list should be included in the
> checksum:
> 
>   One VPD-R (10h) tag is used as a header for the read-only keywords.
>   The VPD-R list (including tag and length) must checksum to zero.
> 

This requirement I don't find in the PCI 3.0 spec, not sure with which
version it was added. Interestingly this doesn't even require the
presence of a RV keyword. Or the presence of the RV keyword is
implicitly assumed.

Maybe this part isn't meant literally. I can imagine they wanted to
clarify that checksum calculation excludes the VPD-W area.
And unfortunately they weren't precise enough, and introduced the
ambiguity you found.


> But sec 6.27.2.2 says "all bytes in VPD ... up to the checksum byte":
> 
>   RV   The first byte of this item is a checksum byte. The checksum is
>        correct if the sum of all bytes in VPD (from VPD address 0 up
>        to and including this byte) is zero.
> 

This one can be found identically in the PCI v3.0 spec already:

The checksum is correct if the sum of all bytes in VPD (from
VPD address 0 up to and including this byte) is zero.

I don't think they want to break backwards-compatibility, therefore
this requirement should still be valid.

> These are obviously different unless VPD-R happens to be the first
> item in VPD.  But sec 6.27 and 6.27.2.1 suggest that the Identifier
> String item should be the first item, preceding the VPD-R list:
> 
>   The first VPD tag is the Identifier String (02h) and provides the
>   product name of the device. [6.27]
> 
>   Large resource type Identifier String (02h)
> 
>     This tag is the first item in the VPD storage component. It
>     contains the name of the add-in card in alphanumeric characters.
>     [6.27.2.1, Table 6-23]
> 
> I think pci_vpd_check_csum() follows sec 6.27.2.2: it sums all the
> bytes in the buffer up to and including the checksum byte of the RV
> keyword.  The range starts at 0, not at the beginning of the VPD-R
> read-only list, so it likely includes the Identifier String.
> 
> As far as I can tell, only the broadcom/tg3 and chelsio/cxgb4/t4
> drivers use pci_vpd_check_csum().  Of course, other drivers might
> compute the checksum themselves.
> 
> Any thoughts on how this spec ambiguity should be resolved?
> 
> Any idea how devices in the field populate their VPD?
> 
> Can you share any VPD dumps from devices that include an RV keyword
> item?
> 

I have only very dated devices which likely date back to before
the existence of PCIe r6.0. So their VPD dump may not really help.

IIRC there's an ongoing discussion regarding making VPD content
user-readable on mlx5 devices. Maybe check with the Mellanox/Nvidia
guys how they interpret the spec and implemented VPD checksumming.

> Bjorn