[libvirt PATCH v6 0/5] Add a PCI/PCIe device VPD Capability

Dmitrii Shcherbakov posted 5 patches 2 years, 5 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20211011140446.220390-1-dmitrii.shcherbakov@canonical.com
There is a newer version of this series
NEWS.rst                                      |  22 +
build-aux/syntax-check.mk                     |   4 +-
docs/drvnodedev.html.in                       |  69 ++
docs/formatnode.html.in                       |  63 +-
docs/schemas/nodedev.rng                      |  96 +++
include/libvirt/libvirt-nodedev.h             |   1 +
po/POTFILES.in                                |   1 +
src/conf/node_device_conf.c                   | 293 +++++++
src/conf/node_device_conf.h                   |   7 +-
src/conf/virnodedeviceobj.c                   |   7 +-
src/libvirt_private.syms                      |  20 +
src/node_device/node_device_driver.c          |   2 +
src/node_device/node_device_udev.c            |   2 +
src/util/meson.build                          |   1 +
src/util/virpci.c                             |  70 ++
src/util/virpci.h                             |   4 +
src/util/virpcivpd.c                          | 743 +++++++++++++++++
src/util/virpcivpd.h                          | 111 +++
src/util/virpcivpdpriv.h                      |  48 ++
tests/meson.build                             |   1 +
.../pci_0000_42_00_0_vpd.xml                  |  42 +
.../pci_0000_42_00_0_vpd.xml                  |   1 +
tests/nodedevxml2xmltest.c                    |   1 +
tests/testutils.c                             |  35 +
tests/testutils.h                             |   4 +
tests/virpcimock.c                            |  30 +
tests/virpcitest.c                            |  39 +
tests/virpcivpdtest.c                         | 768 ++++++++++++++++++
tools/virsh-nodedev.c                         |   3 +
29 files changed, 2483 insertions(+), 5 deletions(-)
create mode 100644 src/util/virpcivpd.c
create mode 100644 src/util/virpcivpd.h
create mode 100644 src/util/virpcivpdpriv.h
create mode 100644 tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml
create mode 120000 tests/nodedevxml2xmlout/pci_0000_42_00_0_vpd.xml
create mode 100644 tests/virpcivpdtest.c
[libvirt PATCH v6 0/5] Add a PCI/PCIe device VPD Capability
Posted by Dmitrii Shcherbakov 2 years, 5 months ago
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;
* The fieldValueFormats hash table is now initialized in a thread-safe
  manner using VIR_ONCE_GLOBAL_INIT. The values are still duplicated
	since the GHashTable requires non-const pointers;
* VPD field value validation now uses a simpler character set validation
  instead of Glib-provided regular expressions as was suggested in
	comments to v5;
* virPCIVPDParse no longer attempts to close the fd passed to it and
  leaves that for the caller to do;
* Extended XML schema testing with RW section validation and reworked
  the RNG schema to account for the new XML formatting structure;
* Improved test coverage in multiple places;
* Refactoring: replaced gchar and gboolean usage with char and bool per
  the mailing list discussion.

Build & test results for targets in ci/manifest.yaml:

ci/helper test --meson-args='-Dexpensive_tests=enabled' <target>

https://gist.github.com/dshcherb/867d87a7419b91bb68388518ca5a9c32

Dmitrii Shcherbakov (5):
  Add a PCI/PCIe device VPD Parser
  Add PCI VPD-related helper functions to virpci
  Add PCI VPD Capability Support
  Add PCI VPD Capability Documentation
  news: Add PCI VPD parser & capability notes

 NEWS.rst                                      |  22 +
 build-aux/syntax-check.mk                     |   4 +-
 docs/drvnodedev.html.in                       |  69 ++
 docs/formatnode.html.in                       |  63 +-
 docs/schemas/nodedev.rng                      |  96 +++
 include/libvirt/libvirt-nodedev.h             |   1 +
 po/POTFILES.in                                |   1 +
 src/conf/node_device_conf.c                   | 293 +++++++
 src/conf/node_device_conf.h                   |   7 +-
 src/conf/virnodedeviceobj.c                   |   7 +-
 src/libvirt_private.syms                      |  20 +
 src/node_device/node_device_driver.c          |   2 +
 src/node_device/node_device_udev.c            |   2 +
 src/util/meson.build                          |   1 +
 src/util/virpci.c                             |  70 ++
 src/util/virpci.h                             |   4 +
 src/util/virpcivpd.c                          | 743 +++++++++++++++++
 src/util/virpcivpd.h                          | 111 +++
 src/util/virpcivpdpriv.h                      |  48 ++
 tests/meson.build                             |   1 +
 .../pci_0000_42_00_0_vpd.xml                  |  42 +
 .../pci_0000_42_00_0_vpd.xml                  |   1 +
 tests/nodedevxml2xmltest.c                    |   1 +
 tests/testutils.c                             |  35 +
 tests/testutils.h                             |   4 +
 tests/virpcimock.c                            |  30 +
 tests/virpcitest.c                            |  39 +
 tests/virpcivpdtest.c                         | 768 ++++++++++++++++++
 tools/virsh-nodedev.c                         |   3 +
 29 files changed, 2483 insertions(+), 5 deletions(-)
 create mode 100644 src/util/virpcivpd.c
 create mode 100644 src/util/virpcivpd.h
 create mode 100644 src/util/virpcivpdpriv.h
 create mode 100644 tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml
 create mode 120000 tests/nodedevxml2xmlout/pci_0000_42_00_0_vpd.xml
 create mode 100644 tests/virpcivpdtest.c

-- 
2.30.2


Re: [libvirt PATCH v6 0/5] Add a PCI/PCIe device VPD Capability
Posted by Daniel P. Berrangé 2 years, 5 months ago
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 :|

Re: [libvirt PATCH v6 0/5] Add a PCI/PCIe device VPD Capability
Posted by Dmitrii Shcherbakov 2 years, 5 months ago
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 :|
>