Re-implement PCI VPD fetch

[se]

Summary

The format of the PCI/PCIe Vital Product Data has been standardized in the PCI-2.2 standard. This format is quite strict and easy to parse.

The parser currently used in the PCI driver is based on a state machine, with tags and parsed data controlling the state changes.
The flexibility of this parser is actually not required. It can lead to the acceptance of non-conformant data as valid VPD, unless restrictions are applied on the permitted sequence of states. It does not detect issues like multiple ident tags, R/O data following the checksum, multiple or unordered R/O and R/W tags, etc.

The reason for the rewrite was a panic reported due to bogus/invalid VPD data provided by an Intel X520 LAN adapter.
Analysis of the VPD code to make it more robust lead me to believe that it was easier to write a "narrow" parser than to restrict the flexible state machine to detect and reject non-well-formed data. A number of restrictions have already been added, but they make the state machine ever more complex and harder to understand.

This patch provides a rewrite of the VPD parser based on the valid structure of the VPD data (ident, R/O data, optional R/W data, end tag). It has been verified to return identical parsed data as the current implementation for the example VPD data in the PCI standard.
It is strict in the sense that it detects and rejects any deviation from a well-formed VPD structure.

The dummy test data contained in this patch is only meant to ease review and verification of the patch.

This version of the patch is not meant to be committed. If it is accepted I'll clean-up the patch to remove the dummy code and a few commented out debug printf()s, for example. The diagnostics could be further improved, but they do already catch more cases than the currently committed code does.

Test Plan

Apply the patch and verify that the correct (identical to the example in the standard) parsed data is returned.
(The dummy data is parsed for each device that has a VPD capability. Many devices have this capability, but do not actually contain VPD).
If one or multiple of INV_CKSUM, INV_RO_LEN, INV_RW_LEN are defined, errors are injected into the dummy data to test the behavior for these cases.

A VPD parser wrapped into a user-land program can be made available for testing, if required.

[se]

Update patch with comments added to all new functions, stricter error checking and removal of the dummy test data generator.

This patch is meant to replace the VPD parser in -CURRENT.
It implements exactly the same interface as the previous version.

Directly passing the data in the format as copied out by pci_list_vpd() to the user-land could probably significantly simplify that function without ABI issues (since the current API and ABI could also be preserved for drivers relying on it).

[daveb_spectralogic.com]

I have applied the patch to Stable 13.1 and tested on several systems that previously demonstrated https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=272018
The VPD is correctly and consistently displayed via pciconf -lV <pci addr> with this patch.

Although I am not conversant in PCI VPD details, the code is much easier to follow with the applied fix.

[kib]

sys/dev/pci/pci.c

1162

1169

Is it more logical to assert that size > 0?

1172

The check is not needed

1224

alloc_buffer cannot fail

[se]

sys/dev/pci/pci.c

1169

I'll have to re-read the description of the VPD format this patch was based on.
I do not remember whether zero-length values are allowed.
The code under review would deal with them in a reasonable way.

1172

Fixed in my sources.

1224

Fixed in my sources.

[se]

A new patch for -CURRENT that addresses all review comments has been uploaded to PR 272018.

sys/dev/pci/pci.c

1162

Fixed in my sources

1169

A value of 0 might legitimately occur, e.g. if a writable resource has been set to an empty resource string. This should not invalidate the VPD. Another example is a "filler" at the end of the writable data, which might just fit with 0 bytes of data in addition to the header.

Since all callers set the size parameter to an unsigned value between 0 and 255 (for resources) or 0 and 65535 (for the ident string), I have removed the test in my sources.

Testing for at most 255 bytes could catch cases of a mis-detected overly long ident , string (which can be assumed to be no longer than 30 or 40 bytes), but testing for <0 is not necessary.

[se]

Updated patch against 13-STABLE that addresses all review comments.

Two unnecessary NULL checks have been removed, a test of the size parameter has been removed since the calling sites limit the possible values, and one style issue in a function declaration has been fixed.

No functional change is intended. The check for size <= 255 might detect unreasonable length values of the ident string (which technically can be up to 65535 bytes long, but in practice will be no longer than 30 or 40 bytes to be fully displayed in fields of limited width).