[libvirt PATCH 1/4] PCI VPD: handle additional edge cases

[Daniel P. Berrangé]

On Tue, Oct 26, 2021 at 08:28:20PM +0300, Dmitrii Shcherbakov wrote:
> Hi Daniel,
> 
> > Something is still not right with the logic here as this error report
> is triggering on my machines. If I comment out this check, then I can
> get data reported by libvirt
> 
> The VPD example you shared previously has multiple violations of the VPD format:
> 
> * Invalid field values ("N/A" in the read-only section, 0xFF in the
> read-write section);
> * The lack of the RW field at the end of the read-write section.
> 
> Previously, the VPD was discarded due to invalid values.
> 
> I thought honoring the RW requirement would still make sense and kept
> the RW presence check.
> 
> I added it as a test case here (converted to hex):
> https://listman.redhat.com/archives/libvir-list/2021-October/msg00871.html
> 
> The spec says the following (PCIe 4.0 "6.28.2.3 Read/Write Fields"):
> 
> "RW | Remaining Read/Write Area |
> This descriptor is used to identify the unused portion of the
> read/write space. The product
> vendor initializes this parameter based on the size of the read/write
> space or the space remaining
> following the Vx VPD items. One or more of the Vx, Yx, and RW items
> are required."
> 
> I can also confirm that by looking at the hexdump and looking for hex
> values for R (0x52) and W (0x57):
> 
> hexdump -e '16/1 "0x%02x, " "\n"' vpd
> 
> I could ease it up and allow it to be missing as we do not care much
> about it when reading so long as the resource
> length is correct. However, if we were to allow Libvirt to write to
> the read-write section in the future we would need to
> know how much free space is there in the first place.

The issue I have is that 'lspci -vvv' will happily report the VPD
data on my machine:

        Capabilities: [e0] Vital Product Data
                Product Name: HP Ethernet 1Gb 2-port 361i Adapter
                Read-only fields:
                        [PN] Part number: N/A
                        [EC] Engineering changes: N/A
                        [SN] Serial number: N/A
                        [V0] Vendor specific: 4W/1W PCIeG2x4 2p 1GbE RJ45 Intel i350   
                        [RV] Reserved: checksum good, 0 byte(s) reserved
                Read/write fields:
                        [V1] Vendor specific: 5.7.06
                        [V3] Vendor specific: 2.8.20
                        [V6] Vendor specific: 1.5.35
                        [YA] Asset tag: N/A
                        [YB] System specific: <FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF>
                        [YC] System specific: <FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF><FF>
                End


while libvirt refuses to report it. Even if the BIOS is not perfectly
following the spec, it is clearly still possible to extract the data
and display it to the user. So I don't think it is reasonable for libvirt
to refuse todo this.  I'd have sympathy if this machine was pre-production
hardware or from a no-name vendor, but this machine is typical production
quality hardware from a major vendor.

The only think there I think it is reasonable for libvirt to reject the
data is in the cae of the two fields that contain non-printable
characters <FF>.  Other than that I expect libvirt to report exactly
the same data as lspci will report, and not try to secondguess whether
the values provided by the vendor are semantically valid or not.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|