The aim of this new module is to contain code that's parsing ACPI
tables. For now, only parsing of IORT table is implemented (it's
ARM specific table). And since we only need to check whether the
table contains SMMU record, the code is very simplified.
I've followed the specification published here:
https://developer.arm.com/documentation/den0049/latest/
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
po/POTFILES | 1 +
src/libvirt_private.syms | 7 ++
src/util/meson.build | 1 +
src/util/viracpi.c | 225 +++++++++++++++++++++++++++++++++++++++
src/util/viracpi.h | 23 ++++
src/util/viracpipriv.h | 59 ++++++++++
6 files changed, 316 insertions(+)
create mode 100644 src/util/viracpi.c
create mode 100644 src/util/viracpi.h
create mode 100644 src/util/viracpipriv.h
diff --git a/po/POTFILES b/po/POTFILES
index fa769a8a95..b122f02818 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -244,6 +244,7 @@ src/storage_file/storage_source.c
src/storage_file/storage_source_backingstore.c
src/test/test_driver.c
src/util/iohelper.c
+src/util/viracpi.c
src/util/viralloc.c
src/util/virarptable.c
src/util/viraudit.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e37373c3c9..1247b67a39 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1829,6 +1829,13 @@ vir_g_strdup_printf;
vir_g_strdup_vprintf;
+# util/viracpi.c
+virAcpiHasSMMU;
+virAcpiParseIORT;
+virIORTNodeTypeTypeFromString;
+virIORTNodeTypeTypeToString;
+
+
# util/viralloc.h
virAppendElement;
virDeleteElementsN;
diff --git a/src/util/meson.build b/src/util/meson.build
index c81500ea04..2fe6f7699e 100644
--- a/src/util/meson.build
+++ b/src/util/meson.build
@@ -1,5 +1,6 @@
util_sources = [
'glibcompat.c',
+ 'viracpi.c',
'viralloc.c',
'virarch.c',
'virarptable.c',
diff --git a/src/util/viracpi.c b/src/util/viracpi.c
new file mode 100644
index 0000000000..e7d3ce2356
--- /dev/null
+++ b/src/util/viracpi.c
@@ -0,0 +1,225 @@
+/*
+ * viracpi.c: ACPI table(s) parser
+ *
+ * Copyright (C) 2023 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+
+#include <inttypes.h>
+#include <fcntl.h>
+
+#define LIBVIRT_VIRACPIPRIV_H_ALLOW
+#include "internal.h"
+#include "viracpi.h"
+#include "viracpipriv.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virfile.h"
+#include "virlog.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("util.acpi");
+
+typedef struct virIORTHeader virIORTHeader;
+struct virIORTHeader {
+ uint32_t signature;
+ uint32_t length;
+ uint8_t revision;
+ uint8_t checksum;
+ char oem_id[6];
+ char oem_table_id[8];
+ char oem_revision[4];
+ char creator_id[4];
+ char creator_revision[4];
+ /* Technically, the following are not part of header, but
+ * they immediately follow the header and are in the table
+ * exactly once. */
+ uint32_t nnodes;
+ uint32_t nodes_offset;
+ /* Here follows reserved and padding fields. Ain't nobody's
+ * interested in that. */
+} ATTRIBUTE_PACKED;
+
+VIR_ENUM_IMPL(virIORTNodeType,
+ VIR_IORT_NODE_TYPE_LAST,
+ "ITS Group",
+ "Named Component",
+ "Root Complex",
+ "SMMU v1/v2",
+ "SMMU v3",
+ "PMCG",
+ "Memory range");
+
+
+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);
+
+ /* 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),
+ NULLSTR(typeStr), nodeHeader->len);
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static ssize_t
+virAcpiParseIORTNodes(int fd,
+ const char *filename,
+ const virIORTHeader *header,
+ virIORTNodeHeader **nodesRet)
+{
+ g_autofree virIORTNodeHeader *nodes = NULL;
+ size_t nnodes = 0;
+ off_t pos;
+
+ /* Firstly, reset position to the start of nodes. */
+ if ((pos = lseek(fd, header->nodes_offset, SEEK_SET)) < 0) {
+ virReportSystemError(errno,
+ _("cannot seek in '%1$s'"),
+ filename);
+ return -1;
+ }
+
+ for (; pos < header->length;) {
+ virIORTNodeHeader node;
+
+ if (virAcpiParseIORTNodeHeader(fd, filename, &node) < 0)
+ return -1;
+
+ if ((pos = lseek(fd, pos + node.len, SEEK_SET)) < 0) {
+ virReportSystemError(errno,
+ _("cannot seek in '%1$s'"),
+ filename);
+ return -1;
+ }
+
+ VIR_APPEND_ELEMENT(nodes, nnodes, node);
+ }
+
+ *nodesRet = g_steal_pointer(&nodes);
+ return nnodes;
+}
+
+
+ssize_t
+virAcpiParseIORT(virIORTNodeHeader **nodesRet,
+ const char *filename)
+{
+ VIR_AUTOCLOSE fd = -1;
+ g_autofree char *headerBuf = NULL;
+ int headerLen;
+ virIORTHeader header;
+
+ if ((fd = open(filename, O_RDONLY)) < 0) {
+ virReportSystemError(errno,
+ _("cannot open '%1$s'"),
+ filename);
+ return -1;
+ }
+
+ headerLen = virFileReadHeaderFD(fd, sizeof(header), &headerBuf);
+ if (headerLen < 0) {
+ virReportSystemError(errno,
+ _("cannot read header '%1$s'"),
+ filename);
+ return -1;
+ }
+
+ if (headerLen != sizeof(header)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("IORT table header ended early"));
+ return -1;
+ }
+
+ memcpy(&header, headerBuf, headerLen);
+
+ VIR_DEBUG("IORT header: len = %" PRIu32 " revision = %" PRIu8
+ " nnodes = %" PRIu32 " OEM = %s",
+ header.length, header.revision,
+ header.nnodes, header.oem_id);
+
+ return virAcpiParseIORTNodes(fd, filename, &header, nodesRet);
+}
+
+
+#define IORT_PATH "/sys/firmware/acpi/tables/IORT"
+
+/**
+ * virAcpiHasSMMU:
+ *
+ * Parse IORT table trying to find SMMU node entry.
+ * Since IORT is ARM specific ACPI table, it doesn't make much
+ * sense to call this function on other platforms and expect
+ * sensible result.
+ *
+ * Returns: 0 if no SMMU node was found,
+ * 1 if a SMMU node was found (i.e. host supports SMMU),
+ * -1 otherwise (with error reported).
+ */
+int
+virAcpiHasSMMU(void)
+{
+ g_autofree virIORTNodeHeader *nodes = NULL;
+ ssize_t nnodes = -1;
+ size_t i;
+
+ if ((nnodes = virAcpiParseIORT(&nodes, IORT_PATH)) < 0)
+ return -1;
+
+ for (i = 0; i < nnodes; i++) {
+ if (nodes[i].type == VIR_IORT_NODE_TYPE_SMMU_V1_2 ||
+ nodes[i].type == VIR_IORT_NODE_TYPE_SMMU_V3)
+ return 1;
+ }
+
+ return 0;
+}
diff --git a/src/util/viracpi.h b/src/util/viracpi.h
new file mode 100644
index 0000000000..5a433b893a
--- /dev/null
+++ b/src/util/viracpi.h
@@ -0,0 +1,23 @@
+/*
+ * viracpi.h: ACPI table(s) parser
+ *
+ * Copyright (C) 2023 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+int virAcpiHasSMMU(void);
diff --git a/src/util/viracpipriv.h b/src/util/viracpipriv.h
new file mode 100644
index 0000000000..f98296632c
--- /dev/null
+++ b/src/util/viracpipriv.h
@@ -0,0 +1,59 @@
+/*
+ * viracpipriv.h: Functions for testing virAcpi APIs
+ *
+ * Copyright (C) 2023 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef LIBVIRT_VIRACPIPRIV_H_ALLOW
+# error "viracpipriv.h may only be included by viracpi.c or test suites"
+#endif /* LIBVIRT_VIRACPIPRIV_H_ALLOW */
+
+#pragma once
+
+#include <inttypes.h>
+
+#include "internal.h"
+#include "virenum.h"
+
+typedef enum {
+ VIR_IORT_NODE_TYPE_UNKNOWN = -1,
+ 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,
+ 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;
+} ATTRIBUTE_PACKED;
+
+ssize_t
+virAcpiParseIORT(virIORTNodeHeader **nodesRet,
+ const char *filename);
--
2.39.2On 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
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
On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
> On 4/5/23 19:21, Andrea Bolognani wrote:
> > On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
> >> + 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
Yeah, either your usage is fine or the syntax-check rule should be
improve to catch it. Jirka?
> 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);
This looks fine.
Perhaps a bit more useful:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("IORT table node type %1$s has invalid length: got
%2$u, expected at least %3$lu"),
NULLSTR(typeStr), (unsigned int)nodeHeader->len,
sizeof(*nodeHeader));
> >> +typedef enum {
> >> + VIR_IORT_NODE_TYPE_UNKNOWN = -1,
> >
> > Do we need this? virIORTNodeHeader.type is defined as unsigned.
You didn't answer this one :)
> >> +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.
It's fine to keep them, I'm not too concerned about the memory usage,
I was just wondering how much we should push towards minimalism ;)
What about renaming @reference_id to @mappings_offset though, to
match the corresponding fields for nodes in virIORTHeader?
--
Andrea Bolognani / Red Hat / Virtualization
On Thu, Apr 06, 2023 at 02:20:31 -0700, Andrea Bolognani wrote:
> On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
> > On 4/5/23 19:21, Andrea Bolognani wrote:
> > > On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
> > >> + 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
>
> Yeah, either your usage is fine or the syntax-check rule should be
> improve to catch it. Jirka?
Yeah, the syntax check is quite simple so it doesn't catch all possible
cases and libvirt-pot-check run after libvirt-pot (either in our CI or
manually) will detect all cases.
> > 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
Well, not really. The tool is not a C preprocessor so it doesn't have
any idea what a specific macro you concatenate with an actual string is.
So the best thing to do here is to avoid the situation and do what you
came up with here:
> > 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);
Jirka
On Wed, Apr 12, 2023 at 12:37:39PM +0200, Jiri Denemark wrote:
> On Thu, Apr 06, 2023 at 02:20:31 -0700, Andrea Bolognani wrote:
> > On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
> > > On 4/5/23 19:21, Andrea Bolognani wrote:
> > > > On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
> > > >> + 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
> >
> > Yeah, either your usage is fine or the syntax-check rule should be
> > improve to catch it. Jirka?
>
> Yeah, the syntax check is quite simple so it doesn't catch all possible
> cases and libvirt-pot-check run after libvirt-pot (either in our CI or
> manually) will detect all cases.
Running libvirt-pot updates po/libvirt.pot as such:
-"IORT table node type %1$s has invalid length: got %2$u, expected at least "
-"%3$zu"
+"IORT table node type %1$s has invalid length: got %2$<PRIu16>, expected at "
+"least %3$zu"
Running libvirt-pot-check afterwards doesn't result in a failure. So,
if this is something that we don't want in our pot file, we need to
find a way to detect and report it.
Note that changing the message as described above causes it to trip
up the libvirt_unmarked_diagnostics syntax-check rule. This was not
the case in Michal's original version of the patch, where PRIu16 was
used at the very end of the format string instead of in the middle of
it.
> So the best thing to do here is to avoid the situation and do what you
> came up with here:
>
> > > 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);
Right, but ideally we wouldn't need to depend on humans noticing :)
To be honest, I'm still unclear on whether allowing this in would
actually have been a problem. Jirka, can you please explicitly
confirm one way or the other?
--
Andrea Bolognani / Red Hat / Virtualization
On 4/6/23 11:20, Andrea Bolognani wrote:
> On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
>> On 4/5/23 19:21, Andrea Bolognani wrote:
>>> On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
>>>> + 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
>
> Yeah, either your usage is fine or the syntax-check rule should be
> improve to catch it. Jirka?
>
>> 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);
>
> This looks fine.
>
> Perhaps a bit more useful:
>
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("IORT table node type %1$s has invalid length: got
> %2$u, expected at least %3$lu"),
> NULLSTR(typeStr), (unsigned int)nodeHeader->len,
> sizeof(*nodeHeader));
>
>>>> +typedef enum {
>>>> + VIR_IORT_NODE_TYPE_UNKNOWN = -1,
>>>
>>> Do we need this? virIORTNodeHeader.type is defined as unsigned.
>
> You didn't answer this one :)
That's because I removed it in v2 ;-)
>
>>>> +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.
>
> It's fine to keep them, I'm not too concerned about the memory usage,
> I was just wondering how much we should push towards minimalism ;)
>
> What about renaming @reference_id to @mappings_offset though, to
> match the corresponding fields for nodes in virIORTHeader?
>
I can do that, but then again - it's named 'Reference to ID Array' in
the spec. And since I renamed SMMU_V1_2 to match whatever was in the
spec I figured staying close to the standard is key. Or how do we decide
when to stay true to the standard an when not?
If anything, it should be named reference_to_id_array. But I believe
that anybody who will extend this implementation will do the same as I
did - open the code and the spec next to each other and try to find a
match between tables in spec and structs in our code. At that point,
whether this field is named 'reference_id', 'mappings_offset' or
'reference_to_id_array' doesn't really matter.
Michal
On Thu, Apr 06, 2023 at 12:02:01PM +0200, Michal Prívozník wrote:
> On 4/6/23 11:20, Andrea Bolognani wrote:
> > On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
> >>>> +typedef enum {
> >>>> + VIR_IORT_NODE_TYPE_UNKNOWN = -1,
> >>>
> >>> Do we need this? virIORTNodeHeader.type is defined as unsigned.
> >
> > You didn't answer this one :)
>
> That's because I removed it in v2 ;-)
I hadn't realized you had posted that already O:-)
> > What about renaming @reference_id to @mappings_offset though, to
> > match the corresponding fields for nodes in virIORTHeader?
>
> I can do that, but then again - it's named 'Reference to ID Array' in
> the spec. And since I renamed SMMU_V1_2 to match whatever was in the
> spec I figured staying close to the standard is key. Or how do we decide
> when to stay true to the standard an when not?
>
> If anything, it should be named reference_to_id_array. But I believe
> that anybody who will extend this implementation will do the same as I
> did - open the code and the spec next to each other and try to find a
> match between tables in spec and structs in our code. At that point,
> whether this field is named 'reference_id', 'mappings_offset' or
> 'reference_to_id_array' doesn't really matter.
Yeah, it's unfortunate that the standard itself didn't adopt a
reasonable and consistent naming convention. Let's stick to what
they've chosen for now.
--
Andrea Bolognani / Red Hat / Virtualization
© 2016 - 2026 Red Hat, Inc.