[PATCH 1/3] util: Introduce virAcpi module

[Andrea Bolognani]

On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
> +++ b/src/util/viracpi.c
> +VIR_ENUM_IMPL(virIORTNodeType,
> +              VIR_IORT_NODE_TYPE_LAST,
> +              "ITS Group",
> +              "Named Component",
> +              "Root Complex",
> +              "SMMU v1/v2",
> +              "SMMU v3",
> +              "PMCG",
> +              "Memory range");

Just a thought: it could be nice to have these match exactly the
strings found in the specification, i.e.

  VIR_ENUM_IMPL(virIORTNodeType,
                VIR_IORT_NODE_TYPE_LAST,
                "ITS Group",
                "Named component",
                "Root complex",
                "SMMUv1 or SMMUv2",
                "SMMUv3",
                "PMCG",
                "Memory range");

But it's fine to leave things as they are.

> +static int
> +virAcpiParseIORTNodeHeader(int fd,
> +                           const char *filename,
> +                           virIORTNodeHeader *nodeHeader)
> +{
> +    g_autofree char *nodeHeaderBuf = NULL;
> +    const char *typeStr = NULL;
> +    int nodeHeaderLen;
> +
> +    nodeHeaderLen = virFileReadHeaderFD(fd, sizeof(*nodeHeader), &nodeHeaderBuf);
> +    if (nodeHeaderLen < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot read node header '%1$s'"),
> +                             filename);
> +        return -1;
> +    }
> +
> +    if (nodeHeaderLen != sizeof(*nodeHeader)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("IORT table node header ended early"));
> +        return -1;
> +    }
> +
> +    memcpy(nodeHeader, nodeHeaderBuf, nodeHeaderLen);
> +
> +    typeStr = virIORTNodeTypeTypeToString(nodeHeader->type);
> +
> +    VIR_DEBUG("IORT node header: type = %" PRIu8 " (%s) len = %" PRIu16,
> +              nodeHeader->type, typeStr, nodeHeader->len);

Missing NULLSTR() call around typeStr here.

> +    /* Basic sanity check. While there's a type specific data
> +     * that follows the node header, the node length should be at
> +     * least size of header itself. */
> +    if (nodeHeader->len < sizeof(*nodeHeader)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("IORT table node type %1$s has invalid length: %2$" PRIu16),

I'm not sure this plays well with the recently introduced changes to
translatable strings. Have you checked with Jirka?

> +++ b/src/util/viracpipriv.h
> +typedef enum {
> +    VIR_IORT_NODE_TYPE_UNKNOWN = -1,

Do we need this? virIORTNodeHeader.type is defined as unsigned.

> +    VIR_IORT_NODE_TYPE_ITS_GROUP = 0,
> +    VIR_IORT_NODE_TYPE_NAMED_COMPONENT,
> +    VIR_IORT_NODE_TYPE_ROOT_COMPLEX,
> +    VIR_IORT_NODE_TYPE_SMMU_V1_2,
> +    VIR_IORT_NODE_TYPE_SMMU_V3,
> +    VIR_IORT_NODE_TYPE_PMCG,
> +    VIR_IORT_NODE_TYPE_MEMORY_RANGE,

Same comment as above for the names of these. Again, the current ones
are fine.

> +    VIR_IORT_NODE_TYPE_LAST,
> +} virIORTNodeType;
> +
> +VIR_ENUM_DECL(virIORTNodeType);
> +
> +typedef struct virIORTNodeHeader virIORTNodeHeader;
> +struct virIORTNodeHeader {
> +    uint8_t type; /* One of virIORTNodeType */
> +    uint16_t len;
> +    uint8_t revision;
> +    uint32_t identifier;
> +    uint32_t nmappings;
> +    uint32_t reference_id;

Since we only care about type and length, we could simply not declare
the other four fields at all, right?

-- 
Andrea Bolognani / Red Hat / Virtualization