On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
> Add support for deserializing the binary PCI/PCIe VPD format and storing
> results in memory.
>
> 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.
>
> A GTree is used as a data structure in order to maintain key ordering
> which will be important in XML to XML tests later.
>
> Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherba...@canonical.com>
> ---
> build-aux/syntax-check.mk | 4 +-
> po/POTFILES.in | 1 +
> src/libvirt_private.syms | 15 +
> src/util/meson.build | 1 +
> src/util/virpcivpd.c | 755 ++++++++++++++++++++++++++++++++++++++
> src/util/virpcivpd.h | 117 ++++++
> src/util/virpcivpdpriv.h | 45 +++
> tests/meson.build | 1 +
> tests/testutils.c | 40 ++
> tests/testutils.h | 4 +
> tests/virpcivpdtest.c | 705 +++++++++++++++++++++++++++++++++++
> 11 files changed, 1686 insertions(+), 2 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/virpcivpdtest.c
> +/**
> + * virPCIVPDParse:
> + * @vpdFileFd: a file descriptor associated with a file containing PCI
> device VPD.
> + *
> + * Parse a PCI device's Vital Product Data (VPD) contained in a file
> descriptor.
> + *
> + * Returns: a pointer to a GList of VPDResource types which needs to be
> freed by the caller or
> + * NULL if getting it failed for some reason.
> + */
> +GList *
> +virPCIVPDParse(int vpdFileFd)
> +{
> + /* A checksum which is calculated as a sum of all bytes from VPD byte 0
> up to
> + * the checksum byte in the RV field's value. The RV field is only
> present in the
> + * VPD-R resource and the checksum byte there is the first byte of the
> field's value.
> + * The checksum byte in RV field is actually a two's complement of the
> sum of all bytes
> + * of VPD that come before it so adding the two together must produce 0
> if data
> + * was not corrupted and VPD storage is intact.
> + */
> + uint8_t csum = 0;
> + uint8_t headerBuf[2];
> +
> + g_autolist(virPCIVPDResource) resList = NULL;
> + uint16_t resPos = 0, resDataLen;
> + uint8_t tag = 0;
> + bool endResReached = false, hasReadOnlyRes = false;
> +
> + g_autoptr(virPCIVPDResource) res = NULL;
> +
> + while (resPos <= PCI_VPD_ADDR_MASK) {
> + /* Read the resource data type tag. */
> + if (virPCIVPDReadVPDBytes(vpdFileFd, &tag, 1, resPos, &csum) != 1) {
> + break;
> + }
> + /* 0x80 == 0b10000000 - the large resource data type flag. */
> + if (tag & PCI_VPD_LARGE_RESOURCE_FLAG) {
> + if (resPos > PCI_VPD_ADDR_MASK + 1 - 3) {
> + /* Bail if the large resource starts at the position where
> the end tag should be. */
> + break;
> + }
> + /* Read the two length bytes of the large resource record. */
> + if (virPCIVPDReadVPDBytes(vpdFileFd, headerBuf, 2, resPos + 1,
> &csum) != 2) {
> + break;
> + }
> + resDataLen = headerBuf[0] + (headerBuf[1] << 8);
> + /* Change the position to the byte following the tag and length
> bytes. */
> + resPos += 3;
> + } else {
> + /* Handle a small resource record.
> + * 0xxxxyyy & 00000111, where xxxx - resource data type bits,
> yyy - length bits. */
> + resDataLen = tag & 7;
> + /* 0xxxxyyy >> 3 == 0000xxxx */
> + tag >>= 3;
> + /* Change the position to the byte past the byte containing tag
> and length bits. */
> + resPos += 1;
> + }
> + if (tag == PCI_VPD_RESOURCE_END_TAG) {
> + /* Stop VPD traversal since the end tag was encountered. */
> + endResReached = true;
> + break;
> + }
> + if (resDataLen > PCI_VPD_ADDR_MASK + 1 - resPos) {
> + /* Bail if the resource is too long to fit into the VPD address
> space. */
> + break;
> + }
> +
> + switch (tag) {
> + /* Large resource type which is also a string: 0x80 | 0x02 =
> 0x82 */
> + case PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_STRING_RESOURCE_FLAG:
> + res =
> + (virPCIVPDResource *)
> virPCIVPDParseVPDLargeResourceString(vpdFileFd, resPos,
> +
> resDataLen, &csum);
> + break;
> + /* Large resource type which is also a VPD-R: 0x80 | 0x10 ==
> 0x90 */
> + case PCI_VPD_LARGE_RESOURCE_FLAG |
> PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG:
> + res =
> + (virPCIVPDResource *)
> virPCIVPDParseVPDLargeResourceFields(vpdFileFd, resPos,
> +
> resDataLen, true,
> +
> &csum);
> + /* Encountered the VPD-R tag. The resource record parsing
> also validates
> + * the presence of the required checksum in the RV field. */
> + hasReadOnlyRes = true;
> + break;
> + /* Large resource type which is also a VPD-W: 0x80 | 0x11 ==
> 0x91 */
> + case PCI_VPD_LARGE_RESOURCE_FLAG |
> PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG:
> + res =
> + (virPCIVPDResource *)
> virPCIVPDParseVPDLargeResourceFields(vpdFileFd, resPos,
> +
> resDataLen, false,
> +
> &csum);
> + break;
> + default:
> + /* While we cannot parse unknown resource types, they can
> still be skipped
> + * based on the header and data length. */
> + VIR_DEBUG("Encountered an unexpected VPD resource tag: %#x",
> tag);
> + resPos += resDataLen;
> + continue;
> + }
> +
> + if (!res) {
> + VIR_DEBUG("Encountered an invalid VPD");
> + return NULL;
> + }
> +
> + resList = g_list_append(resList, g_steal_pointer(&res));
> + /* Continue processing other resource records. */
> + resPos += resDataLen;
> + }
> +
> + if (VIR_CLOSE(vpdFileFd) < 0) {
> + virReportSystemError(errno, _("Unable to close the VPD file, fd:
> %d"), vpdFileFd);
> + return NULL;
> + }
This is closing an FD that is owned & passed in by the caller. I'd
consider that an undesirable pattern. Whomever opens an FD should
generally take responsiiblity for closing it too, as that gives
clear semantics on state of the FD, when this method returns an
error state.
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 :|
