On 4/5/23 19:21, Andrea Bolognani wrote:
> 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.
Fair enough. I've found "SMMU v1/v2" shorter but I guess I don't care
that much.
>
>> +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?
I haven't. But, gcc complains if only a part of string contains
positioned arguments. That is:
"argument value %1$s and another argument %s"
"argument value %s and another argument %2$s"
are invalid format string and compiler throws an error. Now, using no
positions works:
"argument value %s and another argument %s"
but then, Jirka's syntax-check rule throws an error. But it doesn't for
the case I'm using:
"integer value %" PRIu16
Although, one could argue that if the translate tool doesn't allow fixed
size integer types modifiers, then the tool is broken. Surely, libvirt's
not the only one using fixed size integers. But okay, for error messages
I can typecast arguments. IOW:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("IORT table node type %1$s has invalid length: %2$u"),
NULLSTR(typeStr), (unsigned int)nodeHeader->len);
>
>> +++ 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?
>
Yes, except these fields are defined by the standard. We may need them
in the future. Anyway - these are thrown right away anyway, if it's
added memory consumption you're worried about.
By the same token, we don't care about other types (root comples, PMCG,
ITS group, ...) and yet we have them in the enum.
Michal
