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
