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@redhat.com/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 :|
>