On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
> Add support for deserializing the binary PCI/PCIe VPD format and storing
> results in memory.
>
> The VPD format is specified in "I.3. VPD Definitions" in PCI specs
> (2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0
> notes, the PCI Local Bus and PCIe VPD formats are binary compatible
> and PCIe 4.0 merely started incorporating what was already present in
> PCI specs.
>
> Linux kernel exposes a binary blob in the VPD format via sysfs since
> v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires
> a parser to interpret.
>
> A GTree is used as a data structure in order to maintain key ordering
> which will be important in XML to XML tests later.
>
> Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherba...@canonical.com>
> ---
> build-aux/syntax-check.mk | 4 +-
> po/POTFILES.in | 1 +
> src/libvirt_private.syms | 15 +
> src/util/meson.build | 1 +
> src/util/virpcivpd.c | 755 ++++++++++++++++++++++++++++++++++++++
> src/util/virpcivpd.h | 117 ++++++
> src/util/virpcivpdpriv.h | 45 +++
> tests/meson.build | 1 +
> tests/testutils.c | 40 ++
> tests/testutils.h | 4 +
> tests/virpcivpdtest.c | 705 +++++++++++++++++++++++++++++++++++
> 11 files changed, 1686 insertions(+), 2 deletions(-)
> create mode 100644 src/util/virpcivpd.c
> create mode 100644 src/util/virpcivpd.h
> create mode 100644 src/util/virpcivpdpriv.h
> create mode 100644 tests/virpcivpdtest.c
>
> 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 :|
