[libvirt PATCH v5 0/7] Add a PCI/PCIe device VPD Capability

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). Per the existing guidelines, the implementation relies heavily on glib for various purposes. https://libvirt.org/glib-adoption.html * v5 fixes a couple of minor typos and adds NEWS entries. * The v4 of the patch includes a number of fixes compared to v3: * Fixed the patch to correctly build against older glib (2.56.0); * Notably, glib commit 86c073dba9d82ef3f1bc3d3116b058b9b5c3b1eb (in 2.59.0) fixes g_autolist support for derivable Glib types. To make things work in 2.56.0 a workaround is conditionally applied; * virCreateAnonymousFile now uses a temporary file which is unlinked after creation instead of memfd because OpenSUSE 15.2 does not have support memfd; * Keyword resources now use GTree instead of GHashTable as the underlying data structure: * This allows for stable ordering which is important for XML2XML tests as they were failing with when GLib versions were different, resulting in a different ordering of elements; * The keyword resource iteration function was complex and got replaced by a simpler g_tree_foreach-based approach; * Added more testing: functions added to virpci are now assessed by creating a mocked vpd file under a mocked sysfs structure while the parser is still tested in virpcivpdtest file; * Refactoring: * Applied changes based on the indent tool operation with some post-processing; * Renamed functions which had the Glib naming style to use camel case where possible. Auto-generated declarations are an exception: gobject/gtype.h defines type_name##_init, type_name##_class_init, module_obj_name##_get_type functions which were left unchanged; * camelCase is now used for local variables and function parameters; * Replaced //-style comments with multi-line ones; * Split out one patch into 4 based on distinct features: * PCI VPD parser functionality and the respective in-memory types; * VPD helpers in virpci; * XML serialization/deserialization and VPD capability support; * Documentation. Build & test results for targets in ci/manifest.yaml: ci/helper test --meson-args='-Dexpensive_tests=enabled' <target> https://gist.github.com/dshcherb/225b5da9478275f08c220487814ffd1c Dmitrii Shcherbakov (7): Add a PCI/PCIe device VPD Parser News: Add a PCI VPD parser Add PCI VPD-related helper functions to virpci News: Add PCI VPD-related helper functions to virpci Add PCI VPD Capability Support News: Add PCI VPD capability support Add PCI VPD Capability Documentation NEWS.rst | 22 + build-aux/syntax-check.mk | 4 +- docs/drvnodedev.html.in | 46 ++ docs/formatnode.html.in | 24 +- docs/schemas/nodedev.rng | 40 + include/libvirt/libvirt-nodedev.h | 1 + po/POTFILES.in | 1 + src/conf/node_device_conf.c | 271 +++++++ src/conf/node_device_conf.h | 6 +- src/conf/virnodedeviceobj.c | 7 +- src/libvirt_private.syms | 17 + 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 | 62 ++ src/util/virpci.h | 3 + src/util/virpcivpd.c | 755 ++++++++++++++++++ src/util/virpcivpd.h | 117 +++ src/util/virpcivpdpriv.h | 45 ++ tests/meson.build | 1 + .../pci_0000_42_00_0_vpd.xml | 33 + .../pci_0000_42_00_0_vpd.xml | 1 + tests/nodedevxml2xmltest.c | 1 + tests/testutils.c | 40 + tests/testutils.h | 4 + tests/virpcimock.c | 30 + tests/virpcitest.c | 64 ++ tests/virpcivpdtest.c | 705 ++++++++++++++++ tools/virsh-nodedev.c | 3 + 29 files changed, 2303 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

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.shcherbakov@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 diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index cb54c8ba36..a428831380 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -775,9 +775,9 @@ sc_prohibit_windows_special_chars_in_filename: { echo '$(ME): Windows special chars in filename not allowed' 1>&2; echo exit 1; } || : sc_prohibit_mixed_case_abbreviations: - @prohibit='Pci|Usb|Scsi' \ + @prohibit='Pci|Usb|Scsi|Vpd' \ in_vc_files='\.[ch]$$' \ - halt='Use PCI, USB, SCSI, not Pci, Usb, Scsi' \ + halt='Use PCI, USB, SCSI, VPD, not Pci, Usb, Scsi, Vpd' \ $(_sc_search_regexp) # Require #include <locale.h> in all files that call setlocale() diff --git a/po/POTFILES.in b/po/POTFILES.in index c200d7452a..4be4139529 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -302,6 +302,7 @@ @SRCDIR@src/util/virnvme.c @SRCDIR@src/util/virobject.c @SRCDIR@src/util/virpci.c +@SRCDIR@src/util/virpcivpd.c @SRCDIR@src/util/virperf.c @SRCDIR@src/util/virpidfile.c @SRCDIR@src/util/virpolkit.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6de9d9aef1..bb9b380599 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3571,6 +3571,21 @@ virVHBAManageVport; virVHBAPathExists; +# util/virpcivpd.h + +virPCIVPDKeywordResourceForEach; +virPCIVPDKeywordResourceNew; +virPCIVPDParse; +virPCIVPDParseVPDLargeResourceFields; +virPCIVPDParseVPDLargeResourceString; +virPCIVPDReadVPDBytes; +virPCIVPDResourceGetFieldValueFormat; +virPCIVPDResourceGetResourceType; +virPCIVPDResourceIsExpectedKeyword; +virPCIVPDResourceIsValidTextValue; +virPCIVPDStringResourceGetValue; +virPCIVPDStringResourceNew; + # util/virvsock.h virVsockAcquireGuestCid; virVsockSetGuestCid; diff --git a/src/util/meson.build b/src/util/meson.build index 05934f6841..24350a3e67 100644 --- a/src/util/meson.build +++ b/src/util/meson.build @@ -105,6 +105,7 @@ util_sources = [ 'virutil.c', 'viruuid.c', 'virvhba.c', + 'virpcivpd.c', 'virvsock.c', 'virxml.c', ] diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c new file mode 100644 index 0000000000..849ea0570c --- /dev/null +++ b/src/util/virpcivpd.c @@ -0,0 +1,755 @@ +/* + * virpcivpd.c: helper APIs for working with the PCI/PCIe VPD capability + * + * Copyright (C) 2021 Canonical Ltd. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#ifdef __linux__ +# include <unistd.h> +#endif + +#define LIBVIRT_VIRPCIVPDPRIV_H_ALLOW + +#include "virpcivpdpriv.h" +#include "virlog.h" +#include "virerror.h" +#include "virfile.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.pcivpd"); + +GType +vir_pci_vpd_resource_type_get_type(void) +{ + static GType resourceType; + + static const GEnumValue resourceTypes[] = { + {VIR_PCI_VPD_RESOURCE_TYPE_STRING, "String resource", "string"}, + {VIR_PCI_VPD_RESOURCE_TYPE_VPD_R, "Read-only resource", "vpd-r"}, + {VIR_PCI_VPD_RESOURCE_TYPE_VPD_W, "Read-write resource", "vpd-w"}, + {VIR_PCI_VPD_RESOURCE_TYPE_LAST, "last", "last"}, + {0, NULL, NULL}, + }; + + if (!resourceType) { + resourceType = g_enum_register_static("virPCIVPDResourceType", resourceTypes); + } + return resourceType; +} + +static gboolean +virPCIVPDResourceIsVendorKeyword(const gchar *keyword) +{ + return g_str_has_prefix(keyword, "V"); +} + +static gboolean +virPCIVPDResourceIsSystemKeyword(const gchar *keyword) +{ + /* Special-case the system-specific keywords since they share the "Y" prefix with "YA". */ + return g_str_has_prefix(keyword, "Y") && STRNEQ(keyword, "YA"); +} + +static gchar * +virPCIVPDResourceGetKeywordPrefix(const gchar *keyword) +{ + g_autofree gchar *key = NULL; + + /* Keywords must have a length of 2 bytes. */ + if (strlen(keyword) != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("The keyword length is not 2 bytes: %s"), keyword); + return NULL; + } else if (!(g_ascii_isalnum(keyword[0]) && g_ascii_isalnum(keyword[1]))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The keyword contains non-alphanumeric ASCII characters")); + return NULL; + } + /* Special-case the system-specific keywords since they share the "Y" prefix with "YA". */ + if (virPCIVPDResourceIsSystemKeyword(keyword) || virPCIVPDResourceIsVendorKeyword(keyword)) { + key = g_strndup(keyword, 1); + } else { + key = g_strndup(keyword, 2); + } + return g_steal_pointer(&key); +} + +/** + * virPCIVPDResourceGetFieldValueFormat: + * @keyword: A keyword for which to get a value type + * + * Returns: a virPCIVPDResourceFieldValueFormat value which specifies the field value type for + * a provided keyword based on the static information from PCI(e) specs. + */ +virPCIVPDResourceFieldValueFormat +virPCIVPDResourceGetFieldValueFormat(const gchar *keyword) +{ + static GHashTable *fieldValueFormats; + g_autofree gchar *key = NULL; + gpointer keyVal = NULL; + virPCIVPDResourceFieldValueFormat format = VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST; + + /* Keywords are expected to be 2 bytes in length which is defined in the specs. */ + if (strlen(keyword) != 2) { + return VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST; + } + + if (!fieldValueFormats) { + /* Initialize a hash table once with static format metadata coming from the PCI(e) specs. + * The VPD format does not embed format metadata into the resource records so it is not + * possible to do format discovery without static information. Legacy PICMIG keywords + * are not included. */ + fieldValueFormats = g_hash_table_new(g_str_hash, g_str_equal); + /* Extended capability. Contains binary data per PCI(e) specs. */ + g_hash_table_insert(fieldValueFormats, g_strdup("CP"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_BINARY)); + /* Engineering Change Level of an Add-in Card. */ + g_hash_table_insert(fieldValueFormats, g_strdup("EC"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* Manufacture ID */ + g_hash_table_insert(fieldValueFormats, g_strdup("MN"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* Add-in Card Part Number */ + g_hash_table_insert(fieldValueFormats, g_strdup("PN"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* Checksum and Reserved */ + g_hash_table_insert(fieldValueFormats, g_strdup("RV"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD)); + /* Remaining Read/Write Area */ + g_hash_table_insert(fieldValueFormats, g_strdup("RW"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR)); + /* Serial Number */ + g_hash_table_insert(fieldValueFormats, g_strdup("SN"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* Asset Tag Identifier */ + g_hash_table_insert(fieldValueFormats, g_strdup("YA"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* This is a vendor specific item and the characters are alphanumeric. The second + * character (x) of the keyword can be 0 through Z so only the first one is stored. */ + g_hash_table_insert(fieldValueFormats, g_strdup("V"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* This is a system specific item and the characters are alphanumeric. + * The second character (x) of the keyword can be 0 through 9 and B through Z. */ + g_hash_table_insert(fieldValueFormats, g_strdup("Y"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + } + + /* The system and vendor-specific keywords have a variable part - lookup + * the prefix significant for determining the value format. */ + key = virPCIVPDResourceGetKeywordPrefix(keyword); + if (key) { + keyVal = g_hash_table_lookup(fieldValueFormats, key); + if (keyVal) { + format = GPOINTER_TO_INT(keyVal); + } + } + return format; +} + +/** + * virPCIVPDResourceIsExpectedKeyword: + * @keyword: A keyword to assess. + * @readOnly: A parameter indicating whether the resource is read-only or not. + * + * Returns: a boolean indicating whether this keyword is expected to be in a + * read-only or read-write keyword resource or not. The expectations are based + * on the keywords specified in relevant sections of PCI(e) specifications + * ("I.3. VPD Definitions" in PCI specs, "6.28.1 VPD Format" PCIe 4.0). + */ +gboolean +virPCIVPDResourceIsExpectedKeyword(const gchar *keyword, gboolean readOnly) +{ + g_autofree gchar *key = NULL; + + static const char *expectedReadOnlyKeys[] = { + "CP", "EC", "FG", "LC", "MN", + "PG", "PN", "RV", "SN", "V", NULL + }; + static const char *expectedReadWriteKeys[] = { "V", "Y", "YA", "RW", NULL }; + + key = virPCIVPDResourceGetKeywordPrefix(keyword); + if (!key) { + return false; + } + if (readOnly) { + return g_strv_contains(expectedReadOnlyKeys, key); + } else { + return g_strv_contains(expectedReadWriteKeys, key); + } +} + +/** + * virPCIVPDResourceIsValidTextValue: + * @value: A NULL-terminated string to assess. + * + * Returns: a boolean indicating whether this value is a valid string resource + * value or text field value. The expectations are based on the keywords specified + * in relevant sections of PCI(e) specifications + * ("I.3. VPD Definitions" in PCI specs, "6.28.1 VPD Format" PCIe 4.0). + */ +gboolean +virPCIVPDResourceIsValidTextValue(const gchar *value) +{ + /* + * The PCI(e) specs mention alphanumeric characters when talking about text fields + * and the string resource but also include spaces and dashes in the provided example. + * Dots, commas, equal signs have also been observed in values used by major device vendors. + * The specs do not specify a full set of allowed code points and for Libvirt it is important + * to keep values in the ranges allowed within XML elements (mainly excluding less-than, + * greater-than and ampersand). + */ + if (!g_regex_match_simple("^[a-zA-Z0-9\\-_\\s,.:=]*$", value, 0, 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("The provided value contains invalid characters: %s"), value); + return false; + } + return true; +} + +/* virPCIVPDResource */ + +typedef enum { + PROP_RESOURCE_TYPE = 1, + N_PROPERTIES +} virPCIVPDResourceProperty; + +typedef struct _virPCIVPDResourcePrivate { + GObject parent; + virPCIVPDResourceType resource_type; +} virPCIVPDResourcePrivate; + + +static void vir_pci_vpd_resource_class_init(virPCIVPDResourceClass *klass); +static void vir_pci_vpd_resource_init(virPCIVPDResource *res); +static void virPCIVPDResourceFinalize(GObject *object); + +G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE(virPCIVPDResource, vir_pci_vpd_resource, G_TYPE_OBJECT); +static void +vir_pci_vpd_resource_init(virPCIVPDResource *self) +{ + virPCIVPDResourcePrivate *priv; + + priv = vir_pci_vpd_resource_get_instance_private(self); + priv->resource_type = VIR_PCI_VPD_RESOURCE_TYPE_LAST; +} + +static void +virPCIVPDResourceSetProperty(GObject *object, guint propertyId, + const GValue *value, GParamSpec *pspec) +{ + virPCIVPDResource *self = VIR_PCI_VPD_RESOURCE(object); + virPCIVPDResourcePrivate *priv; + + priv = vir_pci_vpd_resource_get_instance_private(self); + + switch ((virPCIVPDResourceProperty) propertyId) { + case PROP_RESOURCE_TYPE: + priv->resource_type = g_value_get_enum(value); + break; + case N_PROPERTIES: + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, propertyId, pspec); + break; + } +} + +static void +virPCIVPDResourceGetProperty(GObject *object, guint propertyId, + GValue *value, GParamSpec *pspec) +{ + virPCIVPDResource *self = VIR_PCI_VPD_RESOURCE(object); + virPCIVPDResourcePrivate *priv; + + priv = vir_pci_vpd_resource_get_instance_private(self); + + switch ((virPCIVPDResourceProperty) propertyId) { + case PROP_RESOURCE_TYPE: + g_value_set_enum(value, priv->resource_type); + break; + case N_PROPERTIES: + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, propertyId, pspec); + break; + } +} + +static void +vir_pci_vpd_resource_class_init(virPCIVPDResourceClass *klass) +{ + GObjectClass *obj_class = G_OBJECT_CLASS(klass); + + obj_class->set_property = virPCIVPDResourceSetProperty; + obj_class->get_property = virPCIVPDResourceGetProperty; + + g_object_class_install_property(obj_class, PROP_RESOURCE_TYPE, + g_param_spec_enum("resource_type", "Resource type", + "A VPD resource type per PCI(e) specifications.", + VIR_TYPE_PCI_VPD_RESOURCE_TYPE, + VIR_PCI_VPD_RESOURCE_TYPE_LAST, + G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE)); + obj_class->finalize = virPCIVPDResourceFinalize; +} + +static void +virPCIVPDResourceFinalize(GObject *object) +{ + G_OBJECT_CLASS(vir_pci_vpd_resource_parent_class)->finalize(object); +} + +GEnumValue * +virPCIVPDResourceGetResourceType(virPCIVPDResource *res) +{ + GValue gval = G_VALUE_INIT; + GEnumValue *enumVal; + GEnumClass *class; + + g_value_init(&gval, VIR_TYPE_PCI_VPD_RESOURCE_TYPE); + g_object_get_property(G_OBJECT(res), "resource_type", &gval); + + class = g_type_class_ref(VIR_TYPE_PCI_VPD_RESOURCE_TYPE); + + enumVal = g_enum_get_value(class, g_value_get_enum(&gval)); + g_type_class_unref(class); + return enumVal; +} + + +/* virPCIVPDStringResource */ + +struct _virPCIVPDStringResource { + virPCIVPDResource parent; + gchar *value; +}; + +G_DEFINE_TYPE(virPCIVPDStringResource, vir_pci_vpd_string_resource, VIR_TYPE_PCI_VPD_RESOURCE); + +static void vir_pci_vpd_string_resource_class_init(virPCIVPDStringResourceClass *klass); +static void vir_pci_vpd_string_resource_init(virPCIVPDStringResource *res); +static void virPCIVPDStringResourceFinalize(GObject *object); + +static void +vir_pci_vpd_string_resource_init(virPCIVPDStringResource *res) +{ + res->value = NULL; +} + +static void +vir_pci_vpd_string_resource_class_init(virPCIVPDStringResourceClass *klass) +{ + GObjectClass *obj = G_OBJECT_CLASS(klass); + + obj->finalize = virPCIVPDStringResourceFinalize; +} + +static void +virPCIVPDStringResourceFinalize(GObject *object) +{ + virPCIVPDStringResource *res = VIR_PCI_VPD_STRING_RESOURCE(object); + + g_free(res->value); + G_OBJECT_CLASS(vir_pci_vpd_resource_parent_class)->finalize(object); +} + +virPCIVPDStringResource * +virPCIVPDStringResourceNew(gchar *value) +{ + g_autoptr(virPCIVPDStringResource) res = NULL; + + res = VIR_PCI_VPD_STRING_RESOURCE(g_object_new(VIR_TYPE_PCI_VPD_STRING_RESOURCE, + "resource_type", + VIR_PCI_VPD_RESOURCE_TYPE_STRING, NULL)); + res->value = value; + return g_steal_pointer(&res); +} + +const gchar * +virPCIVPDStringResourceGetValue(const virPCIVPDStringResource *res) +{ + return res->value; +} + +/* virPCIVPDKeywordResource */ + +struct _virPCIVPDKeywordResource { + virPCIVPDResource parent; + GTree *resource_fields; +}; + +G_DEFINE_TYPE(virPCIVPDKeywordResource, vir_pci_vpd_keyword_resource, VIR_TYPE_PCI_VPD_RESOURCE); + +static void vir_pci_vpd_keyword_resource_class_init(virPCIVPDKeywordResourceClass *klass); +static void vir_pci_vpd_keyword_resource_init(virPCIVPDKeywordResource *res); +static void virPCIVPDKeywordResourceFinalize(GObject *object); + +static void +vir_pci_vpd_keyword_resource_class_init(virPCIVPDKeywordResourceClass *klass) +{ + GObjectClass *obj = G_OBJECT_CLASS(klass); + + obj->finalize = virPCIVPDKeywordResourceFinalize; +} + +static void +vir_pci_vpd_keyword_resource_init(virPCIVPDKeywordResource *res) +{ + res->resource_fields = NULL; +} + +static void +virPCIVPDKeywordResourceFinalize(GObject *object) +{ + virPCIVPDKeywordResource *res = VIR_PCI_VPD_KEYWORD_RESOURCE(object); + + g_tree_destroy(g_steal_pointer(&res->resource_fields)); + G_OBJECT_CLASS(vir_pci_vpd_resource_parent_class)->finalize(object); +} + +virPCIVPDKeywordResource * +virPCIVPDKeywordResourceNew(GTree *resourceFields, bool readOnly) +{ + g_autoptr(virPCIVPDKeywordResource) res = NULL; + virPCIVPDResourceType t; + + t = readOnly ? VIR_PCI_VPD_RESOURCE_TYPE_VPD_R : VIR_PCI_VPD_RESOURCE_TYPE_VPD_W; + /* + * Create an instance and set a property specifying its VPD resource type taking the + * readOnly parameter into account. + */ + res = VIR_PCI_VPD_KEYWORD_RESOURCE(g_object_new(VIR_TYPE_PCI_VPD_KEYWORD_RESOURCE, + "resource_type", t, NULL)); + res->resource_fields = g_tree_ref(resourceFields); + return g_steal_pointer(&res); +} + +void +virPCIVPDKeywordResourceForEach(virPCIVPDKeywordResource *res, GTraverseFunc func, + gpointer userData) +{ + g_tree_foreach(res->resource_fields, func, userData); +} + +#ifdef __linux__ + +/** + * virPCIVPDReadVPDBytes: + * @vpdFileFd: A file descriptor associated with a file containing PCI device VPD. + * @buf: An allocated buffer to use for storing VPD bytes read. + * @count: The number of bytes to read from the VPD file descriptor. + * @offset: The offset at which bytes need to be read. + * @csum: A pointer to a byte containing the current checksum value. Mutated by this function. + * + * Returns: the number of VPD bytes read from the specified file descriptor. The csum value is + * also modified as bytes are read. If an error occurs while reading data from the VPD file + * descriptor, it is reported and -1 is returned to the caller. If EOF is occurred, 0 is returned + * to the caller. + */ +size_t +virPCIVPDReadVPDBytes(int vpdFileFd, uint8_t *buf, size_t count, off_t offset, uint8_t *csum) +{ + ssize_t numRead = pread(vpdFileFd, buf, count, offset); + + if (numRead == -1) { + VIR_DEBUG("Unable to read %zu bytes at offset %ld from fd: %d", count, offset, vpdFileFd); + } else if (numRead) { + /* + * Update the checksum for every byte read. Per the PCI(e) specs + * the checksum is correct if the sum of all bytes in VPD from + * VPD address 0 up to and including the VPD-R RV field's first + * data byte is zero. + */ + while (count--) { + *csum += *buf; + buf++; + } + } + return numRead; +} + +/** + * virPCIVPDParseVPDLargeResourceFields: + * @vpdFileFd: A file descriptor associated with a file containing PCI device VPD. + * @resPos: A position where the resource data bytes begin in a file descriptor. + * @resDataLen: A length of the data portion of a resource. + * @readOnly: A boolean showing whether the resource type is VPD-R or VPD-W. + * @csum: A pointer to a 1-byte checksum. + * + * Returns: a pointer to a VPDResource which needs to be freed by the caller or + * NULL if getting it failed for some reason. + */ +virPCIVPDKeywordResource * +virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, + bool readOnly, uint8_t *csum) +{ + g_autofree char *fieldKeyword = NULL; + g_autofree char *fieldValue = NULL; + virPCIVPDResourceFieldValueFormat fieldFormat = VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST; + + /* A buffer of up to one resource record field size (plus a zero byte) is needed. */ + g_autofree uint8_t *buf = g_malloc0(PCI_VPD_MAX_FIELD_SIZE + 1); + uint16_t fieldDataLen = 0, bytesToRead = 0; + uint16_t fieldPos = resPos; + + g_autoptr(GTree) resFieldTree = g_tree_new_full((GCompareDataFunc)g_strcmp0, NULL, + g_free, g_free); + bool hasChecksum = false; + + while (fieldPos + 3 < resPos + resDataLen) { + /* Keyword resources consist of keywords (2 ASCII bytes per the spec) and 1-byte length. */ + if (virPCIVPDReadVPDBytes(vpdFileFd, buf, 3, fieldPos, csum) != 3) { + /* Invalid field encountered which means the resource itself is invalid too. Report + * That VPD has invalid format and bail. */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not read a resource field header - VPD has invalid format")); + return NULL; + } + fieldDataLen = buf[2]; + /* Change the position to the field's data portion skipping the keyword and length bytes. */ + fieldPos += 3; + fieldKeyword = g_strndup((char *)buf, 2); + + if (!virPCIVPDResourceIsExpectedKeyword(fieldKeyword, readOnly)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected keyword encountered - VPD has invalid format")); + return NULL; + } + fieldFormat = virPCIVPDResourceGetFieldValueFormat(fieldKeyword); + + if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) { + if (!fieldDataLen) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The RV field has a 0 length - VPD has invalid format.")); + return NULL; + } + /* Only need one byte to be read and accounted towards the checksum calculation. */ + bytesToRead = 1; + } else { + bytesToRead = fieldDataLen; + } + if (virPCIVPDReadVPDBytes(vpdFileFd, buf, bytesToRead, fieldPos, csum) != bytesToRead) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not parse a resource field data - VPD has invalid format")); + return NULL; + } + /* Advance the position to the first byte of the next field. */ + fieldPos += fieldDataLen; + + if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT) { + /* Trim whitespace around a retrieved value and set it to be a field's value. Cases + * where unnecessary whitespace was present around a field value have been encountered + * in the wild. + */ + fieldValue = g_strstrip(g_strndup((char *)buf, fieldDataLen)); + if (!virPCIVPDResourceIsValidTextValue(fieldValue)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Field value contains invalid characters")); + return NULL; + } + } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) { + if (*csum) { + /* All bytes up to and including the checksum byte should add up to 0. */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Checksum validation has failed")); + return NULL; + } + hasChecksum = true; + g_free(g_steal_pointer(&fieldKeyword)); + g_free(g_steal_pointer(&fieldValue)); + continue; + } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR || + fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST) { + /* Skip fields which are not known in advance or the read-write space field since + * it is not being used. */ + g_free(g_steal_pointer(&fieldKeyword)); + g_free(g_steal_pointer(&fieldValue)); + continue; + } else { + fieldValue = g_malloc(fieldDataLen); + memcpy(fieldValue, buf, fieldDataLen); + } + /* At this point it is determined that the keyword is expected and field format + * is known and acceptable. */ + g_tree_insert(resFieldTree, + g_steal_pointer(&fieldKeyword), g_steal_pointer(&fieldValue)); + } + if (readOnly && !hasChecksum) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _ + ("A VPD-R resource does not contain the mandatory RV field with a checksum")); + return NULL; + } + return virPCIVPDKeywordResourceNew(g_steal_pointer(&resFieldTree), readOnly); +} + +/** + * virPCIVPDParseVPDLargeResourceString: + * @vpdFileFd: A file descriptor associated with a file containing PCI device VPD. + * @resPos: A position where the resource data bytes begin in a file descriptor. + * @resDataLen: A length of the data portion of a resource. + * @csum: A pointer to a 1-byte checksum. + * + * Returns: a pointer to a VPDResource which needs to be freed by the caller or + * NULL if getting it failed for some reason. + */ +virPCIVPDStringResource * +virPCIVPDParseVPDLargeResourceString(int vpdFileFd, uint16_t resPos, + uint16_t resDataLen, uint8_t *csum) +{ + g_autofree char *resValue = NULL; + + /* The resource value is not NULL-terminated so add one more byte. */ + g_autofree char *buf = g_malloc0(resDataLen + 1); + + if (virPCIVPDReadVPDBytes(vpdFileFd, (uint8_t *) buf, resDataLen, + resPos, csum) != resDataLen) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not read a part of a resource - VPD has invalid format")); + return NULL; + } + resValue = g_strdup(g_strstrip(buf)); + if (!virPCIVPDResourceIsValidTextValue(resValue)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The string resource has invalid characters in its value")); + return NULL; + } + return virPCIVPDStringResourceNew(g_steal_pointer(&resValue)); +} + +/** + * 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; + } + if (!hasReadOnlyRes) { + VIR_DEBUG("Encountered an invalid VPD: does not have a VPD-R record"); + return NULL; + } else if (!(endResReached && g_list_length(resList) > 0)) { + /* Does not have an end tag or there are not any other resources. */ + VIR_DEBUG("Encountered an invalid VPD"); + return NULL; + } + return g_steal_pointer(&resList); +} + +#endif /* __linux__ */ diff --git a/src/util/virpcivpd.h b/src/util/virpcivpd.h new file mode 100644 index 0000000000..7327806df3 --- /dev/null +++ b/src/util/virpcivpd.h @@ -0,0 +1,117 @@ +/* + * virpcivpd.h: helper APIs for working with the PCI/PCIe VPD capability + * + * Copyright (C) 2021 Canonical Ltd. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" + +/* + * PCI Local bus (2.2+, Appendix I) and PCIe 4.0+ (7.9.19 VPD Capability) define + * the VPD capability structure (8 bytes in total) and VPD registers that can be used to access + * VPD data including: + * bit 31 of the first 32-bit DWORD: data transfer completion flag (between the VPD data register + * and the VPD data storage hardware); + * bits 30:16 of the first 32-bit DWORD: VPD address of the first VPD data byte to be accessed; + * bits 31:0 of the second 32-bit DWORD: VPD data bytes with LSB being pointed to by the VPD address. + * + * Given that only 15 bits (30:16) are allocated for VPD address its mask is 0x7fff. +*/ +#define PCI_VPD_ADDR_MASK 0x7FFF + +/* + * VPD data consists of small and large resource data types. Information within a resource type + * consists of a 2-byte keyword, 1-byte length and data bytes (up to 255). +*/ +#define PCI_VPD_MAX_FIELD_SIZE 255 +#define PCI_VPD_LARGE_RESOURCE_FLAG 0x80 +#define PCI_VPD_STRING_RESOURCE_FLAG 0x02 +#define PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG 0x10 +#define PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG 0x11 +#define PCI_VPD_RESOURCE_END_TAG 0x0F +#define PCI_VPD_RESOURCE_END_VAL PCI_VPD_RESOURCE_END_TAG << 3 +#define VIR_TYPE_PCI_VPD_RESOURCE_TYPE vir_pci_vpd_resource_type_get_type() + +G_BEGIN_DECLS; + +typedef enum { + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT = 1, + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_BINARY, + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD, + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR, + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST +} virPCIVPDResourceFieldValueFormat; + +typedef enum { + VIR_PCI_VPD_RESOURCE_TYPE_STRING = 1, + VIR_PCI_VPD_RESOURCE_TYPE_VPD_R, + VIR_PCI_VPD_RESOURCE_TYPE_VPD_W, + VIR_PCI_VPD_RESOURCE_TYPE_LAST +} virPCIVPDResourceType; + +GType vir_pci_vpd_resource_type_get_type(void); + +virPCIVPDResourceFieldValueFormat virPCIVPDResourceGetFieldValueFormat(const gchar *value); + +#define VIR_TYPE_PCI_VPD_RESOURCE vir_pci_vpd_resource_get_type() +G_DECLARE_DERIVABLE_TYPE(virPCIVPDResource, vir_pci_vpd_resource, VIR, PCI_VPD_RESOURCE, GObject); +struct _virPCIVPDResourceClass { + GObjectClass parent_class; +}; + +/* Glib 2.59 adds proper support for g_autolist for derivable types + * see 86c073dba9d82ef3f1bc3d3116b058b9b5c3b1eb. At the time of writing + * 2.56 is the minimum version used by Libvirt. Without the below code + * compilation errors will occur. + */ +#if !GLIB_CHECK_VERSION(2, 59, 0) +typedef GList *virPCIVPDResource_listautoptr; +static inline void +glib_listautoptr_cleanup_virPCIVPDResource(GList **_l) +{ + g_list_free_full(*_l, (GDestroyNotify)g_object_unref); +} +#endif + +GEnumValue *virPCIVPDResourceGetResourceType(virPCIVPDResource *res); + +#define VIR_TYPE_PCI_VPD_STRING_RESOURCE vir_pci_vpd_string_resource_get_type() +G_DECLARE_FINAL_TYPE(virPCIVPDStringResource, vir_pci_vpd_string_resource, VIR, + PCI_VPD_STRING_RESOURCE, virPCIVPDResource); + +struct _virPCIVPDStringResourceClass { + virPCIVPDResourceClass parent_class; +}; + +virPCIVPDStringResource *virPCIVPDStringResourceNew(gchar *value); + +const gchar *virPCIVPDStringResourceGetValue(const virPCIVPDStringResource *res); + +#define VIR_TYPE_PCI_VPD_KEYWORD_RESOURCE vir_pci_vpd_keyword_resource_get_type() +G_DECLARE_FINAL_TYPE(virPCIVPDKeywordResource, vir_pci_vpd_keyword_resource, VIR, + PCI_VPD_KEYWORD_RESOURCE, virPCIVPDResource); +virPCIVPDKeywordResource *virPCIVPDKeywordResourceNew(GTree *resourceFields, bool readOnly); + +void virPCIVPDKeywordResourceForEach(virPCIVPDKeywordResource *res, GTraverseFunc func, + gpointer userData); + + +GList *virPCIVPDParse(int vpdFileFd); + +G_END_DECLS diff --git a/src/util/virpcivpdpriv.h b/src/util/virpcivpdpriv.h new file mode 100644 index 0000000000..9a3cc075c1 --- /dev/null +++ b/src/util/virpcivpdpriv.h @@ -0,0 +1,45 @@ +/* + * virpcivpdpriv.h: helper APIs for working with the PCI/PCIe VPD capability + * + * Copyright (C) 2021 Canonical Ltd. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + */ + +#ifndef LIBVIRT_VIRPCIVPDPRIV_H_ALLOW +# error "virpcivpdpriv.h may only be included by virpcivpd.c or test suites" +#endif /* LIBVIRT_VIRPCIVPDPRIV_H_ALLOW */ + +#pragma once + +#include "virpcivpd.h" + +#ifdef __linux__ + +size_t +virPCIVPDReadVPDBytes(int vpdFileFd, uint8_t *buf, size_t count, off_t offset, uint8_t *csum); + +virPCIVPDKeywordResource *virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, + uint16_t resDataLen, + bool readOnly, uint8_t *csum); + +virPCIVPDStringResource *virPCIVPDParseVPDLargeResourceString(int vpdFileFd, uint16_t resPos, + uint16_t resDataLen, + uint8_t *csum); + +#endif /* __linux__ */ + +gboolean virPCIVPDResourceIsExpectedKeyword(const gchar *keyword, gboolean readOnly); +gboolean virPCIVPDResourceIsValidTextValue(const gchar *value); diff --git a/tests/meson.build b/tests/meson.build index dfbc2c01e2..1948c07ae3 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -336,6 +336,7 @@ tests += [ { 'name': 'virtimetest' }, { 'name': 'virtypedparamtest' }, { 'name': 'viruritest' }, + { 'name': 'virpcivpdtest' }, { 'name': 'vshtabletest', 'link_with': [ libvirt_shell_lib ] }, { 'name': 'virmigtest' }, ] diff --git a/tests/testutils.c b/tests/testutils.c index d071abd6d7..35d9d0645d 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -1143,3 +1143,43 @@ virTestStablePath(const char *path) return g_strdup(path); } + +#ifdef __linux__ +/** + * virCreateAnonymousFile: + * @data: a pointer to data to be written into a new file. + * @len: the length of data to be written (in bytes). + * + * Create a fake fd, write initial data to it. + * + */ +int +virCreateAnonymousFile(const uint8_t *data, size_t len) +{ + int fd = -1; + char path[] = abs_builddir "testutils-memfd-XXXXXX"; + /* A temp file is used since not all supported distributions support memfd. */ + if ((fd = g_mkstemp_full(path, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) { + return fd; + } + g_unlink(path); + + if (ftruncate(fd, len) != 0) { + VIR_TEST_DEBUG("%s: %s", "failed to ftruncate an anonymous file", + g_strerror(errno)); + goto cleanup; + } + if (safewrite(fd, data, len) != len) { + VIR_TEST_DEBUG("%s: %s", "failed to write to an anonymous file", + g_strerror(errno)); + goto cleanup; + } + return fd; + cleanup: + if (VIR_CLOSE(fd) < 0) { + VIR_TEST_DEBUG("%s: %s", "failed to close an anonymous file", + g_strerror(errno)); + } + return -1; +} +#endif diff --git a/tests/testutils.h b/tests/testutils.h index 27d135fc02..13a154a5af 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -173,3 +173,7 @@ int testCompareDomXML2XMLFiles(virCaps *caps, char * virTestStablePath(const char *path); + +#ifdef __linux__ +int virCreateAnonymousFile(const uint8_t *data, size_t len); +#endif diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c new file mode 100644 index 0000000000..09cba4b1a3 --- /dev/null +++ b/tests/virpcivpdtest.c @@ -0,0 +1,705 @@ +/* + * Copyright (C) 2021 Canonical Ltd. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "internal.h" +#include "testutils.h" +#include "virpcivpd.h" + +#define LIBVIRT_VIRPCIVPDPRIV_H_ALLOW + +#include "virpcivpdpriv.h" +#include "virlog.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +#ifdef __linux__ + +VIR_LOG_INIT("tests.vpdtest"); + + +static int +testPCIVPDStringResourceBasic(const void *data G_GNUC_UNUSED) +{ + g_autoptr(virPCIVPDStringResource) strRes = NULL; + g_autofree char *val = g_strdup("testval"); + GEnumValue *resType = NULL; + + strRes = virPCIVPDStringResourceNew(g_steal_pointer(&val)); + + resType = virPCIVPDResourceGetResourceType((virPCIVPDResource *)strRes); + + if (resType->value != VIR_PCI_VPD_RESOURCE_TYPE_STRING) { + VIR_DEBUG("Expected '%d' got '%d'", VIR_PCI_VPD_RESOURCE_TYPE_STRING, resType->value); + return -1; + } + if (!STREQ_NULLABLE(virPCIVPDStringResourceGetValue(strRes), "testval")) { + return -1; + } + + return 0; +} + +typedef struct _TestPCIVPDKeyValue { + const char *keyword; + const char *value; +} TestPCIVPDKeyValue; + +static gboolean +testPCIVPDGetOneField(gpointer *key, gpointer *val, gpointer userData) +{ + TestPCIVPDKeyValue *res = userData; + + res->keyword = (const char *)key; + res->value = (const char *)val; + + return true; +} + +static int +testPCIVPDKeywordResourceBasic(const void *data G_GNUC_UNUSED) +{ + g_autoptr(virPCIVPDKeywordResource) kwRes = NULL; + g_autoptr(GTree) resFieldTree = g_tree_new_full((GCompareDataFunc)g_strcmp0, NULL, + g_free, g_free); + GEnumValue *resType = NULL; + TestPCIVPDKeyValue testKeyVal; + + g_tree_insert(resFieldTree, g_strdup("SN"), g_strdup("DEADBEEFCAFE")); + + kwRes = virPCIVPDKeywordResourceNew(g_steal_pointer(&resFieldTree), true); + resType = virPCIVPDResourceGetResourceType((virPCIVPDResource *)kwRes); + + if (resType->value != VIR_PCI_VPD_RESOURCE_TYPE_VPD_R) { + return -1; + } + + virPCIVPDKeywordResourceForEach(kwRes, (GTraverseFunc) testPCIVPDGetOneField, &testKeyVal); + if (STRNEQ_NULLABLE(testKeyVal.keyword, "SN") || + STRNEQ_NULLABLE(testKeyVal.value, "DEADBEEFCAFE")) { + VIR_DEBUG("Unexpected keyword resource field encountered: %s: %s; expected: %s: %s", + testKeyVal.keyword, testKeyVal.value, "SN", "DEADBEEFCAFE"); + return -1; + } + + g_object_unref(g_steal_pointer(&kwRes)); + + resFieldTree = g_tree_new_full((GCompareDataFunc)g_strcmp0, NULL, g_free, g_free); + g_tree_insert(resFieldTree, g_strdup("SN"), g_strdup("CAFEDEADBEEF")); + + kwRes = virPCIVPDKeywordResourceNew(g_steal_pointer(&resFieldTree), false); + resType = virPCIVPDResourceGetResourceType((virPCIVPDResource *)kwRes); + + if (resType->value != VIR_PCI_VPD_RESOURCE_TYPE_VPD_W) { + return -1; + } + + virPCIVPDKeywordResourceForEach(kwRes, (GTraverseFunc) testPCIVPDGetOneField, &testKeyVal); + if (STRNEQ_NULLABLE(testKeyVal.keyword, "SN") || + STRNEQ_NULLABLE(testKeyVal.value, "CAFEDEADBEEF")) { + VIR_DEBUG("Unexpected keyword resource field encountered: %s: %s; expected: %s: %s", + testKeyVal.keyword, testKeyVal.value, "SN", "DEADBEEFCAFE"); + return -1; + } + + return 0; +} + +typedef struct _TestPCIVPDExpectedString { + const char *keyword; + gboolean expected; +} TestPCIVPDExpectedString; + +/* + * testPCIVPDIsExpectedKeywordReadOnly: + * + * Test expected keyword validation. Static metadata about expected + * keywords is taken from the PCI(e) standards should be respected + * and only keywords known to have a given type need to be returned. + * */ +static int +testPCIVPDIsExpectedKeywordReadOnly(const void *data G_GNUC_UNUSED) +{ + size_t i = 0; + + const TestPCIVPDExpectedString readOnlyCases[] = { + {"CP", true}, + {"EC", true}, + {"FG", true}, + {"LC", true}, + {"MN", true}, + {"PG", true}, + {"PN", true}, + {"RV", true}, + {"SN", true}, + {"V0", true}, + {"VG", true}, + {"IV", false}, + {"YA", false}, + {"YB", false}, + {"RW", false}, + /* Empty: */ + {"", false}, + /* 1-byte: */ + {"Y", false}, + /* 3-byte: */ + {"FOO", false}, + /* Not present in the spec: */ + {"42", false}, + {"4A", false}, + }; + for (i = 0; i < sizeof(readOnlyCases) / sizeof(readOnlyCases[0]); ++i) { + if (virPCIVPDResourceIsExpectedKeyword(readOnlyCases[i].keyword, true) != + readOnlyCases[i].expected) { + return -1; + } + } + return 0; +} + +/* + * testPCIVPDIsExpectedKeywordReadWrite: + * + * Test expected keyword validation. Static metadata about expected + * keywords is taken from the PCI(e) standards should be respected + * and only keywords known to have a given type need to be returned. + * */ +static int +testPCIVPDIsExpectedKeywordReadWrite(const void *data G_GNUC_UNUSED) +{ + size_t i = 0; + + const TestPCIVPDExpectedString readWriteCases[] = { + {"CP", false}, + {"EC", false}, + {"FG", false}, + {"LC", false}, + {"MN", false}, + {"PG", false}, + {"PN", false}, + {"RV", false}, + {"SN", false}, + {"V0", true}, + {"VG", true}, + {"IV", false}, + {"YA", true}, + {"YB", true}, + {"RW", true}, + /* Empty: */ + {"", false}, + /* 1-byte: */ + {"Y", false}, + /* 3-byte: */ + {"FOO", false}, + /* Not present in the spec: */ + {"42", false}, + {"4A", false}, + }; + for (i = 0; i < sizeof(readWriteCases) / sizeof(readWriteCases[0]); ++i) { + if (virPCIVPDResourceIsExpectedKeyword(readWriteCases[i].keyword, false) != + readWriteCases[i].expected) { + return -1; + } + } + return 0; +} + + + +/* + * testPCIVPDIsValidTextValue: + * + * Test expected text value validation. Static metadata about possible values is taken + * from the PCI(e) standards and based on some real-world hardware examples. + * */ +static int +testPCIVPDIsValidTextValue(const void *data G_GNUC_UNUSED) +{ + size_t i = 0; + + const TestPCIVPDExpectedString textValueCases[] = { + /* Numbers */ + {"42", true}, + /* Alphanumeric */ + {"DCM1001008FC52101008FC53201008FC54301008FC5", true}, + /* Dots */ + {"DSV1028VPDR.VER1.0", true}, + /* Whitespace presence */ + {"NMVIntel Corp", true}, + /* Comma and spaces */ + {"BlueField-2 DPU 25GbE Dual-Port SFP56, Tall Bracket", true}, + /* Equal signs and colons. */ + {"MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=BF2H332A", true}, + /* Dashes */ + {"MBF2H332A-AEEOT", true}, + {"under_score_example", true}, + {"", true}, + {";", false}, + {"\\42", false}, + {"/42", false}, + }; + for (i = 0; i < sizeof(textValueCases) / sizeof(textValueCases[0]); ++i) { + if (virPCIVPDResourceIsValidTextValue(textValueCases[i].keyword) != + textValueCases[i].expected) { + return -1; + } + } + return 0; +} + +/* + * testPCIVPDGetFieldValueFormat: + * + * A simple test to assess the functionality of the + * virPCIVPDResourceGetFieldValueFormat function. + * */ +static int +testPCIVPDGetFieldValueFormat(const void *data G_GNUC_UNUSED) +{ + typedef struct _TestPCIVPDExpectedFieldValueFormat { + const char *keyword; + virPCIVPDResourceFieldValueFormat expected; + } TestPCIVPDExpectedFieldValueFormat; + + size_t i = 0; + + const TestPCIVPDExpectedFieldValueFormat valueFormatCases[] = { + {"SN", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT}, + {"RV", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD}, + {"RW", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR}, + {"VA", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT}, + {"YA", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT}, + {"YZ", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT}, + {"CP", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_BINARY}, + /* Invalid keywords. */ + {"", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST}, + {"4", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST}, + {"Y", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST}, + {"V", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST}, + /* 2 bytes but not present in the spec. */ + {"EX", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST}, + /* Many numeric bytes. */ + {"4242", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST}, + /* Many letters. */ + {"EXAMPLE", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST}, + }; + for (i = 0; i < sizeof(valueFormatCases) / sizeof(valueFormatCases[0]); ++i) { + if (virPCIVPDResourceGetFieldValueFormat(valueFormatCases[i].keyword) != + valueFormatCases[i].expected) { + return -1; + } + } + return 0; +} + +# define VPD_STRING_RESOURCE_EXAMPLE_HEADER \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_STRING_RESOURCE_FLAG, 0x08, 0x00 + +# define VPD_STRING_RESOURCE_EXAMPLE_DATA \ + 't', 'e', 's', 't', 'n', 'a', 'm', 'e' + +# define VPD_R_FIELDS_EXAMPLE_HEADER \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x16, 0x00 + +# define VPD_R_EXAMPLE_VALID_RV_FIELD \ + 'R', 'V', 0x02, 0x31, 0x00 + +# define VPD_R_EXAMPLE_INVALID_RV_FIELD \ + 'R', 'V', 0x02, 0xFF, 0x00 + +# define VPD_R_EXAMPLE_FIELDS \ + 'P', 'N', 0x02, '4', '2', \ + 'E', 'C', 0x04, '4', '2', '4', '2', \ + 'V', 'A', 0x02, 'E', 'X' + +# define VPD_R_FIELDS_EXAMPLE_DATA \ + VPD_R_EXAMPLE_FIELDS, \ + VPD_R_EXAMPLE_VALID_RV_FIELD + +# define VPD_W_FIELDS_EXAMPLE_HEADER \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG, 0x19, 0x00 + +# define VPD_W_EXAMPLE_FIELDS \ + 'V', 'Z', 0x02, '4', '2', \ + 'Y', 'A', 0x04, 'I', 'D', '4', '2', \ + 'Y', 'F', 0x02, 'E', 'X', \ + 'Y', 'E', 0x00, \ + 'R', 'W', 0x02, 0x00, 0x00 + +static int +testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) +{ + int fd = -1; + g_autofree uint8_t *buf = NULL; + uint8_t csum = 0; + size_t readBytes = 0; + size_t dataLen = 0; + + /* An example of a valid VPD record with one VPD-R resource and 2 fields. */ + uint8_t fullVPDExample[] = { + VPD_STRING_RESOURCE_EXAMPLE_HEADER, VPD_STRING_RESOURCE_EXAMPLE_DATA, + VPD_R_FIELDS_EXAMPLE_HEADER, VPD_R_FIELDS_EXAMPLE_DATA, + PCI_VPD_RESOURCE_END_VAL + }; + dataLen = sizeof(fullVPDExample) / sizeof(uint8_t) - 2; + buf = g_malloc0(dataLen); + + fd = virCreateAnonymousFile(fullVPDExample, dataLen); + + readBytes = virPCIVPDReadVPDBytes(fd, buf, dataLen, 0, &csum); + + if (readBytes != dataLen) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "The number of bytes read %zu is lower than expected %zu ", + readBytes, dataLen); + return -1; + } + + if (csum) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "The sum of all VPD bytes up to and including the checksum byte" + "is equal to zero: 0x%02x", csum); + return -1; + } + return 0; +} + +static int +testVirPCIVPDParseVPDStringResource(const void *opaque G_GNUC_UNUSED) +{ + int fd = -1; + uint8_t csum = 0; + size_t dataLen = 0; + + g_autoptr(virPCIVPDStringResource) strRes = NULL; + const gchar *expectedValue = "testname", *actualValue = NULL; + + const uint8_t stringResExample[] = { + VPD_STRING_RESOURCE_EXAMPLE_DATA + }; + + dataLen = sizeof(stringResExample) / sizeof(uint8_t); + fd = virCreateAnonymousFile(stringResExample, dataLen); + strRes = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, &csum); + VIR_FORCE_CLOSE(fd); + + actualValue = virPCIVPDStringResourceGetValue(strRes); + if (STRNEQ(expectedValue, actualValue)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Unexpected string resource value: %s, expected: %s", + actualValue, expectedValue); + return -1; + } + return 0; +} + + +typedef struct _TestPCIVPDExpectedFieldsData { + GHashTable *expectedFields; + size_t field_count; +} TestPCIVPDExpectedFieldsData; + +static gboolean +testPCIVPDValidateField(gpointer *key, gpointer *val, gpointer userData) +{ + TestPCIVPDExpectedFieldsData *expectedData = userData; + const char *actualKey = (const char *)key; + const char *expectedValue = NULL, *actualValue = (const char *)val; + + expectedValue = g_hash_table_lookup(expectedData->expectedFields, (char *)key); + if (!expectedValue) { + virReportError(VIR_ERR_INTERNAL_ERROR, "Unexpected key encountered: %s", actualKey); + return true; + } else if (STRNEQ(expectedValue, actualValue)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Unexpected value for key %s encountered: %s expected %s", actualKey, + actualValue, expectedValue); + return true; + } + expectedData->field_count++; + return false; +} + +static gboolean +testVirPCIVPDValidateKeywordResource(virPCIVPDKeywordResource *kwRes, + GHashTable *expectedFields) +{ + size_t expectedCount = g_hash_table_size(expectedFields); + TestPCIVPDExpectedFieldsData expectedFieldsData; + + expectedFieldsData.expectedFields = expectedFields; + expectedFieldsData.field_count = 0; + + virPCIVPDKeywordResourceForEach(kwRes, (GTraverseFunc)testPCIVPDValidateField, + &expectedFieldsData); + /* + * Check that the number of actual fields observed equals the expected number + * which also validates that the "RV" field is not present after parsing. + */ + if (expectedFieldsData.field_count != expectedCount) { + virReportError(VIR_ERR_INTERNAL_ERROR, "Unexpected number of fields observed: %zu vs %zu", + expectedFieldsData.field_count, expectedCount); + return false; + } + return true; +} + + +static int +testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED) +{ + int fd = -1; + const int EXPECTED_RES_LIST_LENGTH = 3; + size_t dataLen = 0; + + g_autolist(virPCIVPDResource) resList = NULL; + GList *listIter = NULL; + virPCIVPDResource *res = NULL; + + const uint8_t fullVPDExample[] = { + VPD_STRING_RESOURCE_EXAMPLE_HEADER, VPD_STRING_RESOURCE_EXAMPLE_DATA, + VPD_R_FIELDS_EXAMPLE_HEADER, VPD_R_FIELDS_EXAMPLE_DATA, + VPD_W_FIELDS_EXAMPLE_HEADER, VPD_W_EXAMPLE_FIELDS, + PCI_VPD_RESOURCE_END_VAL + }; + const gchar *expectedValue = "testname", *actualValue = NULL; + + g_autoptr(GHashTable) expectedReadOnlyKw = NULL, expectedReadWriteKw = NULL; + + expectedReadOnlyKw = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, g_free); + g_hash_table_insert(expectedReadOnlyKw, g_strdup("PN"), g_strdup("42")); + g_hash_table_insert(expectedReadOnlyKw, g_strdup("EC"), g_strdup("4242")); + g_hash_table_insert(expectedReadOnlyKw, g_strdup("VA"), g_strdup("EX")); + + expectedReadWriteKw = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, g_free); + g_hash_table_insert(expectedReadWriteKw, g_strdup("VZ"), g_strdup("42")); + g_hash_table_insert(expectedReadWriteKw, g_strdup("YA"), g_strdup("ID42")); + g_hash_table_insert(expectedReadWriteKw, g_strdup("YF"), g_strdup("EX")); + g_hash_table_insert(expectedReadWriteKw, g_strdup("YE"), g_strdup("")); + + dataLen = sizeof(fullVPDExample) / sizeof(uint8_t); + fd = virCreateAnonymousFile(fullVPDExample, dataLen); + resList = virPCIVPDParse(fd); + VIR_FORCE_CLOSE(fd); + + if (!resList) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "The resource list is empty after parsing which is unexpected"); + return -1; + } else if (g_list_length(resList) != EXPECTED_RES_LIST_LENGTH) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "The resource list length is not equal to: %d", EXPECTED_RES_LIST_LENGTH); + return -1; + } + + for (listIter = resList; listIter; listIter = g_list_next(listIter)) { + res = listIter->data; + switch (virPCIVPDResourceGetResourceType(res)->value) { + case VIR_PCI_VPD_RESOURCE_TYPE_STRING: + actualValue = virPCIVPDStringResourceGetValue((virPCIVPDStringResource *)res); + if (STRNEQ(expectedValue, actualValue)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Unexpected string resource value: %s, expected: %s", + actualValue, expectedValue); + return -1; + } + break; + case VIR_PCI_VPD_RESOURCE_TYPE_VPD_R: + if (!testVirPCIVPDValidateKeywordResource((virPCIVPDKeywordResource *)res, + expectedReadOnlyKw)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Keyword resource fields do not match the expected ones"); + return -1; + } + break; + case VIR_PCI_VPD_RESOURCE_TYPE_VPD_W: + if (!testVirPCIVPDValidateKeywordResource((virPCIVPDKeywordResource *)res, + expectedReadWriteKw)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Keyword resource fields do not match the expected ones"); + return -1; + } + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", "unexpected resource type encountered"); + return -1; + } + } + return 0; +} + +static int +testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) +{ + int fd = -1; + size_t dataLen = 0; + +# define VPD_INVALID_ZERO_BYTE \ + 0x00 + +# define VPD_INVALID_STRING_HEADER_DATA_LONG \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_STRING_RESOURCE_FLAG, 0x04, 0x00, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x05, 0x00, \ + 'R', 'V', 0x02, 0xDA, 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_INVALID_STRING_HEADER_DATA_SHORT \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_STRING_RESOURCE_FLAG, 0x0A, 0x00, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x05, 0x00, \ + 'R', 'V', 0x02, 0xD4, 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_NO_VPD_R \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_R_NO_RV \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + VPD_R_FIELDS_EXAMPLE_HEADER, \ + VPD_R_EXAMPLE_FIELDS, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_R_INVALID_RV \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + VPD_R_FIELDS_EXAMPLE_HEADER, \ + VPD_R_EXAMPLE_FIELDS, \ + VPD_R_EXAMPLE_INVALID_RV_FIELD, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_R_INVALID_RV_ZERO_LENGTH \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x14, 0x00, \ + VPD_R_EXAMPLE_FIELDS, \ + 'R', 'V', 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +/* The RW key is not expected in a VPD-R record. */ +# define VPD_R_UNEXPECTED_KEY \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x1B, 0x00, \ + VPD_R_EXAMPLE_FIELDS, \ + 'R', 'W', 0x02, 0x00, 0x00, \ + 'R', 'V', 0x02, 0x8F, 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_R_INVALID_KEY_FIRST_BYTE \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x1B, 0x00, \ + VPD_R_EXAMPLE_FIELDS, \ + 0x07, 'A', 0x02, 0x00, 0x00, \ + 'R', 'V', 0x02, 0xE2, 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_R_INVALID_KEY_SECOND_BYTE \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x1B, 0x00, \ + VPD_R_EXAMPLE_FIELDS, \ + 'V', 0x07, 0x02, 0x00, 0x00, \ + 'R', 'V', 0x02, 0xCD, 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_R_INVALID_FIELD_VALUE \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + VPD_STRING_RESOURCE_EXAMPLE_DATA, \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x0A, 0x00, \ + 'S', 'N', 0x02, 0x04, 0x02, \ + 'R', 'V', 0x02, 0x28, 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +# define VPD_INVALID_STRING_RESOURCE_VALUE \ + VPD_STRING_RESOURCE_EXAMPLE_HEADER, \ + 't', 0x03, 's', 't', 'n', 'a', 'm', 'e', \ + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x0A, 0x00, \ + 'S', 'N', 0x02, 0x04, 0x02, \ + 'R', 'V', 0x02, 0x8A, 0x00, \ + PCI_VPD_RESOURCE_END_VAL + +# define TEST_INVALID_VPD(invalidVPD) \ + do { \ + const uint8_t testCase[] = { invalidVPD }; \ + dataLen = sizeof(testCase) / sizeof(uint8_t); \ + fd = virCreateAnonymousFile(testCase, dataLen); \ + if (virPCIVPDParse(fd)) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + "Successfully parsed an invalid VPD - this is not expected"); \ + return -1; \ + } \ + VIR_FORCE_CLOSE(fd); \ + } while (0); + + TEST_INVALID_VPD(VPD_INVALID_ZERO_BYTE); + TEST_INVALID_VPD(VPD_INVALID_STRING_HEADER_DATA_SHORT); + TEST_INVALID_VPD(VPD_INVALID_STRING_HEADER_DATA_LONG); + TEST_INVALID_VPD(VPD_NO_VPD_R); + TEST_INVALID_VPD(VPD_R_NO_RV); + TEST_INVALID_VPD(VPD_R_INVALID_RV); + TEST_INVALID_VPD(VPD_R_INVALID_RV_ZERO_LENGTH); + TEST_INVALID_VPD(VPD_R_UNEXPECTED_KEY); + TEST_INVALID_VPD(VPD_R_INVALID_KEY_FIRST_BYTE); + TEST_INVALID_VPD(VPD_R_INVALID_KEY_SECOND_BYTE); + TEST_INVALID_VPD(VPD_R_INVALID_FIELD_VALUE); + TEST_INVALID_VPD(VPD_INVALID_STRING_RESOURCE_VALUE); + + return 0; +} + +static int +mymain(void) +{ + int ret = 0; + + if (virTestRun("String resource (basic test) ", testPCIVPDStringResourceBasic, NULL) < 0) + ret = -1; + if (virTestRun("Keyword resource (basic test) ", testPCIVPDKeywordResourceBasic, NULL) < 0) + ret = -1; + if (virTestRun("Expected read-only resource keywords ", testPCIVPDIsExpectedKeywordReadOnly, + NULL) < 0) + ret = -1; + if (virTestRun("Expected read-write resource keywords ", testPCIVPDIsExpectedKeywordReadWrite, + NULL) < 0) + ret = -1; + if (virTestRun("Valid text values ", testPCIVPDIsValidTextValue, NULL) < 0) + ret = -1; + if (virTestRun("Determining a field value format by a key ", + testPCIVPDGetFieldValueFormat, NULL) < 0) + ret = -1; + if (virTestRun("Reading VPD bytes ", testVirPCIVPDReadVPDBytes, NULL) < 0) + ret = -1; + if (virTestRun("Parsing VPD string resources ", testVirPCIVPDParseVPDStringResource, NULL) < 0) + ret = -1; + if (virTestRun("Parsing VPD resources from a full VPD ", testVirPCIVPDParseFullVPD, NULL) < 0) + ret = -1; + if (virTestRun("Parsing invalid VPD records ", testVirPCIVPDParseFullVPDInvalid, NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) +#endif -- 2.30.2

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.shcherbakov@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
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index cb54c8ba36..a428831380 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -775,9 +775,9 @@ sc_prohibit_windows_special_chars_in_filename: { echo '$(ME): Windows special chars in filename not allowed' 1>&2; echo exit 1; } || :
sc_prohibit_mixed_case_abbreviations: - @prohibit='Pci|Usb|Scsi' \ + @prohibit='Pci|Usb|Scsi|Vpd' \ in_vc_files='\.[ch]$$' \ - halt='Use PCI, USB, SCSI, not Pci, Usb, Scsi' \ + halt='Use PCI, USB, SCSI, VPD, not Pci, Usb, Scsi, Vpd' \ $(_sc_search_regexp)
# Require #include <locale.h> in all files that call setlocale() diff --git a/po/POTFILES.in b/po/POTFILES.in index c200d7452a..4be4139529 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -302,6 +302,7 @@ @SRCDIR@src/util/virnvme.c @SRCDIR@src/util/virobject.c @SRCDIR@src/util/virpci.c +@SRCDIR@src/util/virpcivpd.c @SRCDIR@src/util/virperf.c @SRCDIR@src/util/virpidfile.c @SRCDIR@src/util/virpolkit.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6de9d9aef1..bb9b380599 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3571,6 +3571,21 @@ virVHBAManageVport; virVHBAPathExists;
+# util/virpcivpd.h + +virPCIVPDKeywordResourceForEach; +virPCIVPDKeywordResourceNew; +virPCIVPDParse; +virPCIVPDParseVPDLargeResourceFields; +virPCIVPDParseVPDLargeResourceString; +virPCIVPDReadVPDBytes; +virPCIVPDResourceGetFieldValueFormat; +virPCIVPDResourceGetResourceType; +virPCIVPDResourceIsExpectedKeyword; +virPCIVPDResourceIsValidTextValue; +virPCIVPDStringResourceGetValue; +virPCIVPDStringResourceNew; + # util/virvsock.h virVsockAcquireGuestCid; virVsockSetGuestCid; diff --git a/src/util/meson.build b/src/util/meson.build index 05934f6841..24350a3e67 100644 --- a/src/util/meson.build +++ b/src/util/meson.build @@ -105,6 +105,7 @@ util_sources = [ 'virutil.c', 'viruuid.c', 'virvhba.c', + 'virpcivpd.c', 'virvsock.c', 'virxml.c', ] diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c new file mode 100644 index 0000000000..849ea0570c --- /dev/null +++ b/src/util/virpcivpd.c @@ -0,0 +1,755 @@ +/* + * virpcivpd.c: helper APIs for working with the PCI/PCIe VPD capability + * + * Copyright (C) 2021 Canonical Ltd. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#ifdef __linux__ +# include <unistd.h> +#endif + +#define LIBVIRT_VIRPCIVPDPRIV_H_ALLOW + +#include "virpcivpdpriv.h" +#include "virlog.h" +#include "virerror.h" +#include "virfile.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.pcivpd"); + +GType +vir_pci_vpd_resource_type_get_type(void) +{ + static GType resourceType; + + static const GEnumValue resourceTypes[] = { + {VIR_PCI_VPD_RESOURCE_TYPE_STRING, "String resource", "string"}, + {VIR_PCI_VPD_RESOURCE_TYPE_VPD_R, "Read-only resource", "vpd-r"}, + {VIR_PCI_VPD_RESOURCE_TYPE_VPD_W, "Read-write resource", "vpd-w"}, + {VIR_PCI_VPD_RESOURCE_TYPE_LAST, "last", "last"}, + {0, NULL, NULL}, + }; + + if (!resourceType) { + resourceType = g_enum_register_static("virPCIVPDResourceType", resourceTypes); + } + return resourceType; +} + +static gboolean +virPCIVPDResourceIsVendorKeyword(const gchar *keyword) +{ + return g_str_has_prefix(keyword, "V"); +} + +static gboolean +virPCIVPDResourceIsSystemKeyword(const gchar *keyword) +{ + /* Special-case the system-specific keywords since they share the "Y" prefix with "YA". */ + return g_str_has_prefix(keyword, "Y") && STRNEQ(keyword, "YA"); +} + +static gchar * +virPCIVPDResourceGetKeywordPrefix(const gchar *keyword) +{ + g_autofree gchar *key = NULL; + + /* Keywords must have a length of 2 bytes. */ + if (strlen(keyword) != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("The keyword length is not 2 bytes: %s"), keyword); + return NULL; + } else if (!(g_ascii_isalnum(keyword[0]) && g_ascii_isalnum(keyword[1]))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The keyword contains non-alphanumeric ASCII characters")); + return NULL; + } + /* Special-case the system-specific keywords since they share the "Y" prefix with "YA". */ + if (virPCIVPDResourceIsSystemKeyword(keyword) || virPCIVPDResourceIsVendorKeyword(keyword)) { + key = g_strndup(keyword, 1); + } else { + key = g_strndup(keyword, 2); + } + return g_steal_pointer(&key); +} + +/** + * virPCIVPDResourceGetFieldValueFormat: + * @keyword: A keyword for which to get a value type + * + * Returns: a virPCIVPDResourceFieldValueFormat value which specifies the field value type for + * a provided keyword based on the static information from PCI(e) specs. + */ +virPCIVPDResourceFieldValueFormat +virPCIVPDResourceGetFieldValueFormat(const gchar *keyword) +{ + static GHashTable *fieldValueFormats; + g_autofree gchar *key = NULL; + gpointer keyVal = NULL; + virPCIVPDResourceFieldValueFormat format = VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST; + + /* Keywords are expected to be 2 bytes in length which is defined in the specs. */ + if (strlen(keyword) != 2) { + return VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST; + } + + if (!fieldValueFormats) {
I don't know what the thread concurrency rules are of the callers of this code, but regardless we generally aim to make any one-time initializers thread safe by default. Recommend using VIR_ONCE_GLOBAL_INIT to do one-time init.
+ /* Initialize a hash table once with static format metadata coming from the PCI(e) specs. + * The VPD format does not embed format metadata into the resource records so it is not + * possible to do format discovery without static information. Legacy PICMIG keywords + * are not included. */ + fieldValueFormats = g_hash_table_new(g_str_hash, g_str_equal); + /* Extended capability. Contains binary data per PCI(e) specs. */ + g_hash_table_insert(fieldValueFormats, g_strdup("CP"),
IIUC, this hash table is created once and never changed. I'm not seeing a clear need for g_strdup here. Can't we just directly use the constant string as the key ?
+ GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_BINARY)); + /* Engineering Change Level of an Add-in Card. */ + g_hash_table_insert(fieldValueFormats, g_strdup("EC"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* Manufacture ID */ + g_hash_table_insert(fieldValueFormats, g_strdup("MN"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* Add-in Card Part Number */ + g_hash_table_insert(fieldValueFormats, g_strdup("PN"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* Checksum and Reserved */ + g_hash_table_insert(fieldValueFormats, g_strdup("RV"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD)); + /* Remaining Read/Write Area */ + g_hash_table_insert(fieldValueFormats, g_strdup("RW"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR)); + /* Serial Number */ + g_hash_table_insert(fieldValueFormats, g_strdup("SN"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* Asset Tag Identifier */ + g_hash_table_insert(fieldValueFormats, g_strdup("YA"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* This is a vendor specific item and the characters are alphanumeric. The second + * character (x) of the keyword can be 0 through Z so only the first one is stored. */ + g_hash_table_insert(fieldValueFormats, g_strdup("V"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + /* This is a system specific item and the characters are alphanumeric. + * The second character (x) of the keyword can be 0 through 9 and B through Z. */ + g_hash_table_insert(fieldValueFormats, g_strdup("Y"), + GINT_TO_POINTER(VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT)); + } + + /* The system and vendor-specific keywords have a variable part - lookup + * the prefix significant for determining the value format. */ + key = virPCIVPDResourceGetKeywordPrefix(keyword); + if (key) { + keyVal = g_hash_table_lookup(fieldValueFormats, key); + if (keyVal) { + format = GPOINTER_TO_INT(keyVal); + } + } + return format; +}
+/** + * virPCIVPDResourceIsValidTextValue: + * @value: A NULL-terminated string to assess. + * + * Returns: a boolean indicating whether this value is a valid string resource + * value or text field value. The expectations are based on the keywords specified + * in relevant sections of PCI(e) specifications + * ("I.3. VPD Definitions" in PCI specs, "6.28.1 VPD Format" PCIe 4.0). + */ +gboolean +virPCIVPDResourceIsValidTextValue(const gchar *value) +{ + /* + * The PCI(e) specs mention alphanumeric characters when talking about text fields + * and the string resource but also include spaces and dashes in the provided example. + * Dots, commas, equal signs have also been observed in values used by major device vendors. + * The specs do not specify a full set of allowed code points and for Libvirt it is important + * to keep values in the ranges allowed within XML elements (mainly excluding less-than, + * greater-than and ampersand). + */ + if (!g_regex_match_simple("^[a-zA-Z0-9\\-_\\s,.:=]*$", value, 0, 0)) {
Since you're only trying to validate a set of permitted characters, it is sufficient to use strspn() for this task. eg what we do in vsh.c is /* Characters permitted in $EDITOR environment variable and temp filename. */ #define ACCEPTED_CHARS \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-/_.:@" if (strspn(editor, ACCEPTED_CHARS) != strlen(editor)) { ....error...
+/* + * PCI Local bus (2.2+, Appendix I) and PCIe 4.0+ (7.9.19 VPD Capability) define + * the VPD capability structure (8 bytes in total) and VPD registers that can be used to access + * VPD data including: + * bit 31 of the first 32-bit DWORD: data transfer completion flag (between the VPD data register + * and the VPD data storage hardware); + * bits 30:16 of the first 32-bit DWORD: VPD address of the first VPD data byte to be accessed; + * bits 31:0 of the second 32-bit DWORD: VPD data bytes with LSB being pointed to by the VPD address. + * + * Given that only 15 bits (30:16) are allocated for VPD address its mask is 0x7fff. +*/ +#define PCI_VPD_ADDR_MASK 0x7FFF + +/* + * VPD data consists of small and large resource data types. Information within a resource type + * consists of a 2-byte keyword, 1-byte length and data bytes (up to 255). +*/ +#define PCI_VPD_MAX_FIELD_SIZE 255 +#define PCI_VPD_LARGE_RESOURCE_FLAG 0x80 +#define PCI_VPD_STRING_RESOURCE_FLAG 0x02 +#define PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG 0x10 +#define PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG 0x11 +#define PCI_VPD_RESOURCE_END_TAG 0x0F +#define PCI_VPD_RESOURCE_END_VAL PCI_VPD_RESOURCE_END_TAG << 3 +#define VIR_TYPE_PCI_VPD_RESOURCE_TYPE vir_pci_vpd_resource_type_get_type() + +G_BEGIN_DECLS;
This is not required in libvirt internal code, since we never use C++ internally.
+#ifdef __linux__ +/** + * virCreateAnonymousFile: + * @data: a pointer to data to be written into a new file. + * @len: the length of data to be written (in bytes). + * + * Create a fake fd, write initial data to it. + * + */ +int +virCreateAnonymousFile(const uint8_t *data, size_t len) +{ + int fd = -1; + char path[] = abs_builddir "testutils-memfd-XXXXXX"; + /* A temp file is used since not all supported distributions support memfd. */ + if ((fd = g_mkstemp_full(path, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) { + return fd; + } + g_unlink(path); + + if (ftruncate(fd, len) != 0) { + VIR_TEST_DEBUG("%s: %s", "failed to ftruncate an anonymous file", + g_strerror(errno)); + goto cleanup; + }
Since it is a new temporary file it is guaranteed zero length, so this should be redundant AFAICT.
+ if (safewrite(fd, data, len) != len) { + VIR_TEST_DEBUG("%s: %s", "failed to write to an anonymous file", + g_strerror(errno)); + goto cleanup; + } + return fd; + cleanup: + if (VIR_CLOSE(fd) < 0) { + VIR_TEST_DEBUG("%s: %s", "failed to close an anonymous file", + g_strerror(errno)); + } + return -1; +} +#endif
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 :|

On 10/1/21 5:57 AM, Daniel P. Berrangé wrote:
On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
[...]
+GType
+vir_pci_vpd_resource_type_get_type(void)
I know you had asked about using under_scored_naming in a reply to Peter pointing out "non-standard" names in V3 of your patches, but I don't think anyone followed up on that. I do see some of the examples you pointed out in your reply, so definitely there is precedent. Personally when I see a function with underscores in its name, my subconscious immediately thinks (before looking at the words in the name) that they must be from an external library somewhere. My preference would be to have all functions defined within libvirt follow libvirt's naming convention (yeah, I know there's lots of stuff that doesn't!), but I may be an outlier here though (especially since at least some of your examples, e.g. vir_object_init(), was authored by danpb, who also hasn't complained about your choices for names, so...) So this is more a question for the rest of longtime libvirt developers rather than you - do we care about this? If so, exactly what is the policy?
+{ + static GType resourceType; + + static const GEnumValue resourceTypes[] = { + {VIR_PCI_VPD_RESOURCE_TYPE_STRING, "String resource", "string"}, + {VIR_PCI_VPD_RESOURCE_TYPE_VPD_R, "Read-only resource", "vpd-r"}, + {VIR_PCI_VPD_RESOURCE_TYPE_VPD_W, "Read-write resource", "vpd-w"}, + {VIR_PCI_VPD_RESOURCE_TYPE_LAST, "last", "last"}, + {0, NULL, NULL}, + }; + + if (!resourceType) { + resourceType = g_enum_register_static("virPCIVPDResourceType", resourceTypes); + } + return resourceType; +} + +static gboolean
In a similar "coding standards" vein - other uses of "gboolean" (rather than the much more common "bool", or *very rarely* "boolean") in our code seem to be the product of auto-generated(?) xdr headers for wireshark, functions that are registered as callbacks to glib (and so must follow the types in the callback function pointer prototype), and a very small number of other cases. Do we want new code using gboolean rather than bool in these "other" cases that don't require gboolean for proper glib type compatibility? I don't have a strong opinion except that I think consistency is good (otherwise people will spend time trying to decide which one to use in every case), and now is a better time than change the types than later, if that's what people want. (a side-nit completely unrelated to your patches - I noticed that in at least a couple places in libvirt, a gboolean is assigned "true" or "false", although the glib documentation says that it should only have the value "TRUE" or "FALSE". Good thing those happen to use the same values!)
+virPCIVPDResourceIsVendorKeyword(const gchar *keyword)
I similarly wonder about use of gchar rather than char when it's not required for type compatibility with glib APIs. So I guess basically I'm wondering just how far down the glib rabbit hole we aim to go :-)
+ +G_BEGIN_DECLS;
This is not required in libvirt internal code, since we never use C++ internally.
Note to Daniel: I'm glad you gave up on waiting for me to review these patches, because I'm not conversant enough with glib to have caught this (and also would have missed the whole thing about the unnecessary strduping of hash keys).
+#ifdef __linux__ +/** + * virCreateAnonymousFile: + * @data: a pointer to data to be written into a new file. + * @len: the length of data to be written (in bytes). + * + * Create a fake fd, write initial data to it. + * + */ +int +virCreateAnonymousFile(const uint8_t *data, size_t len) +{ + int fd = -1; + char path[] = abs_builddir "testutils-memfd-XXXXXX"; + /* A temp file is used since not all supported distributions support memfd. */ + if ((fd = g_mkstemp_full(path, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) { + return fd; + } + g_unlink(path); + + if (ftruncate(fd, len) != 0) { + VIR_TEST_DEBUG("%s: %s", "failed to ftruncate an anonymous file", + g_strerror(errno)); + goto cleanup; + }
Since it is a new temporary file it is guaranteed zero length, so this should be redundant AFAICT.
I would've missed this too, not due to unfamiliarity, but just that I'd be too busy looking for non-standard names and leaked pointers to pay attention. (Okay, I'll stop embarrassing myself now).

On Fri, Oct 01, 2021 at 01:13:00PM -0400, Laine Stump wrote:
On 10/1/21 5:57 AM, Daniel P. Berrangé wrote:
On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
[...]
+GType
+vir_pci_vpd_resource_type_get_type(void)
I know you had asked about using under_scored_naming in a reply to Peter pointing out "non-standard" names in V3 of your patches, but I don't think anyone followed up on that.
That very specific case is something that is required when we use GObject for the type. see util/viridentity.h for the same scenario. There are a couple of other similar requirements force on us by GObject in this regard, but aside from that we should follow normal libvirt naming practice.
+static gboolean
In a similar "coding standards" vein - other uses of "gboolean" (rather than the much more common "bool", or *very rarely* "boolean") in our code seem to be the product of auto-generated(?) xdr headers for wireshark, functions that are registered as callbacks to glib (and so must follow the types in the callback function pointer prototype), and a very small number of other cases. Do we want new code using gboolean rather than bool in these "other" cases that don't require gboolean for proper glib type compatibility?
gboolean is a completely differnt sized type from bool. As a general rule we want to use 'bool' + true/false, except where we must have strict type compatibility with glib. The main scenario that matters is callbacks integrating with GLib's event loop or similar. All the other gNNNN basic types are essentially identical to stdint.h types, so there's no compelling reason to use them. We can use the plain C types, or the stdint.h sized types as appropriate and they'll be fully compatible with any GLib APIs.
(a side-nit completely unrelated to your patches - I noticed that in at least a couple places in libvirt, a gboolean is assigned "true" or "false", although the glib documentation says that it should only have the value "TRUE" or "FALSE". Good thing those happen to use the same values!)
We're lucky in that the constants do the right thing if you assign/compare. eg a bool foo= TRUE; if (foo == TRUE) .. will be ok. I'm not so sure about bool foo = TRUE gboolean bar == TRUE; if (foo == bar) ... because they're different types, and so when you size-extend I think they end up with different values, despite both being TRUE.
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 :|

On Fri, Oct 1, 2021 at 8:28 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Fri, Oct 01, 2021 at 01:13:00PM -0400, Laine Stump wrote:
On 10/1/21 5:57 AM, Daniel P. Berrangé wrote:
On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
[...]
+GType
+vir_pci_vpd_resource_type_get_type(void)
I know you had asked about using under_scored_naming in a reply to Peter pointing out "non-standard" names in V3 of your patches, but I don't think anyone followed up on that.
That very specific case is something that is required when we use GObject for the type. see util/viridentity.h for the same scenario. There are a couple of other similar requirements force on us by GObject in this regard, but aside from that we should follow normal libvirt naming practice.
Yes, those names are enforced in the glib macros: https://github.com/GNOME/glib/blob/2.53.7/gobject/gtype.h#L1396 GType module_obj_name##_get_type (void); \ https://github.com/GNOME/glib/blob/2.53.7/gobject/gtype.h#L1949-L1951 static void type_name##_init (TypeName *self); \ static void type_name##_class_init (TypeName##Class *klass); \ So there isn't much choice. But Peter was right about property getters/setters and the finalize function - for those glib merely needs function pointers and Libvirt naming conventions can be used there.
+static gboolean
In a similar "coding standards" vein - other uses of "gboolean" (rather than the much more common "bool", or *very rarely* "boolean") in our code seem to be the product of auto-generated(?) xdr headers for wireshark, functions that are registered as callbacks to glib (and so must follow the types in the callback function pointer prototype), and a very small number of other cases. Do we want new code using gboolean rather than bool in these "other" cases that don't require gboolean for proper glib type compatibility?
gboolean is a completely differnt sized type from bool.
As a general rule we want to use 'bool' + true/false, except where we must have strict type compatibility with glib. The main scenario that matters is callbacks integrating with GLib's event loop or similar.
All the other gNNNN basic types are essentially identical to stdint.h types, so there's no compelling reason to use them. We can use the plain C types, or the stdint.h sized types as appropriate and they'll be fully compatible with any GLib APIs.
(a side-nit completely unrelated to your patches - I noticed that in at least a couple places in libvirt, a gboolean is assigned "true" or "false", although the glib documentation says that it should only have the value "TRUE" or "FALSE". Good thing those happen to use the same values!)
We're lucky in that the constants do the right thing if you assign/compare. eg a
bool foo= TRUE; if (foo == TRUE) ..
will be ok.
I'm not so sure about
bool foo = TRUE gboolean bar == TRUE; if (foo == bar) ...
because they're different types, and so when you size-extend I think they end up with different values, despite both being TRUE.
Thanks a lot for raising and discussing this - I was confused as to which types to preferably use as well.

Thanks a lot for the review! Responses inline - ACKs to address in v6. On Fri, Oct 1, 2021 at 12:58 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
I don't know what the thread concurrency rules are of the callers of this code, but regardless we generally aim to make any one-time initializers thread safe by default.
Recommend using VIR_ONCE_GLOBAL_INIT to do one-time init.
Ack, will address in v6.
IIUC, this hash table is created once and never changed. I'm not seeing a clear need for g_strdup here. Can't we just directly use the constant string as the key ?
Good point, no need to store copies of constant strings here.
+ if (!g_regex_match_simple("^[a-zA-Z0-9\\-_\\s,.:=]*$", value, 0, 0)) {
Since you're only trying to validate a set of permitted characters, it is sufficient to use strspn() for this task.
eg what we do in vsh.c is
/* Characters permitted in $EDITOR environment variable and temp filename. */ #define ACCEPTED_CHARS \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-/_.:@"
if (strspn(editor, ACCEPTED_CHARS) != strlen(editor)) { ....error...
Ack, seems like a cheaper way to do this than with regex.
+G_BEGIN_DECLS;
This is not required in libvirt internal code, since we never use C++ internally.
Ack, will remove in v6.
+ if (ftruncate(fd, len) != 0) { + VIR_TEST_DEBUG("%s: %s", "failed to ftruncate an anonymous file", + g_strerror(errno)); + goto cleanup; + }
Since it is a new temporary file it is guaranteed zero length, so this should be redundant AFAICT.
Ack, will address in v6.

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.shcherbakov@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 :|

+ 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.
Makes sense, I'll rework it to have the FD closed in caller functions.

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.
Now, I've learnt a bit more about VPD and considering my comments on the XML format in the last patch, I think this use of GTree is unduly opaque and overkill I think we should be representing the data we're extracting as plan struct fields, in the same way that we handle SMBIOS in the virsysinfo.h file....
diff --git a/src/util/virpcivpd.h b/src/util/virpcivpd.h new file mode 100644 index 0000000000..7327806df3 --- /dev/null +++ b/src/util/virpcivpd.h
+ +#include "internal.h" + +/* + * PCI Local bus (2.2+, Appendix I) and PCIe 4.0+ (7.9.19 VPD Capability) define + * the VPD capability structure (8 bytes in total) and VPD registers that can be used to access + * VPD data including: + * bit 31 of the first 32-bit DWORD: data transfer completion flag (between the VPD data register + * and the VPD data storage hardware); + * bits 30:16 of the first 32-bit DWORD: VPD address of the first VPD data byte to be accessed; + * bits 31:0 of the second 32-bit DWORD: VPD data bytes with LSB being pointed to by the VPD address. + * + * Given that only 15 bits (30:16) are allocated for VPD address its mask is 0x7fff. +*/ +#define PCI_VPD_ADDR_MASK 0x7FFF + +/* + * VPD data consists of small and large resource data types. Information within a resource type + * consists of a 2-byte keyword, 1-byte length and data bytes (up to 255). +*/ +#define PCI_VPD_MAX_FIELD_SIZE 255 +#define PCI_VPD_LARGE_RESOURCE_FLAG 0x80 +#define PCI_VPD_STRING_RESOURCE_FLAG 0x02 +#define PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG 0x10 +#define PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG 0x11 +#define PCI_VPD_RESOURCE_END_TAG 0x0F +#define PCI_VPD_RESOURCE_END_VAL PCI_VPD_RESOURCE_END_TAG << 3 +#define VIR_TYPE_PCI_VPD_RESOURCE_TYPE vir_pci_vpd_resource_type_get_type() + +G_BEGIN_DECLS; + +typedef enum { + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT = 1, + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_BINARY, + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD, + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR, + VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST +} virPCIVPDResourceFieldValueFormat; + +typedef enum { + VIR_PCI_VPD_RESOURCE_TYPE_STRING = 1, + VIR_PCI_VPD_RESOURCE_TYPE_VPD_R, + VIR_PCI_VPD_RESOURCE_TYPE_VPD_W, + VIR_PCI_VPD_RESOURCE_TYPE_LAST +} virPCIVPDResourceType; + +GType vir_pci_vpd_resource_type_get_type(void); + +virPCIVPDResourceFieldValueFormat virPCIVPDResourceGetFieldValueFormat(const gchar *value); + +#define VIR_TYPE_PCI_VPD_RESOURCE vir_pci_vpd_resource_get_type() +G_DECLARE_DERIVABLE_TYPE(virPCIVPDResource, vir_pci_vpd_resource, VIR, PCI_VPD_RESOURCE, GObject); +struct _virPCIVPDResourceClass { + GObjectClass parent_class; +}; + +/* Glib 2.59 adds proper support for g_autolist for derivable types + * see 86c073dba9d82ef3f1bc3d3116b058b9b5c3b1eb. At the time of writing + * 2.56 is the minimum version used by Libvirt. Without the below code + * compilation errors will occur. + */ +#if !GLIB_CHECK_VERSION(2, 59, 0) +typedef GList *virPCIVPDResource_listautoptr; +static inline void +glib_listautoptr_cleanup_virPCIVPDResource(GList **_l) +{ + g_list_free_full(*_l, (GDestroyNotify)g_object_unref); +} +#endif + +GEnumValue *virPCIVPDResourceGetResourceType(virPCIVPDResource *res); + +#define VIR_TYPE_PCI_VPD_STRING_RESOURCE vir_pci_vpd_string_resource_get_type() +G_DECLARE_FINAL_TYPE(virPCIVPDStringResource, vir_pci_vpd_string_resource, VIR, + PCI_VPD_STRING_RESOURCE, virPCIVPDResource); + +struct _virPCIVPDStringResourceClass { + virPCIVPDResourceClass parent_class; +}; + +virPCIVPDStringResource *virPCIVPDStringResourceNew(gchar *value); + +const gchar *virPCIVPDStringResourceGetValue(const virPCIVPDStringResource *res); + +#define VIR_TYPE_PCI_VPD_KEYWORD_RESOURCE vir_pci_vpd_keyword_resource_get_type() +G_DECLARE_FINAL_TYPE(virPCIVPDKeywordResource, vir_pci_vpd_keyword_resource, VIR, + PCI_VPD_KEYWORD_RESOURCE, virPCIVPDResource); +virPCIVPDKeywordResource *virPCIVPDKeywordResourceNew(GTree *resourceFields, bool readOnly); + +void virPCIVPDKeywordResourceForEach(virPCIVPDKeywordResource *res, GTraverseFunc func, + gpointer userData); + + +GList *virPCIVPDParse(int vpdFileFd);
....I feel like all of this can be reduced down to just a couple of public structs and a single method: typedef struct virPCIVPDResourceCustom virPCIVPDResourceCustom; struct virPCIVPDResourceCustom { char idx; char *value; }; typedef struct virPCIVPDResourceRO virPCIVPDResourceRO; struct virPCIVPDResourceRO { char *part_numer; char *change_level; char *fabric_geography; char *location; char *manufcatur_id; char *pci_geography; char *serial_number; size_t nvendor_specific; virPCIVPDResourceCustom *vendor_specific; }; typedef struct virPCIVPDResourceRW virPCIVPDResourceRW; struct virPCIVPDResourceRW { char *asset_tag; size_t nvendor_specific; virPCIVPDResourceCustom *vendor_specific; size_t nsystem_specific; virPCIVPDResourceCustom *system_specific; }; typedef struct virPCIVPDResource virPCIVPDResource; struct virPCIVPDResource { char *name; virPCIVPDResourceRO *ro; virPCIVPDResourceRW *rw; } virPCIVPDResource *virPCIVPDParse(int vpdFileFd); void virPCIVPDResourceFree(virPCIVPDResource *res); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIVPDResource, virPCIVPDResourceFree); 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 :|

On Fri, Oct 1, 2021 at 7:49 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
A GTree is used as a data structure in order to maintain key ordering which will be important in XML to XML tests later.
Now, I've learnt a bit more about VPD and considering my comments on the XML format in the last patch, I think this use of GTree is unduly opaque and overkill
I think we should be representing the data we're extracting as plan struct fields, in the same way that we handle SMBIOS in the virsysinfo.h file....
....I feel like all of this can be reduced down to just a couple of public structs and a single method:
typedef struct virPCIVPDResourceCustom virPCIVPDResourceCustom; struct virPCIVPDResourceCustom { char idx; char *value; };
typedef struct virPCIVPDResourceRO virPCIVPDResourceRO; struct virPCIVPDResourceRO { char *part_numer; char *change_level; char *fabric_geography; char *location; char *manufcatur_id; char *pci_geography; char *serial_number; size_t nvendor_specific; virPCIVPDResourceCustom *vendor_specific; };
typedef struct virPCIVPDResourceRW virPCIVPDResourceRW; struct virPCIVPDResourceRW { char *asset_tag; size_t nvendor_specific; virPCIVPDResourceCustom *vendor_specific; size_t nsystem_specific; virPCIVPDResourceCustom *system_specific; };
typedef struct virPCIVPDResource virPCIVPDResource; struct virPCIVPDResource { char *name; virPCIVPDResourceRO *ro; virPCIVPDResourceRW *rw; }
virPCIVPDResource *virPCIVPDParse(int vpdFileFd); void virPCIVPDResourceFree(virPCIVPDResource *res); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIVPDResource, virPCIVPDResourceFree);
Ack, I can rework it as you describe. There will be a bit more work to do in XML serialization and deserialization code to map struct fields to keyword names and vice versa but I'll see what I can come up with. My original goal with using Glib data structures was to use standard iteration, insertion and lookup operations and also simplify automatic cleanup implementation. On the other hand, there are some specifics of working with them as my workaround for a bug with derived types shows.

Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- NEWS.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index fd20e50d18..a4178e505c 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -29,6 +29,14 @@ v7.8.0 (unreleased) active. This information can also be retrieved with the new virsh command ``nodedev-info``. + * virpcivpd: Add a PCI VPD parser + + A parser for the standard PCI/PCIe VPD ("I.3. VPD Definitions" in PCI 2.2+ + and an equivalent definition in "6.28.1 VPD Format" PCIe 4.0) was added + along with relevant types to represent PCI VPD in memory. This + functionality got added for Linux only at this point (kernels above + v2.6.26 have support for exposing VPD via sysfs). + * **Improvements** * **Bug fixes** -- 2.30.2

On Mon, Sep 27, 2021 at 10:30:48PM +0300, Dmitrii Shcherbakov wrote:
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- NEWS.rst | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst index fd20e50d18..a4178e505c 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -29,6 +29,14 @@ v7.8.0 (unreleased) active. This information can also be retrieved with the new virsh command ``nodedev-info``.
+ * virpcivpd: Add a PCI VPD parser + + A parser for the standard PCI/PCIe VPD ("I.3. VPD Definitions" in PCI 2.2+ + and an equivalent definition in "6.28.1 VPD Format" PCIe 4.0) was added + along with relevant types to represent PCI VPD in memory. This + functionality got added for Linux only at this point (kernels above + v2.6.26 have support for exposing VPD via sysfs). +
I think this is likely overkill. From a user pov I think it is sufficient to just have the last news item in this series that mentions the new VPD features reported against node devices. 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 :|

Add helper functions to virpci to provide means of checking for a VPD file presence and for VPD resource retrieval using the PCI VPD parser. The added test assesses the basic functionality of VPD retrieval while the full parser is tested by virpcivpdtest. Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- src/libvirt_private.syms | 2 ++ src/util/virpci.c | 62 ++++++++++++++++++++++++++++++++++++++ src/util/virpci.h | 3 ++ tests/virpcimock.c | 30 +++++++++++++++++++ tests/virpcitest.c | 64 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bb9b380599..ebee46ce9b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2988,7 +2988,9 @@ virPCIDeviceGetReprobe; virPCIDeviceGetStubDriver; virPCIDeviceGetUnbindFromStub; virPCIDeviceGetUsedBy; +virPCIDeviceGetVPDResources; virPCIDeviceHasPCIExpressLink; +virPCIDeviceHasVPD; virPCIDeviceIsAssignable; virPCIDeviceIsPCIExpress; virPCIDeviceListAdd; diff --git a/src/util/virpci.c b/src/util/virpci.c index f307580a53..480d91c17f 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -37,6 +37,7 @@ #include "virkmod.h" #include "virstring.h" #include "viralloc.h" +#include "virpcivpd.h" VIR_LOG_INIT("util.pci"); @@ -2640,6 +2641,53 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, return 0; } + +gboolean +virPCIDeviceHasVPD(virPCIDevice *dev) +{ + g_autofree char *vpdPath = NULL; + vpdPath = virPCIFile(dev->name, "vpd"); + if (!virFileExists(vpdPath)) { + VIR_INFO("Device VPD file does not exist %s", vpdPath); + return false; + } else if (!virFileIsRegular(vpdPath)) { + VIR_WARN("VPD path does not point to a regular file %s", vpdPath); + return false; + } + return true; +} + +/** + * virPCIDeviceGetVPDResources: + * @dev: a PCI device to get a list of VPD resources for. + * + * Obtain resources stored in the PCI device's Vital Product Data (VPD). + * VPD is optional in both PCI Local Bus and PCIe specifications so there is + * no guarantee it will be there for a particular device. + * + * Returns: a pointer to a list of VPDResource types which needs to be freed by the caller or + * NULL if getting it failed for some reason. + */ +GList * +virPCIDeviceGetVPDResources(virPCIDevice *dev) +{ + g_autofree char *vpdPath = NULL; + int fd; + + vpdPath = virPCIFile(dev->name, "vpd"); + if (!virPCIDeviceHasVPD(dev)) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Device %s does not have a VPD"), + virPCIDeviceGetName(dev)); + return NULL; + } + if ((fd = open(vpdPath, O_RDONLY)) < 0) { + virReportSystemError(-fd, + _("Failed to open a VPD file '%s'"), vpdPath); + return NULL; + } + return virPCIVPDParse(fd); +} + #else static const char *unsupported = N_("not supported on non-linux platforms"); @@ -2713,6 +2761,20 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path G_GNUC_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; } + +gboolean +virPCIDeviceHasVPD(virPCIDevice *dev) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return NULL; +} + +GList * +virPCIDeviceGetVPDResources(virPCIDevice *dev) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return NULL; +} #endif /* __linux__ */ int diff --git a/src/util/virpci.h b/src/util/virpci.h index 9a3db6c6d8..a89561496b 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -269,6 +269,9 @@ int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, char **pfname, int *vf_index); +gboolean virPCIDeviceHasVPD(virPCIDevice *dev); +GList * virPCIDeviceGetVPDResources(virPCIDevice *dev); + int virPCIDeviceUnbind(virPCIDevice *dev); int virPCIDeviceRebind(virPCIDevice *dev); int virPCIDeviceGetDriverPathAndName(virPCIDevice *dev, diff --git a/tests/virpcimock.c b/tests/virpcimock.c index d4d43aac51..e10ebce76f 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -18,6 +18,8 @@ #include <config.h> +#include "virpcivpd.h" + #if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__) # define VIR_MOCK_LOOKUP_MAIN # include "virmock.h" @@ -123,6 +125,13 @@ struct pciDeviceAddress { }; # define ADDR_STR_FMT "%04x:%02x:%02x.%u" +struct pciVPD { + /* PCI VPD contents (binary, may contain NULLs), NULL if not present. */ + const char *data; + /* VPD length in bytes. */ + size_t vpd_len; +}; + struct pciDevice { struct pciDeviceAddress addr; int vendor; @@ -131,6 +140,7 @@ struct pciDevice { int iommuGroup; const char *physfn; struct pciDriver *driver; /* Driver attached. NULL if attached to no driver */ + struct pciVPD vpd; }; struct fdCallback { @@ -537,6 +547,10 @@ pci_device_new_from_stub(const struct pciDevice *data) make_symlink(devpath, "physfn", tmp); } + if (dev->vpd.data && dev->vpd.vpd_len) { + make_file(devpath, "vpd", dev->vpd.data, dev->vpd.vpd_len); + } + if (pci_device_autobind(dev) < 0) ABORT("Unable to bind: %s", devid); @@ -942,6 +956,20 @@ static void init_env(void) { g_autofree char *tmp = NULL; + const char fullVPDExampleData[] = { + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_STRING_RESOURCE_FLAG, 0x08, 0x00, + 't', 'e', 's', 't', 'n', 'a', 'm', 'e', + PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x16, 0x00, + 'P', 'N', 0x02, '4', '2', + 'E', 'C', 0x04, '4', '2', '4', '2', + 'V', 'A', 0x02, 'E', 'X', + 'R', 'V', 0x02, 0x31, 0x00, + PCI_VPD_RESOURCE_END_VAL + }; + struct pciVPD exampleVPD = { + .data = fullVPDExampleData, + .vpd_len = sizeof(fullVPDExampleData) / sizeof(fullVPDExampleData[0]), + }; if (!(fakerootdir = getenv("LIBVIRT_FAKE_ROOT_DIR"))) ABORT("Missing LIBVIRT_FAKE_ROOT_DIR env variable\n"); @@ -1008,6 +1036,8 @@ init_env(void) MAKE_PCI_DEVICE("0000:01:00.0", 0x1cc1, 0x8201, 14, .klass = 0x010802); MAKE_PCI_DEVICE("0000:02:00.0", 0x1cc1, 0x8201, 15, .klass = 0x010802); + + MAKE_PCI_DEVICE("0000:03:00.0", 0x15b3, 0xa2d6, 16, .vpd = exampleVPD); } diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 6fe9b7d13a..c6bf12ba0b 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -17,6 +17,7 @@ */ #include <config.h> +#include "internal.h" #include "testutils.h" @@ -26,6 +27,7 @@ # include <sys/stat.h> # include <fcntl.h> # include <virpci.h> +# include <virpcivpd.h> # define VIR_FROM_THIS VIR_FROM_NONE @@ -328,6 +330,66 @@ testVirPCIDeviceUnbind(const void *opaque) return ret; } + +static gboolean +testVirPCIGetOneResourceField(gpointer *key, gpointer *val, gpointer userData) +{ + const char **fieldValue = (const char **)userData; + if (STREQ((const char*)key, "PN")) { + *fieldValue = (const char*)val; + return true; + } + return false; +} + +static int +testVirPCIDeviceGetVPDResources(const void *opaque) +{ + const struct testPCIDevData *data = opaque; + int ret = -1; + g_autofree virPCIDevice *dev = NULL; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function}; + g_autofree GList *resources = NULL; + const char* resValue = NULL; + const char* fieldValue = NULL; + virPCIVPDStringResource *strRes; + virPCIVPDKeywordResource *kwRes; + + dev = virPCIDeviceNew(&devAddr); + if (!dev) { + return ret; + } + + resources = virPCIDeviceGetVPDResources(dev); + + /* Here we simply check that 2 resources are present and their basic use. + * Full parser validation is done elsewhere. + */ + if (g_list_length(resources) != 2) { + return ret; + } + + strRes = g_list_nth_data(resources, 0); + resValue = virPCIVPDStringResourceGetValue(strRes); + if (STRNEQ(resValue, "testname")) { + VIR_TEST_DEBUG("Unexpected string resource value: %s", resValue); + return ret; + } + kwRes = g_list_nth_data(resources, 1); + virPCIVPDKeywordResourceForEach(kwRes, + (GTraverseFunc)testVirPCIGetOneResourceField, &fieldValue); + + if (!fieldValue || STRNEQ(fieldValue, "42")) { + VIR_TEST_DEBUG("Could not retrieve an expected value from the Keyword resource: %s", + resValue); + return ret; + } + + ret = 0; + return ret; +} + # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int @@ -409,6 +471,8 @@ mymain(void) DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 3, 0); DO_TEST_PCI_DRIVER(0, 0x0a, 3, 0, NULL); + DO_TEST_PCI(testVirPCIDeviceGetVPDResources, 0, 0x03, 0, 0); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); -- 2.30.2

On Mon, Sep 27, 2021 at 10:30:49PM +0300, Dmitrii Shcherbakov wrote:
Add helper functions to virpci to provide means of checking for a VPD file presence and for VPD resource retrieval using the PCI VPD parser.
The added test assesses the basic functionality of VPD retrieval while the full parser is tested by virpcivpdtest.
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- src/libvirt_private.syms | 2 ++ src/util/virpci.c | 62 ++++++++++++++++++++++++++++++++++++++ src/util/virpci.h | 3 ++ tests/virpcimock.c | 30 +++++++++++++++++++ tests/virpcitest.c | 64 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bb9b380599..ebee46ce9b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2988,7 +2988,9 @@ virPCIDeviceGetReprobe; virPCIDeviceGetStubDriver; virPCIDeviceGetUnbindFromStub; virPCIDeviceGetUsedBy; +virPCIDeviceGetVPDResources; virPCIDeviceHasPCIExpressLink; +virPCIDeviceHasVPD; virPCIDeviceIsAssignable; virPCIDeviceIsPCIExpress; virPCIDeviceListAdd; diff --git a/src/util/virpci.c b/src/util/virpci.c index f307580a53..480d91c17f 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -37,6 +37,7 @@ #include "virkmod.h" #include "virstring.h" #include "viralloc.h" +#include "virpcivpd.h"
VIR_LOG_INIT("util.pci");
@@ -2640,6 +2641,53 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, return 0; }
+ +gboolean +virPCIDeviceHasVPD(virPCIDevice *dev) +{ + g_autofree char *vpdPath = NULL; + vpdPath = virPCIFile(dev->name, "vpd"); + if (!virFileExists(vpdPath)) { + VIR_INFO("Device VPD file does not exist %s", vpdPath);
IIUC this is normal case for most PCI devices, so DEBUG level is better.
+ return false; + } else if (!virFileIsRegular(vpdPath)) { + VIR_WARN("VPD path does not point to a regular file %s", vpdPath); + return false; + } + return true; +} + +/** + * virPCIDeviceGetVPDResources: + * @dev: a PCI device to get a list of VPD resources for. + * + * Obtain resources stored in the PCI device's Vital Product Data (VPD). + * VPD is optional in both PCI Local Bus and PCIe specifications so there is + * no guarantee it will be there for a particular device. + * + * Returns: a pointer to a list of VPDResource types which needs to be freed by the caller or + * NULL if getting it failed for some reason. + */ +GList * +virPCIDeviceGetVPDResources(virPCIDevice *dev) +{ + g_autofree char *vpdPath = NULL; + int fd; + + vpdPath = virPCIFile(dev->name, "vpd"); + if (!virPCIDeviceHasVPD(dev)) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Device %s does not have a VPD"), + virPCIDeviceGetName(dev));
Nit-pick - align with the '(' above.
+ return NULL; + } + if ((fd = open(vpdPath, O_RDONLY)) < 0) { + virReportSystemError(-fd, + _("Failed to open a VPD file '%s'"), vpdPath); + return NULL; + } + return virPCIVPDParse(fd);
Nothing is closing 'fd' here, unless virPCIVPDParse is doing it, which I would consider an unexpected design. 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 :|

Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- NEWS.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index a4178e505c..4617198f82 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -37,6 +37,13 @@ v7.8.0 (unreleased) functionality got added for Linux only at this point (kernels above v2.6.26 have support for exposing VPD via sysfs). + * virpci: Add PCI VPD-related helper functions to virpci + + In order to utilize the PCI VPD parser, a couple of helper functions got + introduced to check for the presence of a VPD file in the sysfs tree and + to invoke the PCI VPD parser to get a list of resources representing PCI + VPD contents in memory. + * **Improvements** * **Bug fixes** -- 2.30.2

* XML serialization and deserialization of PCI VPD resources; * PCI VPD capability flags added and used in relevant places; * XML to XML tests for the added capability. Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- docs/schemas/nodedev.rng | 40 +++ include/libvirt/libvirt-nodedev.h | 1 + src/conf/node_device_conf.c | 271 ++++++++++++++++++ src/conf/node_device_conf.h | 6 +- src/conf/virnodedeviceobj.c | 7 +- src/node_device/node_device_driver.c | 2 + src/node_device/node_device_udev.c | 2 + .../pci_0000_42_00_0_vpd.xml | 33 +++ .../pci_0000_42_00_0_vpd.xml | 1 + tests/nodedevxml2xmltest.c | 1 + tools/virsh-nodedev.c | 3 + 11 files changed, 365 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml create mode 120000 tests/nodedevxml2xmlout/pci_0000_42_00_0_vpd.xml diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index e089e66858..fbbee4d0c6 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -223,6 +223,10 @@ <ref name="mdev_types"/> </optional> + <optional> + <ref name="vpd"/> + </optional> + <optional> <element name="iommuGroup"> <attribute name="number"> @@ -770,6 +774,42 @@ </element> </define> + <define name="vpd"> + <element name="capability"> + <attribute name="type"> + <value>vpd</value> + </attribute> + <oneOrMore> + <choice> + <element name="resource"> + <attribute name="type"> + <choice> + <value>string</value> + </choice> + </attribute> + <text/> + </element> + <element name="resource"> + <attribute name="type"> + <choice> + <value>vpd-r</value> + <value>vpd-w</value> + </choice> + </attribute> + <oneOrMore> + <element name="field"> + <attribute name="keyword"> + <data type="string"/> + </attribute> + <text/> + </element> + </oneOrMore> + </element> + </choice> + </oneOrMore> + </element> + </define> + <define name="apDomainRange"> <choice> <data type="string"> diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index e492634217..245365b07f 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -84,6 +84,7 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD = 1 << 18, /* s390 AP Card device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE = 1 << 19, /* s390 AP Queue */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX = 1 << 20, /* s390 AP Matrix */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPD = 1 << 21, /* Device with VPD */ /* filter the devices by active state */ VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE = 1 << 30, /* Inactive devices */ diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 9bbff97ffd..7d9f74f45b 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -36,6 +36,7 @@ #include "virrandom.h" #include "virlog.h" #include "virfcp.h" +#include "virpcivpd.h" #define VIR_FROM_THIS VIR_FROM_NODEDEV @@ -70,6 +71,7 @@ VIR_ENUM_IMPL(virNodeDevCap, "ap_card", "ap_queue", "ap_matrix", + "vpd", ); VIR_ENUM_IMPL(virNodeDevNetCap, @@ -240,6 +242,90 @@ virNodeDeviceCapMdevTypesFormat(virBuffer *buf, } } +static void +virNodeDeviceCapVPDStringResourceFormat(virBuffer *buf, virPCIVPDResource *res) +{ + virBufferAdjustIndent(buf, 2); + virBufferEscapeString(buf, "%s", virPCIVPDStringResourceGetValue((virPCIVPDStringResource *)res)); + virBufferAdjustIndent(buf, -2); +} + +static gboolean +virNodeDeviceCapVPDWriteTextField(gpointer key, gpointer val, gpointer userData) +{ + const gchar *k = (const gchar*)key, *v = (const gchar*)val; + virBuffer *buf = userData; + virPCIVPDResourceFieldValueFormat format = VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST; + + format = virPCIVPDResourceGetFieldValueFormat(k); + + if (format == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT) { + virBufferEscapeString(buf, "<field keyword='%s'>", k); + virBufferEscapeString(buf, "%s", v); + virBufferEscapeString(buf, "</field>\n", k); + } + return false; +} + +static void +virNodeDeviceCapVPDKeywordResourceFormat(virBuffer *buf, virPCIVPDResource *res) +{ + virPCIVPDKeywordResource *keywordRes = NULL; + g_autofree GHashTableIter *iter = NULL; + + virBufferAddLit(buf, "\n"); + virBufferAdjustIndent(buf, 2); + keywordRes = (virPCIVPDKeywordResource*)res; + + virPCIVPDKeywordResourceForEach(keywordRes, virNodeDeviceCapVPDWriteTextField, buf); + virBufferAdjustIndent(buf, -2); +} + +static void +virNodeDeviceCapVPDResourceFormat(virBuffer *buf, virPCIVPDResource *res) +{ + GEnumValue *resRype = NULL; + void (*resFormatFunc)(virBuffer *buf, virPCIVPDResource *res); + + if (G_TYPE_CHECK_INSTANCE_TYPE(res, VIR_TYPE_PCI_VPD_STRING_RESOURCE)) { + resFormatFunc = virNodeDeviceCapVPDStringResourceFormat; + } else if (G_TYPE_CHECK_INSTANCE_TYPE(res, VIR_TYPE_PCI_VPD_KEYWORD_RESOURCE)) { + resFormatFunc = virNodeDeviceCapVPDKeywordResourceFormat; + } else { + /* Unexpected resource type. This should not happen unless the PCI(e) specs + * change and new resource types are introduced into util.virpcivpd. Either way, + * we can only return the control back to the caller here. + */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected VPD resource type encountered during formatting")); + return; + } + virBufferAdjustIndent(buf, 2); + + resRype = virPCIVPDResourceGetResourceType(res); + virBufferEscapeString(buf, "<resource type='%s'>", resRype->value_nick); + /* Format the resource in a type-specific way. */ + resFormatFunc(buf, res); + virBufferAddLit(buf, "</resource>\n"); + + virBufferAdjustIndent(buf, -2); +} + +static void +virNodeDeviceCapVPDFormat(virBuffer *buf, GList *vpdResources) +{ + if (!g_list_length(vpdResources)) { + return; + } + + virBufferAddLit(buf, "<capability type='vpd'>\n"); + while (vpdResources) { + GList *next = vpdResources->next; + virNodeDeviceCapVPDResourceFormat(buf, vpdResources->data); + vpdResources = next; + } + virBufferAddLit(buf, "</capability>\n"); +} static void virNodeDeviceCapPCIDefFormat(virBuffer *buf, @@ -315,6 +401,9 @@ virNodeDeviceCapPCIDefFormat(virBuffer *buf, data->pci_dev.mdev_types, data->pci_dev.nmdev_types); } + if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_VPD) { + virNodeDeviceCapVPDFormat(buf, data->pci_dev.vpd_resources); + } if (data->pci_dev.nIommuGroupDevices) { virBufferAsprintf(buf, "<iommuGroup number='%d'>\n", data->pci_dev.iommuGroupNumber); @@ -673,6 +762,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_VPD: case VIR_NODE_DEV_CAP_LAST: break; } @@ -859,6 +949,132 @@ virNodeDevCapMdevTypesParseXML(xmlXPathContextPtr ctxt, return ret; } +static GTree * +virNodeDeviceCapVPDParseXMLKeywordResource(xmlXPathContextPtr ctxt) +{ + int nfields = -1; + g_autofree gchar* fieldValue = NULL; + g_autofree gchar* fieldKeyword = NULL; + g_autoptr(GTree) resourceFields = NULL; + g_autofree xmlNodePtr *nodes = NULL; + xmlNodePtr orignode = NULL; + size_t i = 0; + + resourceFields = g_tree_new_full( + (GCompareDataFunc)g_strcmp0, NULL, g_free, g_free); + + if ((nfields = virXPathNodeSet("./field[@keyword]", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no VPD <field> elements with a keyword specified found")); + ctxt->node = orignode; + return NULL; + } + + orignode = ctxt->node; + for (i = 0; i < nfields; i++) { + ctxt->node = nodes[i]; + if (!(fieldKeyword = virXPathString("string(./@keyword[1])", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("VPD resource field keyword parsing has failed")); + continue; + } + if (!(fieldValue = virXPathString("string(./text())", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("VPD resource field keyword parsing has failed")); + continue; + } + g_tree_insert(resourceFields, g_steal_pointer(&fieldKeyword), + g_steal_pointer(&fieldValue)); + } + ctxt->node = orignode; + return g_steal_pointer(&resourceFields); +} + +static int +virNodeDeviceCapVPDParseXML(xmlXPathContextPtr ctxt, GList **vpdResources) +{ + xmlNodePtr orignode = NULL; + g_autofree gchar* resText = NULL; + g_autofree GTree *resourceFields = NULL; + g_autofree xmlNodePtr *nodes = NULL; + int nresources = -1; + g_autofree gchar* resTypeStr = NULL; + virPCIVPDResourceType type = VIR_PCI_VPD_RESOURCE_TYPE_LAST; + GEnumClass *class; + size_t i = 0; + + if ((nresources = virXPathNodeSet("./resource[@type]", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no VPD <resource> elements with a type specified found")); + return -1; + } + + orignode = ctxt->node; + for (i = 0; i < nresources; i++) { + ctxt->node = nodes[i]; + if (!(resTypeStr = virXPathString("string(./@type[1])", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("VPD resource type parsing has failed")); + ctxt->node = orignode; + return -1; + } + + class = g_type_class_ref(VIR_TYPE_PCI_VPD_RESOURCE_TYPE); + type = g_enum_get_value_by_nick(class, resTypeStr)->value; + g_type_class_unref(class); + if (!type) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Unexpected VPD resource type encountered")); + /* Skip resources with unknown types. */ + continue; + } + + /* Create in-memory representations of resources based on their type. If a resource is not + * valid, report an error and skip to parsing another resource. + */ + switch (type) { + case VIR_PCI_VPD_RESOURCE_TYPE_STRING: + if (!(resText = virXPathString("string(./text())", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Could not read a string resource text")); + continue; + } + *vpdResources = g_list_append(*vpdResources, virPCIVPDStringResourceNew( + g_steal_pointer(&resText))); + break; + case VIR_PCI_VPD_RESOURCE_TYPE_VPD_R: + if (!(resourceFields = virNodeDeviceCapVPDParseXMLKeywordResource(ctxt))) { + virReportError(VIR_ERR_XML_ERROR, + _("Could not parse %s VPD resource fields"), resTypeStr); + continue; + } + *vpdResources = g_list_append(*vpdResources, + virPCIVPDKeywordResourceNew(g_steal_pointer(&resourceFields), true)); + break; + case VIR_PCI_VPD_RESOURCE_TYPE_VPD_W: + if (!(resourceFields = virNodeDeviceCapVPDParseXMLKeywordResource(ctxt))) { + virReportError(VIR_ERR_XML_ERROR, + _("Could not parse %s VPD resource fields"), resTypeStr); + continue; + } + *vpdResources = g_list_append(*vpdResources, + virPCIVPDKeywordResourceNew(g_steal_pointer(&resourceFields), false)); + break; + case VIR_PCI_VPD_RESOURCE_TYPE_LAST: + default: + /* If the VPD module is aware of a resource type and it has been serialized into + * XML while the parser does not then this is a bug. + */ + virReportError(VIR_ERR_XML_ERROR, + _("The XML parser has encountered an unsupported resource type %s"), + resTypeStr); + ctxt->node = orignode; + return -1; + } + } + ctxt->node = orignode; + return 0; +} static int virNodeDevAPMatrixCapabilityParseXML(xmlXPathContextPtr ctxt, @@ -1718,6 +1934,11 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, &pci_dev->nmdev_types) < 0) return -1; pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_MDEV; + } else if (STREQ(type, "vpd")) { + if (virNodeDeviceCapVPDParseXML(ctxt, &pci_dev->vpd_resources) < 0) { + return -1; + } + pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VPD; } else { int hdrType = virPCIHeaderTypeFromString(type); @@ -2024,6 +2245,7 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_SCSI_GENERIC: case VIR_NODE_DEV_CAP_VDPA: + case VIR_NODE_DEV_CAP_VPD: case VIR_NODE_DEV_CAP_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown capability type '%d' for '%s'"), @@ -2287,6 +2509,8 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps) for (i = 0; i < data->pci_dev.nmdev_types; i++) virMediatedDeviceTypeFree(data->pci_dev.mdev_types[i]); g_free(data->pci_dev.mdev_types); + g_list_free_full(g_steal_pointer(&data->pci_dev.vpd_resources), + g_object_unref); break; case VIR_NODE_DEV_CAP_USB_DEV: g_free(data->usb_dev.product_name); @@ -2352,6 +2576,7 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps) case VIR_NODE_DEV_CAP_VDPA: case VIR_NODE_DEV_CAP_AP_CARD: case VIR_NODE_DEV_CAP_AP_QUEUE: + case VIR_NODE_DEV_CAP_VPD: case VIR_NODE_DEV_CAP_LAST: /* This case is here to shutup the compiler */ break; @@ -2418,6 +2643,7 @@ virNodeDeviceUpdateCaps(virNodeDeviceDef *def) case VIR_NODE_DEV_CAP_VDPA: case VIR_NODE_DEV_CAP_AP_CARD: case VIR_NODE_DEV_CAP_AP_QUEUE: + case VIR_NODE_DEV_CAP_VPD: case VIR_NODE_DEV_CAP_LAST: break; } @@ -2489,6 +2715,10 @@ virNodeDeviceCapsListExport(virNodeDeviceDef *def, MAYBE_ADD_CAP(VIR_NODE_DEV_CAP_MDEV_TYPES); ncaps++; } + if (flags & VIR_NODE_DEV_CAP_FLAG_PCI_VPD) { + MAYBE_ADD_CAP(VIR_NODE_DEV_CAP_VPD); + ncaps++; + } } if (caps->data.type == VIR_NODE_DEV_CAP_CSS_DEV) { @@ -2749,6 +2979,44 @@ virNodeDeviceGetMdevTypesCaps(const char *sysfspath, } +/** + * virNodeDeviceGetPCIVPDDynamicCap: + * @devCapPCIDev: a virNodeDevCapPCIDev for which to add VPD resources. + * + * While VPD has a read-only portion, there may be a read-write portion per + * the specs which may change dynamically. + * + * Returns: 0 if the operation was successful (even if VPD is not present for + * that device since it is optional in the specs, -1 otherwise. + */ +static int +virNodeDeviceGetPCIVPDDynamicCap(virNodeDevCapPCIDev *devCapPCIDev) +{ + g_autoptr(virPCIDevice) pciDev = NULL; + virPCIDeviceAddress devAddr; + g_autolist(virPCIVPDResource) resourceList = NULL; + + devAddr.domain = devCapPCIDev->domain; + devAddr.bus = devCapPCIDev->bus; + devAddr.slot = devCapPCIDev->slot; + devAddr.function = devCapPCIDev->function; + + if (!(pciDev = virPCIDeviceNew(&devAddr))) + return -1; + + if (virPCIDeviceHasVPD(pciDev)) { + /* VPD is optional in PCI(e) specs. If it is there, attempt to add it. */ + if ((resourceList = virPCIDeviceGetVPDResources(pciDev))) { + devCapPCIDev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VPD; + devCapPCIDev->vpd_resources = g_steal_pointer(&resourceList); + } else { + g_list_free_full(devCapPCIDev->vpd_resources, g_object_unref); + } + } + return 0; +} + + /* virNodeDeviceGetPCIDynamicCaps() get info that is stored in sysfs * about devices related to this device, i.e. things that can change * without this device itself changing. These must be refreshed @@ -2771,6 +3039,9 @@ virNodeDeviceGetPCIDynamicCaps(const char *sysfsPath, if (pci_dev->nmdev_types > 0) pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_MDEV; + if (virNodeDeviceGetPCIVPDDynamicCap(pci_dev) < 0) + return -1; + return 0; } diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 5a4d9c7a55..32e59fa52a 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -69,6 +69,7 @@ typedef enum { VIR_NODE_DEV_CAP_AP_CARD, /* s390 AP Card device */ VIR_NODE_DEV_CAP_AP_QUEUE, /* s390 AP Queue */ VIR_NODE_DEV_CAP_AP_MATRIX, /* s390 AP Matrix device */ + VIR_NODE_DEV_CAP_VPD, /* Device provides VPD */ VIR_NODE_DEV_CAP_LAST } virNodeDevCapType; @@ -103,6 +104,7 @@ typedef enum { VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION = (1 << 1), VIR_NODE_DEV_CAP_FLAG_PCIE = (1 << 2), VIR_NODE_DEV_CAP_FLAG_PCI_MDEV = (1 << 3), + VIR_NODE_DEV_CAP_FLAG_PCI_VPD = (1 << 4), } virNodeDevPCICapFlags; typedef enum { @@ -181,6 +183,7 @@ struct _virNodeDevCapPCIDev { int hdrType; /* enum virPCIHeaderType or -1 */ virMediatedDeviceType **mdev_types; size_t nmdev_types; + GList *vpd_resources; }; typedef struct _virNodeDevCapUSBDev virNodeDevCapUSBDev; @@ -418,7 +421,8 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNodeDevCapsDef, virNodeDevCapsDefFree); VIR_CONNECT_LIST_NODE_DEVICES_CAP_VDPA | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE | \ - VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX) + VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPD) #define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE \ VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE | \ diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 9a9841576a..165ec1f1dd 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -701,6 +701,9 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, if (type == VIR_NODE_DEV_CAP_MDEV_TYPES && (cap->data.pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) return true; + if (type == VIR_NODE_DEV_CAP_VPD && + (cap->data.pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_VPD)) + return true; break; case VIR_NODE_DEV_CAP_SCSI_HOST: @@ -742,6 +745,7 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, case VIR_NODE_DEV_CAP_VDPA: case VIR_NODE_DEV_CAP_AP_CARD: case VIR_NODE_DEV_CAP_AP_QUEUE: + case VIR_NODE_DEV_CAP_VPD: case VIR_NODE_DEV_CAP_LAST: break; } @@ -899,7 +903,8 @@ virNodeDeviceObjMatch(virNodeDeviceObj *obj, MATCH_CAP(VDPA) || MATCH_CAP(AP_CARD) || MATCH_CAP(AP_QUEUE) || - MATCH_CAP(AP_MATRIX))) + MATCH_CAP(AP_MATRIX) || + MATCH_CAP(VPD))) return false; } diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 3bc6eb1c11..d19ed7d948 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -708,6 +708,7 @@ nodeDeviceObjFormatAddress(virNodeDeviceObj *obj) case VIR_NODE_DEV_CAP_VDPA: case VIR_NODE_DEV_CAP_AP_CARD: case VIR_NODE_DEV_CAP_AP_QUEUE: + case VIR_NODE_DEV_CAP_VPD: case VIR_NODE_DEV_CAP_LAST: break; } @@ -1983,6 +1984,7 @@ int nodeDeviceDefValidate(virNodeDeviceDef *def, case VIR_NODE_DEV_CAP_AP_CARD: case VIR_NODE_DEV_CAP_AP_QUEUE: case VIR_NODE_DEV_CAP_AP_MATRIX: + case VIR_NODE_DEV_CAP_VPD: case VIR_NODE_DEV_CAP_LAST: break; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 71f0bef827..7c3bb762b3 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -42,6 +42,7 @@ #include "virnetdev.h" #include "virmdev.h" #include "virutil.h" +#include "virpcivpd.h" #include "configmake.h" @@ -1397,6 +1398,7 @@ udevGetDeviceDetails(struct udev_device *device, case VIR_NODE_DEV_CAP_AP_MATRIX: return udevProcessAPMatrix(device, def); case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_VPD: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: diff --git a/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml b/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml new file mode 100644 index 0000000000..831b6feb24 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml @@ -0,0 +1,33 @@ +<device> + <name>pci_0000_42_00_0</name> + <capability type='pci'> + <class>0x020000</class> + <domain>0</domain> + <bus>66</bus> + <slot>0</slot> + <function>0</function> + <product id='0xa2d6'>MT42822 BlueField-2 integrated ConnectX-6 Dx network controller</product> + <vendor id='0x15b3'>Mellanox Technologies</vendor> + <capability type='virt_functions' maxCount='16'/> + <capability type='vpd'> + <resource type='string'>BlueField-2 DPU 25GbE Dual-Port SFP56, Crypto Enabled, 16GB on-board DDR, 1GbE OOB management, Tall Bracket</resource> + <resource type='vpd-r'> + <field keyword='EC'>B1</field> + <field keyword='PN'>MBF2H332A-AEEOT</field> + <field keyword='SN'>MT2113X00000</field> + <field keyword='V0'>PCIeGen4 x8</field> + <field keyword='V2'>MBF2H332A-AEEOT</field> + <field keyword='V3'>3c53d07eec484d8aab34dabd24fe575aa</field> + <field keyword='VA'>MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=BF2H332A</field> + </resource> + </capability> + <iommuGroup number='65'> + <address domain='0x0000' bus='0x42' slot='0x00' function='0x0'/> + </iommuGroup> + <numa node='0'/> + <pci-express> + <link validity='cap' port='0' speed='16' width='8'/> + <link validity='sta' speed='8' width='8'/> + </pci-express> + </capability> +</device> diff --git a/tests/nodedevxml2xmlout/pci_0000_42_00_0_vpd.xml b/tests/nodedevxml2xmlout/pci_0000_42_00_0_vpd.xml new file mode 120000 index 0000000000..a0b5372ca0 --- /dev/null +++ b/tests/nodedevxml2xmlout/pci_0000_42_00_0_vpd.xml @@ -0,0 +1 @@ +../nodedevschemadata/pci_0000_42_00_0_vpd.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index 9e32e7d553..557347fb07 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -121,6 +121,7 @@ mymain(void) DO_TEST("pci_0000_02_10_7_sriov_pf_vfs_all_header_type"); DO_TEST("drm_renderD129"); DO_TEST("pci_0000_02_10_7_mdev_types"); + DO_TEST("pci_0000_42_00_0_vpd"); DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b"); DO_TEST("ccw_0_0_ffff"); DO_TEST("css_0_0_ffff"); diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index f72359121f..63725587fc 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -476,6 +476,9 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) case VIR_NODE_DEV_CAP_MDEV: flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV; break; + case VIR_NODE_DEV_CAP_VPD: + flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPD; + break; case VIR_NODE_DEV_CAP_CCW_DEV: flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV; break; -- 2.30.2

On Mon, Sep 27, 2021 at 10:30:51PM +0300, Dmitrii Shcherbakov wrote:
* XML serialization and deserialization of PCI VPD resources; * PCI VPD capability flags added and used in relevant places; * XML to XML tests for the added capability.
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
diff --git a/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml b/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml new file mode 100644 index 0000000000..831b6feb24 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml @@ -0,0 +1,33 @@ +<device> + <name>pci_0000_42_00_0</name> + <capability type='pci'> + <class>0x020000</class> + <domain>0</domain> + <bus>66</bus> + <slot>0</slot> + <function>0</function> + <product id='0xa2d6'>MT42822 BlueField-2 integrated ConnectX-6 Dx network controller</product> + <vendor id='0x15b3'>Mellanox Technologies</vendor> + <capability type='virt_functions' maxCount='16'/> + <capability type='vpd'> + <resource type='string'>BlueField-2 DPU 25GbE Dual-Port SFP56, Crypto Enabled, 16GB on-board DDR, 1GbE OOB management, Tall Bracket</resource> + <resource type='vpd-r'> + <field keyword='EC'>B1</field> + <field keyword='PN'>MBF2H332A-AEEOT</field> + <field keyword='SN'>MT2113X00000</field> + <field keyword='V0'>PCIeGen4 x8</field> + <field keyword='V2'>MBF2H332A-AEEOT</field> + <field keyword='V3'>3c53d07eec484d8aab34dabd24fe575aa</field> + <field keyword='VA'>MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=BF2H332A</field>
I've got a general comment about what do any of these 2-letter keywords actually mean. I presume they are explaned in the PCI spec, but AFAICT the spec is not publically available for free. So at the very least we need to document each one's meaning in libvirt docs IMHO. If I had insight into what they meant, then I might also suggest giving them meaningful names in libvirt. These two level codes are presumably chosen for reasons of space efficiency at the low level. This is not a constraint we especially care about in libvirt, where ease of understanding is usually more important. 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 :|

On Fri, Oct 01, 2021 at 01:11:19PM +0100, Daniel P. Berrangé wrote:
On Mon, Sep 27, 2021 at 10:30:51PM +0300, Dmitrii Shcherbakov wrote:
* XML serialization and deserialization of PCI VPD resources; * PCI VPD capability flags added and used in relevant places; * XML to XML tests for the added capability.
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
diff --git a/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml b/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml new file mode 100644 index 0000000000..831b6feb24 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml @@ -0,0 +1,33 @@ +<device> + <name>pci_0000_42_00_0</name> + <capability type='pci'> + <class>0x020000</class> + <domain>0</domain> + <bus>66</bus> + <slot>0</slot> + <function>0</function> + <product id='0xa2d6'>MT42822 BlueField-2 integrated ConnectX-6 Dx network controller</product> + <vendor id='0x15b3'>Mellanox Technologies</vendor> + <capability type='virt_functions' maxCount='16'/> + <capability type='vpd'> + <resource type='string'>BlueField-2 DPU 25GbE Dual-Port SFP56, Crypto Enabled, 16GB on-board DDR, 1GbE OOB management, Tall Bracket</resource> + <resource type='vpd-r'> + <field keyword='EC'>B1</field> + <field keyword='PN'>MBF2H332A-AEEOT</field> + <field keyword='SN'>MT2113X00000</field> + <field keyword='V0'>PCIeGen4 x8</field> + <field keyword='V2'>MBF2H332A-AEEOT</field> + <field keyword='V3'>3c53d07eec484d8aab34dabd24fe575aa</field> + <field keyword='VA'>MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=BF2H332A</field>
I've got a general comment about what do any of these 2-letter keywords actually mean. I presume they are explaned in the PCI spec, but AFAICT the spec is not publically available for free.
So at the very least we need to document each one's meaning in libvirt docs IMHO.
If I had insight into what they meant, then I might also suggest giving them meaningful names in libvirt. These two level codes are presumably chosen for reasons of space efficiency at the low level. This is not a constraint we especially care about in libvirt, where ease of understanding is usually more important.
Ok, I found a copy of the PCI spec I see the data is classified at 2 levels, with the first level being: String Tag: This tag is the first item in the VPD storage component. It contains the name of the add-in card in alphanumeric characters. VPD-R Tag: This tag contains the read only VPD keywords for an add-in card. VPD-W Tag: This tag contains the read/write VPD keywords for an add-in card Then for VPD-R, the next level PN: Add-in Card Part Number EC: Engineering change level of the card FG: Fabric Geography LC: Location MN: Manufacture ID PG: PCI Geography SN: Serial number Vx: Vendor string (Repeated many times for 'x' in range 0-9, A-Z) CP: Extended capability RV: Checksum + reserved space I don't see a need to report the "CP" and "RV" tags in the XML. Then for VPD-W,the next level Vx: Vendor string (Repeated many times for 'x' in range 0-9, A-Z) Sx: System string (Repeated many times for 'x' in range 0-9, B-Z) YA: Asset tag With all this in mind, I think we a better to represent these all as meaingfully named elements <capability type="vpd"> <name>BlueField-2 DPU 25GbE Dual-Port SFP56, Crypto Enabled, 16GB on-board DDR, 1GbE OOB management, Tall Bracket</name> <tags access="readonly"> <change_level>B1</change_level> <part_number>MBF2H332A-AEEOT</part_number> <serial_number>MT2113X00000</serial_number> <vendor_string index="0">PCIeGen4 x8</vendor_string> <vendor_string index="2">MBF2H332A-AEEOT</vendor_string> <vendor_string index="3">3c53d07eec484d8aab34dabd24fe575aa</vendor_string> </tags> <tags access="readwrite"> ... </tags> </capability>
+ <capability type='vpd'> + <resource type='string'>BlueField-2 DPU 25GbE Dual-Port SFP56, Crypto Enabled, 16GB on-board DDR, 1GbE OOB management, Tall Bracket</resource> + <resource type='vpd-r'> + <field keyword='EC'>B1</field> + <field keyword='PN'>MBF2H332A-AEEOT</field> + <field keyword='SN'>MT2113X00000</field> + <field keyword='V0'>PCIeGen4 x8</field> + <field keyword='V2'>MBF2H332A-AEEOT</field> + <field keyword='V3'>3c53d07eec484d8aab34dabd24fe575aa</field>
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 :|

I've got a general comment about what do any of these 2-letter keywords actually mean. I presume they are explaned in the PCI spec, but AFAICT the spec is not publically available for free.
So at the very least we need to document each one's meaning in libvirt docs IMHO.
If I had insight into what they meant, then I might also suggest giving them meaningful names in libvirt. These two level codes are presumably chosen for reasons of space efficiency at the low level. This is not a constraint we especially care about in libvirt, where ease of understanding is usually more important.
Ok, I found a copy of the PCI spec
Ack, I could document those fields in a doc update for users to understand what to expect. Just a note: those specs are available if an e-mail of a PCI SIG member company is used during registration at the PCI SIG website so I think it should be available to you too.
I see the data is classified at 2 levels, with the first level being:
String Tag: This tag is the first item in the VPD storage component. It contains the name of the add-in card in alphanumeric characters.
VPD-R Tag: This tag contains the read only VPD keywords for an add-in card.
VPD-W Tag: This tag contains the read/write VPD keywords for an add-in card
Then for VPD-R, the next level
PN: Add-in Card Part Number EC: Engineering change level of the card FG: Fabric Geography LC: Location MN: Manufacture ID PG: PCI Geography SN: Serial number Vx: Vendor string (Repeated many times for 'x' in range 0-9, A-Z) CP: Extended capability RV: Checksum + reserved space
I don't see a need to report the "CP" and "RV" tags in the XML.
Agreed, "CP" is currently ignored and "RV" is used for checksum validation only in the current code. There is also the "RW" keyword which is used to identify an unused portion of the read-write space: AFAIU the read-write VPD memory will have some free space for future writes and the "RW" keyword value probably needs to be updated by the software making writes to this area. I have not actually seen any devices with a pre-programmed VPD-W yet but so far the focus has been on reading what's in there properly. There is only one string resource specified in the spec at the moment (the device name). I suppose we could assume that there will only be one and deal with possible VPD format changes as they arise. Likewise, for VPD-R and VPD-W: there is currently no use-case for having multiple instances for the same type - so maybe I was too forward-looking in this case (i.e. the format hasn't seen much change in years).
Then for VPD-W,the next level
Vx: Vendor string (Repeated many times for 'x' in range 0-9, A-Z) Sx: System string (Repeated many times for 'x' in range 0-9, B-Z) YA: Asset tag
With all this in mind, I think we a better to represent these all as meaingfully named elements
<capability type="vpd"> <name>BlueField-2 DPU 25GbE Dual-Port SFP56, Crypto Enabled, 16GB on-board DDR, 1GbE OOB management, Tall Bracket</name> <tags access="readonly"> <change_level>B1</change_level> <part_number>MBF2H332A-AEEOT</part_number> <serial_number>MT2113X00000</serial_number> <vendor_string index="0">PCIeGen4 x8</vendor_string> <vendor_string index="2">MBF2H332A-AEEOT</vendor_string> <vendor_string index="3">3c53d07eec484d8aab34dabd24fe575aa</vendor_string> </tags> <tags access="readwrite"> ... </tags> </capability>
Ack, this certainly has benefits for human readability but maybe introduces slightly more work to map human-readable names to spec keywords at the client side (not a big deal for the purposes I plan to use it though). Going to adjust what I have to match the above.

On 9/27/21 3:30 PM, Dmitrii Shcherbakov wrote: [...]
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c [...] + +static void +virNodeDeviceCapVPDResourceFormat(virBuffer *buf, virPCIVPDResource *res) +{ + GEnumValue *resRype = NULL; + void (*resFormatFunc)(virBuffer *buf, virPCIVPDResource *res); + + if (G_TYPE_CHECK_INSTANCE_TYPE(res, VIR_TYPE_PCI_VPD_STRING_RESOURCE)) { + resFormatFunc = virNodeDeviceCapVPDStringResourceFormat; + } else if (G_TYPE_CHECK_INSTANCE_TYPE(res, VIR_TYPE_PCI_VPD_KEYWORD_RESOURCE)) { + resFormatFunc = virNodeDeviceCapVPDKeywordResourceFormat; + } else { + /* Unexpected resource type. This should not happen unless the PCI(e) specs + * change and new resource types are introduced into util.virpcivpd. Either way, + * we can only return the control back to the caller here. + */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected VPD resource type encountered during formatting")); + return; + } + virBufferAdjustIndent(buf, 2); + + resRype = virPCIVPDResourceGetResourceType(res); + virBufferEscapeString(buf, "<resource type='%s'>", resRype->value_nick); + /* Format the resource in a type-specific way. */ + resFormatFunc(buf, res);
It's probably irrelevant since this will be mostly rewritten based on Dan's suggestions, but the resFormatFunc() pointer seems like an unnecessary complication - you could just move the if/else construct down to here and call the appropriate function directly.

It's probably irrelevant since this will be mostly rewritten based on Dan's suggestions, but the resFormatFunc() pointer seems like an unnecessary complication - you could just move the if/else construct down to here and call the appropriate function directly.
Ack, I was trying to reuse the same parent type and a function signature to handle resource formatting in a generic way - in case new resource types ever got added. But that was probably too forward-looking.

Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- NEWS.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 4617198f82..4db4e0e3e3 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -44,6 +44,13 @@ v7.8.0 (unreleased) to invoke the PCI VPD parser to get a list of resources representing PCI VPD contents in memory. + * nodedev: Add PCI VPD capability support + + Support for serializing and deserializing PCI VPD data structures is added + following the addition of the PCI VPD parser. A new PCI device capability + called "vpd" is introduced holding string resources and keyword resources + found in PCI VPD. + * **Improvements** * **Bug fixes** -- 2.30.2

Describes the format of the newly added VPD capability and gives and example for a real-world device. Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> --- docs/drvnodedev.html.in | 46 +++++++++++++++++++++++++++++++++++++++++ docs/formatnode.html.in | 24 ++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/docs/drvnodedev.html.in b/docs/drvnodedev.html.in index 70f7e6717d..438d20f2dd 100644 --- a/docs/drvnodedev.html.in +++ b/docs/drvnodedev.html.in @@ -185,6 +185,52 @@ </capability> </device></pre> + <h3><a id="VPDCap">VPD capability</a></h3> + <p> + A device that exposes a PCI/PCIe VPD capability will include a nested + capability <code>vpd</code> which presents all resources (string, read-only + and read-write keyword resources) that may be contained in a valid VPD + entry. The XML format follows the binary format described in + "I.3. VPD Definitions" in PCI Local Bus (2.2+) and the identical format + in PCIe 4.0+. At the time of writing, the support for exposing this capability + is only present on Linux-based systems (kernel version v2.6.26 is the first one + to expose VPD via sysfs which Libvirt relies on). Reading the VPD contents requires + root privileges, therefore, <code>virsh nodedev-dumpxml</code> must be executed + accordingly. + A description of the XML format for the <code>vpd</code> capability can + be found <a href="formatnode.html#VPDCap">here</a>. + </p> + <p> + The following example shows a VPD representation for a device that exposes the + VPD capability with a string resource and a VPD-R resource. Among other things, + the VPD of this particular device includes a unique board serial number in the + "SN" field. + </p> +<pre> +<device> + <!-- ... --> + <driver> + <name>mlx5_core</name> + </driver> + <capability type='pci'> + <!-- ... --> + <capability type='vpd'> + <resource type='string'>BlueField-2 DPU 25GbE Dual-Port SFP56, 16GB on-board DDR, 1GbE OOB management, Tall Bracket</resource> + <resource type='vpd-r'> + <field keyword='SN'>MT2113X00000</field> + <field keyword='V3'>d644e35a61d0eb11e000e8cef671bf3e</field> + <field keyword='VA'>MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=BF2H332A</field> + <field keyword='PN'>MBF2H332A-AEEOT</field> + <field keyword='V2'>MBF2H332A-AEEOT</field> + <field keyword='EC'>B1</field> + <field keyword='V0'>PCIeGen4 x8</field> + </resource> + </capability> + <!-- ... --> + </capability> +</device> +</pre> + <h2><a id="MDEV">Mediated devices (MDEVs)</a></h2> <p> Mediated devices (<span class="since">Since 3.2.0</span>) are software diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 3b3c3105d4..3e8c771398 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -162,7 +162,13 @@ This device is capable of creating mediated devices. The sub-elements are summarized in <a href="#MDEVTypesCap">mdev_types capability</a>. - </dd> + </dd> + <dt><code><a id="VPDCapPCI">vpd</a></code></dt> + <dd> + This device exposes a VPD PCI/PCIe capability. + The sub-elements are summarized in + <a href="#VPDCap">vpd capability</a>. + </dd> </dl> </dd> @@ -592,5 +598,21 @@ </device> </pre> + <h3><a id="VPDCap">vpd capability</a></h3> + + <p> + <a href="#VPDCapPCI">PCI</a> devices can expose a VPD capability which + is optional per PCI Local Bus 2.2+ and PCIe 4.0+ specifications. If + the VPD capability is present, then the parent <code>capability</code> + element with the <code>vpd</code> type will contain a list of + <code>resource</code> elements with contents applicable to a + particular resource type (string or keyword resources). The + <code>resource</code> element will either contain a string + resource value or a list of <code>field</code> elements in + the case of keyword resources. <code>field</code> elements + contain a <code>keyword</code> attribute corresponding to the + 2-byte VPD keyword and text as element data representing the + field value. + </p> </body> </html> -- 2.30.2
participants (3)
-
Daniel P. Berrangé
-
Dmitrii Shcherbakov
-
Laine Stump