Re: [PATCH 1/3] util: Introduce virAcpi module
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