[PATCH 0/3] virt-host-validate: Detect SMMU presence on ARMs by parsing IORT table
*** BLURB HERE *** Michal Prívozník (3): util: Introduce virAcpi module tests: Introduce viracpitest virt-host-validate: Detect SMMU presence on ARMs by parsing IORT table build-aux/syntax-check.mk | 2 +- 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 ++++++++ tests/meson.build | 1 + tests/viracpidata/IORT_ampere | Bin 0 -> 2304 bytes tests/viracpidata/IORT_empty | Bin 0 -> 65 bytes tests/viracpidata/IORT_gigabyte | Bin 0 -> 10100 bytes tests/viracpidata/IORT_qualcom | Bin 0 -> 3560 bytes tests/viracpidata/IORT_virt_aarch64 | Bin 0 -> 128 bytes tests/viracpitest.c | 135 +++++++++++++++++ tools/virt-host-validate-common.c | 18 ++- 15 files changed, 468 insertions(+), 4 deletions(-) create mode 100644 src/util/viracpi.c create mode 100644 src/util/viracpi.h create mode 100644 src/util/viracpipriv.h create mode 100644 tests/viracpidata/IORT_ampere create mode 100644 tests/viracpidata/IORT_empty create mode 100644 tests/viracpidata/IORT_gigabyte create mode 100644 tests/viracpidata/IORT_qualcom create mode 100644 tests/viracpidata/IORT_virt_aarch64 create mode 100644 tests/viracpitest.c -- 2.39.2
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.2
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
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 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
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
Introduce a test that checks newly introduced virArch module. There are three IORT tables from a real HW (IORT_ampere, IORT_gigabyte and IORT_qualcom), then there's one from a VM (IORT_virt_aarch64) and one that I handcrafted to be empty (IORT_empty). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- build-aux/syntax-check.mk | 2 +- tests/meson.build | 1 + tests/viracpidata/IORT_ampere | Bin 0 -> 2304 bytes tests/viracpidata/IORT_empty | Bin 0 -> 65 bytes tests/viracpidata/IORT_gigabyte | Bin 0 -> 10100 bytes tests/viracpidata/IORT_qualcom | Bin 0 -> 3560 bytes tests/viracpidata/IORT_virt_aarch64 | Bin 0 -> 128 bytes tests/viracpitest.c | 135 ++++++++++++++++++++++++++++ 8 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 tests/viracpidata/IORT_ampere create mode 100644 tests/viracpidata/IORT_empty create mode 100644 tests/viracpidata/IORT_gigabyte create mode 100644 tests/viracpidata/IORT_qualcom create mode 100644 tests/viracpidata/IORT_virt_aarch64 create mode 100644 tests/viracpitest.c diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 5829dc9011..64c1e2773e 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1370,7 +1370,7 @@ exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/vir(file|event)\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c)|tools/nss/libvirt_nss_(leases|macs)\.c)|tools/virt-qemu-qmp-proxy$$) exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ - (^tests/(nodedevmdevctl|virhostcpu|virpcitest|virstoragetest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) + (^tests/(nodedevmdevctl|viracpi|virhostcpu|virpcitest|virstoragetest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ (^(src/(util/(vircommand|virdaemon)|lxc/lxc_controller)|tests/testutils)\.c$$) diff --git a/tests/meson.build b/tests/meson.build index 24d08e4f97..35adbc2d56 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -269,6 +269,7 @@ tests += [ { 'name': 'storagevolxml2xmltest' }, { 'name': 'sysinfotest' }, { 'name': 'utiltest' }, + { 'name': 'viracpitest' }, { 'name': 'viralloctest' }, { 'name': 'virauthconfigtest' }, { 'name': 'virbitmaptest' }, diff --git a/tests/viracpidata/IORT_ampere b/tests/viracpidata/IORT_ampere new file mode 100644 index 0000000000000000000000000000000000000000..02e7fd98499729025346c6ac3ae5cf10b0f1074d GIT binary patch literal 2304 zcmbW1y-EW?6opUzV}1)8%LN4mK`kr-7P4TWg&4%rLXlu$L<trF3*SI2d;m)y!oo6- zU}@n4<PFw6a}$x7++mo7OlH2Bv)?d#caKg^&#(mGIla9fj0WlOVRV@!NluU+_qUdT z!dl+nA>g$lfAhAGcZwV=;3b8Z6<$$zRpB*-Hx%AfcuV1Jg?AL*&3Um09*JpqZe{?O zqhNo2j#<W4z|_q6b6jWV&dnkEb9^p#ZW55=mh9ZFc{qL_Z*Aw^0&-kp=UV2W`+Zzu z=f(j!){UL}3CPtK<oc!v{rlwWW&1t?a;#%Jx4}*1m#@xP=XUOOmMahCb!BZF@J7IQ zwg>YGkfFbw)ib1ii+>)bWKJ0S3tjuMt`ZLaZB1OlRM%>(tBecBB}{d#$GR#QJ1$|W zYctkW#f#$-rn<IcT{XNrE@7(cG0eq0f4rm)8~8LMX=F~A>UxTGHSy)RgsHBpSXT>E n$0ba4UB|lGsLR0_qjZI-uA5j_2Z`enrn>H8U0w7XmoVaAS)q0T literal 0 HcmV?d00001 diff --git a/tests/viracpidata/IORT_empty b/tests/viracpidata/IORT_empty new file mode 100644 index 0000000000000000000000000000000000000000..58b696fe935b432020377aabc76fe2b95376b48f GIT binary patch literal 65 ucmebD4+?Q$U|?Y6a`g=eiDdBcbPDqf3SnRbih&p)8U!4`B$SXqaJT?S&j#KA literal 0 HcmV?d00001 diff --git a/tests/viracpidata/IORT_gigabyte b/tests/viracpidata/IORT_gigabyte new file mode 100644 index 0000000000000000000000000000000000000000..b18b445b5de0911ba7190dfe396a72fec3fe6cf5 GIT binary patch literal 10100 zcmeI1OHRWu5Qg2REq(9^AS76TA~tNhV95qiBr266RV`x4f;U?ZP>CaO1rESrH~<IW z4#mG`Y%0}>oNhgq5>JSK2mi4?O{V>;(fI91sY?6o^twN}9A8X^z4OscMJd%Ejt6^< zo$Nrz$NF$>*F_2;#-b&+H51}AsNpZVy_<6t=eh}%I??@99|OL|*gKAGch7<JWQ6!2 zKEQ!k_5s#9o8kP=o+?8Ke981-U^Cb2&}l4Nd`wCMne>1O=h`F3(vuP5gZKakV%Y~+ zD>13!@}VLoSw2~g{cuiHq~RfsV<DP4{xRLkJf}gD1&xoFSs*W;cuw0rb09q#AwGx? za3Gd_fVC2nszJOoABj$*RNBT2{_o0eAd_Bb3U_|xNP03td=MYtKrH(JYb7QnK}<>@ z(9PynpX+p8l*6sikW!lF5}H<y#uVlFo~_Rz7xm?K>cb3_`W*WeOMOMwNiSS{Sk4Ei zh>1=0y-jlzF_BX_$HZUO_?VOiGD+%l<a;dj7g?vgZ#a;*4{(o|)Cyu!2Ljz}Za3xM zHH$dL>0c?$20f;fCe6|Kc$o$AQtESLYAp2^S*N^jIFPpwaF5v64&vo11iD#Xe(lq> zk4fD?CP{scJdCCOBI}g*4F~e}0qzlBIzdcYgFrXSq~0N27v*r9G^CVfYY7d1;x0Vr E57fpI9{>OV literal 0 HcmV?d00001 diff --git a/tests/viracpidata/IORT_qualcom b/tests/viracpidata/IORT_qualcom new file mode 100644 index 0000000000000000000000000000000000000000..5364a9968fa024f34b5e911513a08e8a7f88cf91 GIT binary patch literal 3560 zcmb7_yK7WI6vk)w?gfdc+XO5}jbI^i_hDy|WN$P9V~9y%A;c$GhzN?HK?NJFM9@YX zTmKYG!TbS&7AXbo*Yn+ZoZX!D-Z=xi^Z3p;bLMyV++Eway1jqI7;|ZJw6SjOW_htc zEDGDArL`;Dm*?|&yF%lFK2<v&2ChdOTIas0hHV;`)ylW=_^@${n9qH$<`_PppeG2V z_Q>~PpYhnO$Ma|k4ZC3t<Mxogs9|h<PhEz2XknjKzmVIYgnqBe=OLYs7@>C?blad8 z)4JsI;{czZk<Vkw>6z#9bS9q`KM%?s_r`81d;+7u=bm2>_cy;HAH)c~+o0P9y_nV| zpWg@g{E2*C5tCid<@HQHZNUe-rSS3FnZW0bUl8{<e<L5n2))~&+XlUu)+HaRh-2>A zSHEb~7tPV$Qa;ap-tT=s{oFIb2fL;4$=nf<&lkTS?r-+0eBROdh!HyKi}L-ij{2(8 zQC}0f<U<ufdbqclqxzyb+6T(%h3E2dCZCSrgWXd2blee<&sV=7?vMKN`6EW?sINL5 z^;M^%z9w|Zhbn?}=0o*GbF@#y<fZ5Gc_yD+@WE~=d~$b0<g@P=#QjlUK7Ygr9rabG zqrU2N)YpXGy5JH6k25|;jE~a)%Hs8%t>v9_<$8e|24;sU28c=X_M*3CwBCO7w$<K2 z^k!<W`=iAjJ8JLwcy6xs_L#F(dzteIwU;@cRC}58DYch5FQ~oD`Lx#CXU>aiFLPc} zdztgH+RL0rYA<sx)n4Yj(%`Lr`1D$c?~MU-zM=Lq=bLITbH1haGUwZBFLS=5_A=+Y zliu)B_HfbRy+kcraPQ}>a?~@eq_}=!+#oS-SQ~fN<#%m$R3ygL--Gw7zX!(+%s=~I BN)P}5 literal 0 HcmV?d00001 diff --git a/tests/viracpidata/IORT_virt_aarch64 b/tests/viracpidata/IORT_virt_aarch64 new file mode 100644 index 0000000000000000000000000000000000000000..7efd0ce8a6b3928efa7e1373f688ab4c5f50543b GIT binary patch literal 128 zcmebD4+?2uU|?Y0?Bwt45v<@85#X!<1dKp25F11@0kHuPgMkDCNC*yK93~3}W)K^M VRiHGGVg_O`aDdYP|3ers^8jQz3IPBB literal 0 HcmV?d00001 diff --git a/tests/viracpitest.c b/tests/viracpitest.c new file mode 100644 index 0000000000..c34abec47b --- /dev/null +++ b/tests/viracpitest.c @@ -0,0 +1,135 @@ +/* + * viracpitest.c: Test ACPI table parsing + * + * 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> + +#define LIBVIRT_VIRACPIPRIV_H_ALLOW +#include "testutils.h" +#include "viracpi.h" +#include "viracpipriv.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +typedef struct testAarch64SMMUData testAarch64SMMUData; +struct testAarch64SMMUData { + const char *filename; + ssize_t nnodes; + const virIORTNodeType *node_types; +}; + +static void +printBitmap(virBitmap *types) +{ + size_t i; + + for (i = 0; i < VIR_IORT_NODE_TYPE_LAST; i++) { + if (virBitmapIsBitSet(types, i)) { + fprintf(stderr, "%s\n", virIORTNodeTypeTypeToString(i)); + } + } +} + +static int +testAarch64SMMU(const void *opaque) +{ + const testAarch64SMMUData *data = opaque; + g_autofree char *path = NULL; + g_autofree virIORTNodeHeader *nodes = NULL; + ssize_t nnodes = 0; + + path = g_strdup_printf("%s/viracpidata/%s", + abs_srcdir, data->filename); + + nnodes = virAcpiParseIORT(&nodes, path); + + if (nnodes != data->nnodes) { + fprintf(stderr, + "virAcpiParseIORT() returned wrong number of nodes: %zd, expected %zd\n", + nnodes, data->nnodes); + return -1; + } + + if (nnodes > 0) { + g_autoptr(virBitmap) typesSeen = virBitmapNew(VIR_IORT_NODE_TYPE_LAST); + g_autoptr(virBitmap) typesExp = virBitmapNew(VIR_IORT_NODE_TYPE_LAST); + size_t i = 0; + + for (i = 0; data->node_types[i] != VIR_IORT_NODE_TYPE_LAST; i++) { + size_t type = data->node_types[i]; + + ignore_value(virBitmapSetBit(typesExp, type)); + } + + for (i = 0; i < nnodes; i++) { + virIORTNodeHeader *h = &nodes[i]; + + ignore_value(virBitmapSetBit(typesSeen, h->type)); + } + + if (!virBitmapEqual(typesSeen, typesExp)) { + fprintf(stderr, "node types mismatch.\n\nExpected:\n"); + printBitmap(typesExp); + fprintf(stderr, "\nActual:\n"); + printBitmap(typesSeen); + return -1; + } + } + + return 0; +} + + +static int +mymain(void) +{ + int ret = 0; + +#define DO_TEST(filename, nnodes, ...) \ + do { \ + const virIORTNodeType node_types[] = { __VA_ARGS__, VIR_IORT_NODE_TYPE_LAST }; \ + const testAarch64SMMUData data = {filename, nnodes, node_types }; \ + if (virTestRun("aarch64 SMMU " filename, testAarch64SMMU, &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST("IORT_empty", 0, VIR_IORT_NODE_TYPE_LAST); + DO_TEST("IORT_virt_aarch64", 2, + VIR_IORT_NODE_TYPE_ITS_GROUP, + VIR_IORT_NODE_TYPE_ROOT_COMPLEX); + DO_TEST("IORT_ampere", 36, + VIR_IORT_NODE_TYPE_ITS_GROUP, + VIR_IORT_NODE_TYPE_ROOT_COMPLEX, + VIR_IORT_NODE_TYPE_SMMU_V3); + DO_TEST("IORT_gigabyte", 30, + VIR_IORT_NODE_TYPE_ITS_GROUP, + VIR_IORT_NODE_TYPE_ROOT_COMPLEX, + VIR_IORT_NODE_TYPE_SMMU_V1_2); + DO_TEST("IORT_qualcom", 69, + VIR_IORT_NODE_TYPE_ITS_GROUP, + VIR_IORT_NODE_TYPE_NAMED_COMPONENT, + VIR_IORT_NODE_TYPE_ROOT_COMPLEX, + VIR_IORT_NODE_TYPE_SMMU_V3, + VIR_IORT_NODE_TYPE_PMCG); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) -- 2.39.2
On Wed, Apr 05, 2023 at 01:30:18PM +0200, Michal Privoznik wrote:
Introduce a test that checks newly introduced virArch module.
s/virArch/virAcpi/
+++ b/tests/viracpitest.c +static int +mymain(void) +{ + int ret = 0; + +#define DO_TEST(filename, nnodes, ...) \ + do { \ + const virIORTNodeType node_types[] = { __VA_ARGS__, VIR_IORT_NODE_TYPE_LAST }; \ + const testAarch64SMMUData data = {filename, nnodes, node_types }; \ + if (virTestRun("aarch64 SMMU " filename, testAarch64SMMU, &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST("IORT_empty", 0, VIR_IORT_NODE_TYPE_LAST); + DO_TEST("IORT_virt_aarch64", 2, + VIR_IORT_NODE_TYPE_ITS_GROUP, + VIR_IORT_NODE_TYPE_ROOT_COMPLEX); + DO_TEST("IORT_ampere", 36, + VIR_IORT_NODE_TYPE_ITS_GROUP, + VIR_IORT_NODE_TYPE_ROOT_COMPLEX, + VIR_IORT_NODE_TYPE_SMMU_V3); + DO_TEST("IORT_gigabyte", 30, + VIR_IORT_NODE_TYPE_ITS_GROUP, + VIR_IORT_NODE_TYPE_ROOT_COMPLEX, + VIR_IORT_NODE_TYPE_SMMU_V1_2); + DO_TEST("IORT_qualcom", 69,
s/qualcom/qualcomm/ Same change in the commit message. With those typos fixed, Signed-off-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On 4/5/23 19:24, Andrea Bolognani wrote:
On Wed, Apr 05, 2023 at 01:30:18PM +0200, Michal Privoznik wrote:
<snip/>
With those typos fixed,
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
I believe you meant Reviewed-by, correct? ;-) Michal
On Thu, Apr 06, 2023 at 10:41:01AM +0200, Michal Prívozník wrote:
On 4/5/23 19:24, Andrea Bolognani wrote:
With those typos fixed,
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
I believe you meant Reviewed-by, correct? ;-)
Oops! Of course O:-) -- Andrea Bolognani / Red Hat / Virtualization
In my previous commit v9.2.0-rc1~3 I've made virt-host-validate to report host IOMMU check pass if IORT table is present. This is not sufficient though, because IORT describes much more than just IOMMU (well, it's called SMMU in ARM world). In fact, this can be seen in previous commit which adds test cases: there are tables (IORT_virt_aarch64) which does not contain any SMMU records. But after previous commits, we can parse the table so switch to that. Fixes: 2c13a2a7c9c368ea81eccd4ba12d9cf34bdd331b Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virt-host-validate-common.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index e96986bb48..c8a23e2e99 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -26,6 +26,7 @@ #include <sys/utsname.h> #include <sys/stat.h> +#include "viracpi.h" #include "viralloc.h" #include "vircgroup.h" #include "virfile.h" @@ -389,13 +390,24 @@ int virHostValidateIOMMU(const char *hvname, } virHostMsgPass(); } else if (ARCH_IS_ARM(arch)) { - if (access("/sys/firmware/acpi/tables/IORT", F_OK) == 0) { - virHostMsgPass(); - } else { + if (access("/sys/firmware/acpi/tables/IORT", F_OK) != 0) { virHostMsgFail(level, "No ACPI IORT table found, IOMMU not " "supported by this hardware platform"); return VIR_HOST_VALIDATE_FAILURE(level); + } else { + rc = virAcpiHasSMMU(); + if (rc < 0) { + virHostMsgFail(level, + "Failed to parse ACPI IORT table"); + return VIR_HOST_VALIDATE_FAILURE(level); + } else if (rc == 0) { + virHostMsgFail(level, + "No SMMU found"); + return VIR_HOST_VALIDATE_FAILURE(level); + } else { + virHostMsgPass(); + } } } else { virHostMsgFail(level, -- 2.39.2
On Wed, Apr 05, 2023 at 01:30:19PM +0200, Michal Privoznik wrote:
In my previous commit v9.2.0-rc1~3 I've made virt-host-validate
You're including the commit hash below in the Fixes: pseudo-header, so mentioning it here again feels unnecessary. But it's fine if you prefer keeping this.
to report host IOMMU check pass if IORT table is present. This is not sufficient though, because IORT describes much more than just IOMMU (well, it's called SMMU in ARM world). In fact, this can be seen in previous commit which adds test cases: there are tables (IORT_virt_aarch64) which does not contain any SMMU records.
But after previous commits, we can parse the table so switch to that.
Fixes: 2c13a2a7c9c368ea81eccd4ba12d9cf34bdd331b
Personally I've grown to quite like the alternative take on this pseudo-header Fixes: 2c13a2a ("virt-host-validate: Detect SMMU support on ARMs") as seen very frequently in QEMU and other projects. I feel that it's more immediately informative than just the raw hash. But your version is perfectly okay too. Please include a reference to https://bugzilla.redhat.com/show_bug.cgi?id=2178885 though, since this is finishing the fix for that bug. Regardless of whether you decide to follow any of the suggestions above, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On 4/5/23 19:31, Andrea Bolognani wrote:
On Wed, Apr 05, 2023 at 01:30:19PM +0200, Michal Privoznik wrote:
In my previous commit v9.2.0-rc1~3 I've made virt-host-validate
You're including the commit hash below in the Fixes: pseudo-header, so mentioning it here again feels unnecessary. But it's fine if you prefer keeping this.
to report host IOMMU check pass if IORT table is present. This is not sufficient though, because IORT describes much more than just IOMMU (well, it's called SMMU in ARM world). In fact, this can be seen in previous commit which adds test cases: there are tables (IORT_virt_aarch64) which does not contain any SMMU records.
But after previous commits, we can parse the table so switch to that.
Fixes: 2c13a2a7c9c368ea81eccd4ba12d9cf34bdd331b
Personally I've grown to quite like the alternative take on this pseudo-header
Fixes: 2c13a2a ("virt-host-validate: Detect SMMU support on ARMs")
as seen very frequently in QEMU and other projects. I feel that it's more immediately informative than just the raw hash. But your version is perfectly okay too.
Since you're bringing this up, I find the QEMU style horrible. It boils down to why we include 'Fixes' in the first place. IMO, the part of commit message above is for humans, and that's why I tend to put output of: 'git describe $hash' or 'git describe --contains $hash' there because that gives me (as a human) a hint which version the regression occurred in (v9.2.0 in this case). But for 'Fixes:' field - my understanding is that it's for maintainers so that they know what other commits to backport. And thus it should be as unique as possible. But these abbreviated hashes are horrible in that sense. TLDR: the length of abbreviated hashes changes over time Longer version: So, when you do 'git log --oneline' and see those abbreviated hashes, for both libvirt.git and qemu.git they're both shortened to 10 characters. And it makes sense because each repo has relatively small number of objects in the pack (~42K for libvirt and ~703K for qemu; obtained via 'git count-objects -v' and looking at 'in-pack' value) and 10 chars are sufficient to guarantee uniqueness. But if you take a look at a bigger repo, say kernel.git (~9M objects), 10 characters are no longer sufficient and git needs to use 12 characters. IOW as the repository grows, abbreviated hashes are less and less unique. BTw: the code that picks the length for abbreviated hashes can be found here: https://github.com/git/git/blob/master/object-name.c#L762 We can expect length to bump to 11 soon-ish for qemu. Not to mention, that these calculations set the starting position, lower limit, after which git still has to walk through all hashes to make sure there's no conflict using that lower limit. Therefore, even adding one new commit can cause conflict on say 10 characters. And if I look at this from a downstream maintainer POV and my (probably unique) workflow, then whenever I backport a patch, I also bring up git-log (in less) and do a quick search of the hash '/$hash'. This finds all 'Fixes: $hash' lines and thus brings my attention to fixes of the commit I'm about to backport. Using these abbreviated hashes doesn't work with my workflow (obviously). Now, sure - I can search for just an abbreviated hash - but how much abbreviated? Also, what good is the commit subject? I certainly do not remember commits by their subjects. And thus I need to look at the commit anyway. For instance, from your example I'd still need to: git show $abbrevHash to understand what the commit did (okay, not in this example, because I'm author of both commits and wrote them in span of a a couple of days; but if I were a downstream maintainer you can bet I'd need to do that). Therefore, using: Fixes: $abbrevHash ($commitSubject) is a horrible idea that looks good on paper but in fact hurts everybody involved.
Please include a reference to
https://bugzilla.redhat.com/show_bug.cgi?id=2178885
though, since this is finishing the fix for that bug.
Yeah, there's still a case with device tree (i.e. no ACPI tables) so I was dubious whether to put it there. But you're right, it can't hurt.
Regardless of whether you decide to follow any of the suggestions above,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks, Michal
participants (4)
-
Andrea Bolognani -
Jiri Denemark -
Michal Privoznik -
Michal Prívozník