Hi Daniel, > This new XML format looks good to me now > > I've done a high level review and only major issue i've seen is the > memory leaks mentioned on the first patch. Thanks a lot! I'll address those and send out a v7. Best Regards, Dmitrii Shcherbakov LP/oftc: dmitriis > > On Mon, Oct 11, 2021 at 05:04:41PM +0300, Dmitrii Shcherbakov wrote: > > Add support for deserializing the binary PCI/PCIe VPD format and > > exposing VPD resources as XML elements in a new nested capability > > of PCI/PCIe devices called 'vpd'. > > > > The series contains the following incremental changes: > > > > * The PCI VPD parser module, in-memory resource representation > > and tests; > > * VPD-related helpers added to the virpci module; > > * VPD capability support: XML serialization/deserialization from/into > > VPD resource data structures; > > * Documentation. > > > > The VPD format is specified in "I.3. VPD Definitions" in PCI specs > > (2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0 > > notes, the PCI Local Bus and PCIe VPD formats are binary compatible > > and PCIe 4.0 merely started incorporating what was already present in > > PCI specs. > > > > Linux kernel exposes a binary blob in the VPD format via sysfs since > > v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires > > a parser to interpret. > > > > There are usage scenarios where information such as the board serial > > number needs to be retrieved from PCI(e) VPD. Projects like Nova can > > utilize this information for cases which involve virtual interface > > plugging on SmartNIC DPUs but there may be other scenarios and types of > > information useful to retrieve from VPD. The fact that the format is > > binary requires proper parsing instead of substring searching hence the > > full parser is proposed. Likewise, checksum validation requires proper > > parsing as well. > > > > A usage example is present here: > > https://review.opendev.org/c/openstack/nova/+/808199 > > > > The patch follows a prior discussion on the mailing list which has > > additional context about the use-case but a narrower proposal: > > > > https://listman.redhat.com/archives/libvir-list/2021-May/msg00873.html > > https://www.mail-archive.com/libvir-list@xxxxxxxxxx/msg218165.html > > > > The new functionality is mostly contained in virpcivpd with a > > couple of new functions added to virpci. Additionally, the necessary XML > > serialization/deserialization and glue code is added to expose the VPD > > capability to external clients as XML. > > > > A new capability flag is added along with a new capability in order to > > allow for filtering of PCI devices with the VPD capability using virsh: > > > > virsh nodedev-list --cap vpd > > sudo virsh nodedev-dumpxml --device pci_dddd_bb_ss_f > > > > In this example having the root uid is required in order to access the > > vpd sysfs entry, therefore, the nodedev XML output will only contain > > the VPD capability if virsh is run as root. > > > > The capability is treated as dynamic due to the presence of read-write > > sections in the VPD format per PCI/PCIe specs (the idea being that > > read-write resource fields may potentially be altered by the DPU OS > > over time independently from the host OS). > > > > Unit tests cover the parser functionality (including many possible > > invalid cases), in-memory representation as well as XML serialization > > and deserialization. > > > > Manual functional testing was performed with 2 DPUs and several other > > NIC models which expose PCI(e) VPD. Testing have also been performed > > for devices that do not have VPD or those that expose a VPD capability > > but exhibit invalid behavior (I/O errors while reading a sysfs entry). > > > > v6 changes: > > > > * Reworked in-memory data structures according to the feedback from > > Daniel: virPCIVPDResource is now a struct with nested structures for > > read-only and read-write parts of the VPD. Custom fields (vendor, > > system are now stored in GPtrArray instances; > > * Reworked XML element naming: well-known fields from the spec now have > > human-readable names. Legacy PICMIG fields and binary fields are > > omitted as before; > > This new XML format looks good to me now > > I've done a high level review and only major issue i've seen is the > memory leaks mentioned on the first patch. > > > 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 :| >
