[libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser

Fri Oct 1 16:48:46 UTC 2021

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 :|