Re: [PATCH 1/3] util: Introduce virAcpi module

5 Apr 2023


      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