On Wed, Oct 20, 2021 at 11:30:31AM +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.
>
> Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@xxxxxxxxxxxxx>
> ---
> build-aux/syntax-check.mk | 4 +-
> po/POTFILES.in | 1 +
> src/libvirt_private.syms | 18 +
> src/util/meson.build | 1 +
> src/util/virpcivpd.c | 754 +++++++++++++++++++++++++++++++++++
> src/util/virpcivpd.h | 76 ++++
> src/util/virpcivpdpriv.h | 83 ++++
> tests/meson.build | 1 +
> tests/testutils.c | 35 ++
> tests/testutils.h | 4 +
> tests/virpcivpdtest.c | 809 ++++++++++++++++++++++++++++++++++++++
> 11 files changed, 1784 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 cb12b64532..2a6e2f86a1 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 b554cf08ca..8a726f624e 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 c5d788285e..444b51c880 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3576,6 +3576,24 @@ virVHBAManageVport;
> virVHBAPathExists;
>
>
> +# util/virpcivpd.h
> +
> +virPCIVPDParse;
> +virPCIVPDParseVPDLargeResourceFields;
> +virPCIVPDParseVPDLargeResourceString;
> +virPCIVPDReadVPDBytes;
> +virPCIVPDResourceCustomCompareIndex;
> +virPCIVPDResourceCustomFree;
> +virPCIVPDResourceCustomUpsertValue;
> +virPCIVPDResourceFree;
> +virPCIVPDResourceGetFieldValueFormat;
> +virPCIVPDResourceIsValidTextValue;
> +virPCIVPDResourceROFree;
> +virPCIVPDResourceRONew;
> +virPCIVPDResourceRWFree;
> +virPCIVPDResourceRWNew;
> +virPCIVPDResourceUpdateKeyword;
> +
> # 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..a208224228
> --- /dev/null
> +++ b/src/util/virpcivpd.c
> @@ -0,0 +1,754 @@
> +/*
> + * 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 "virthread.h"
> +#include "virpcivpdpriv.h"
> +#include "virlog.h"
> +#include "virerror.h"
> +#include "virfile.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +VIR_LOG_INIT("util.pcivpd");
> +
> +static bool
> +virPCIVPDResourceIsUpperOrNumber(const char c)
> +{
> + return g_ascii_isupper(c) || g_ascii_isdigit(c);
> +}
> +
> +static bool
> +virPCIVPDResourceIsVendorKeyword(const char *keyword)
> +{
> + return g_str_has_prefix(keyword, "V") && virPCIVPDResourceIsUpperOrNumber(keyword[1]);
> +}
> +
> +static bool
> +virPCIVPDResourceIsSystemKeyword(const char *keyword)
> +{
> + /* Special-case the system-specific keywords since they share the "Y" prefix with "YA". */
> + return (g_str_has_prefix(keyword, "Y") && virPCIVPDResourceIsUpperOrNumber(keyword[1]) &&
> + STRNEQ(keyword, "YA"));
> +}
> +
> +static char *
> +virPCIVPDResourceGetKeywordPrefix(const char *keyword)
> +{
> + g_autofree char *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 (!(virPCIVPDResourceIsUpperOrNumber(keyword[0]) &&
> + virPCIVPDResourceIsUpperOrNumber(keyword[1]))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("The keyword is not comprised only of uppercase ASCII letters or digits"));
> + 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);
> +}
> +
> +static GHashTable *fieldValueFormats;
> +
> +static int
> +virPCIVPDResourceOnceInit(void)
> +{
> + if (!fieldValueFormats) {
Since you're now using VIR_ONCE_GLOBAL_INIT, you are guaranteed
this only gets run once, so don't need the conditional check.
> + /* 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. NOTE: string literals are copied as g_hash_table_insert
> + * requires pointers to non-const data. */
> + 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));
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Field value formats must only be initialized once."));
> + return -1;
> + }
> + return 0;
> +}
> +
> +VIR_ONCE_GLOBAL_INIT(virPCIVPDResource);
> +
> +/**
> + * 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 char *keyword)
> +{
> + g_autofree char *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 ()... with a single line body don't use "{}" by libvirt convention.
I see this in multiple places through this series.
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 :|
