Re: [Qemu-devel] [PATCH 1/2] ACPI: Add IORT Structure definition
Hi Drew,
On 13/10/2016 17:00, Andrew Jones wrote:
> On Thu, Oct 13, 2016 at 12:55:42PM +0200, Eric Auger wrote:
>> From: Prem Mallappa
>>
>> ACPI Spec 6.0 introduces IO Remapping Table Structure. This patch
>> introduces the definitions required to describe the IO relationship
>> between the PCIe root complex and the ITS.
>>
>> This conforms to:
>> "IO Remapping Table System Software on ARM Platforms",
>> Document number: ARM DEN 0049B, October 2015.
>>
>> Signed-off-by: Prem Mallappa
>> Signed-off-by: Eric Auger
>> ---
>> include/hw/acpi/acpi-defs.h | 91
>> +
>> 1 file changed, 91 insertions(+)
>>
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index 9c1b7cb..9ad3c01 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -609,4 +609,95 @@ typedef struct AcpiDmarHardwareUnit
>> AcpiDmarHardwareUnit;
>> /* Masks for Flags field above */
>> #define ACPI_DMAR_INCLUDE_PCI_ALL 1
>>
>> +/*
>> + * Input Output Remapping Table (IORT)
>> + * Conforms to "IO Remapping Table System Software on ARM Platforms",
>> + * Document number: ARM DEN 0049B, October 2015
>> + */
>> +
>> +struct AcpiIortTable {
>> +ACPI_TABLE_HEADER_DEF /* ACPI common table header */
>> +uint32_t node_count;
>> +uint32_t node_offset;
>> +uint32_t reserved;
>> +} QEMU_PACKED;
>> +typedef struct AcpiIortTable AcpiIortTable;
>> +
>> +/*
>> + * IORT subtables
>
> s/subtables/Nodes/
subtable terminology was used in include/acpi/actbl2.h but well let's
simply remove that then.
>
>> + */
>> +
>> +struct AcpiIortNode {
>> +uint8_t type;
>> +uint16_t length;
>> +uint8_t revision;
>> +uint32_t reserved;
>> +uint32_t mapping_count;
>> +uint32_t mapping_offset;
>> +} QEMU_PACKED;
>> +typedef struct AcpiIortNode AcpiIortNode;
>
> The above should be a define like ACPI_TABLE_HEADER_DEF, as it's not
> a unique node type. The fields are just common node header fields.
OK
>
>> +
>> +/* Values for subtable Type above */
>
> s/subtable/node/
removed
>
>> +enum {
>> +ACPI_IORT_NODE_ITS_GROUP = 0x00,
>> +ACPI_IORT_NODE_NAMED_COMPONENT = 0x01,
>> +ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
>> +ACPI_IORT_NODE_SMMU = 0x03,
>> +ACPI_IORT_NODE_SMMU_V3 = 0x04
>> +};
>> +
>> +struct AcpiIortIdMapping {
>> +uint32_t input_base;
>> +uint32_t id_count;
>> +uint32_t output_base;
>> +uint32_t output_reference;
>> +uint32_t flags;
>> +} QEMU_PACKED;
>> +typedef struct AcpiIortIdMapping AcpiIortIdMapping;
>> +
>> +/* Masks for Flags field above for IORT subtable */
>> +#define ACPI_IORT_ID_SINGLE_MAPPING (1)
>
> We don't need to introduce defines/enums for all flags. Sometimes
> it makes the code easier to read, but sometimes it's just cruft.
> Everything is already documented in the spec, so a comment at
> assignment time is usually enough. See the SPCR generation for an
> example of attempting to minimize a reproduction of the spec.
OK. I just keep node type enum.
>
>> +
>> +struct AcpiIortMemoryAccess {
>> +uint32_t cache_coherency;
>> +uint8_t hints;
>> +uint16_t reserved;
>> +uint8_t memory_flags;
>> +} QEMU_PACKED;
>> +typedef struct AcpiIortMemoryAccess AcpiIortMemoryAccess;
>> +
>> +/* Values for cache_coherency field above */
>> +#define ACPI_IORT_NODE_COHERENT 0x0001
>> +#define ACPI_IORT_NODE_NOT_COHERENT 0x
>
> I'd replace these defines with comments at assignment time.
OK
>
>> +
>> +/* Masks for Hints field above */
>> +#define ACPI_IORT_HT_TRANSIENT (1)
>> +#define ACPI_IORT_HT_WRITE (1 << 1)
>> +#define ACPI_IORT_HT_READ (1 << 2)
>> +#define ACPI_IORT_HT_OVERRIDE (1 << 3)
>
> I'd drop these, I see they're not used anyway.
OK
>
>> +
>> +/* Masks for memory_flags field above */
>> +#define ACPI_IORT_MF_COHERENCY (1)
>> +#define ACPI_IORT_MF_ATTRIBUTES (1 << 1)
>
> And these can go.
>
OK
>> +
>> +/*
>> + * IORT node specific subtables
>> + */
>
> No need for the above header, we're already under
OK
Thanks
Eric
>
> /* IORT Nodes */
>
>> +
>> +struct AcpiIortItsGroup {
>> +AcpiIortNode iort_node;
>
> ACPI_IORT_NODE_HEADER_DEF
>
>> +uint32_t its_count;
>> +uint32_t identifiers[0];
>> +} QEMU_PACKED;
>> +typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>> +
>> +struct AcpiIortRC {
>> +AcpiIortNode iort_node;
>
> ACPI_IORT_NODE_HEADER_DEF
>
>> +AcpiIortMemoryAccess memory_properties;
>> +uint32_t ats_attribute;
>> +uint32_t pci_segment_number;
>> +AcpiIortIdMapping id_mapping_array[0];
>> +} QEMU_PACKED;
>> +typedef struct AcpiIortRC AcpiIortRC;
>> +
>> #endif
>> --
>> 2.5.5
>>
>>
>
> Thanks,
> drew
>
Re: [Qemu-devel] [PATCH 1/2] ACPI: Add IORT Structure definition
On Thu, Oct 13, 2016 at 12:55:42PM +0200, Eric Auger wrote:
> From: Prem Mallappa
>
> ACPI Spec 6.0 introduces IO Remapping Table Structure. This patch
> introduces the definitions required to describe the IO relationship
> between the PCIe root complex and the ITS.
>
> This conforms to:
> "IO Remapping Table System Software on ARM Platforms",
> Document number: ARM DEN 0049B, October 2015.
>
> Signed-off-by: Prem Mallappa
> Signed-off-by: Eric Auger
> ---
> include/hw/acpi/acpi-defs.h | 91
> +
> 1 file changed, 91 insertions(+)
>
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 9c1b7cb..9ad3c01 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -609,4 +609,95 @@ typedef struct AcpiDmarHardwareUnit AcpiDmarHardwareUnit;
> /* Masks for Flags field above */
> #define ACPI_DMAR_INCLUDE_PCI_ALL 1
>
> +/*
> + * Input Output Remapping Table (IORT)
> + * Conforms to "IO Remapping Table System Software on ARM Platforms",
> + * Document number: ARM DEN 0049B, October 2015
> + */
> +
> +struct AcpiIortTable {
> +ACPI_TABLE_HEADER_DEF /* ACPI common table header */
> +uint32_t node_count;
> +uint32_t node_offset;
> +uint32_t reserved;
> +} QEMU_PACKED;
> +typedef struct AcpiIortTable AcpiIortTable;
> +
> +/*
> + * IORT subtables
s/subtables/Nodes/
> + */
> +
> +struct AcpiIortNode {
> +uint8_t type;
> +uint16_t length;
> +uint8_t revision;
> +uint32_t reserved;
> +uint32_t mapping_count;
> +uint32_t mapping_offset;
> +} QEMU_PACKED;
> +typedef struct AcpiIortNode AcpiIortNode;
The above should be a define like ACPI_TABLE_HEADER_DEF, as it's not
a unique node type. The fields are just common node header fields.
> +
> +/* Values for subtable Type above */
s/subtable/node/
> +enum {
> +ACPI_IORT_NODE_ITS_GROUP = 0x00,
> +ACPI_IORT_NODE_NAMED_COMPONENT = 0x01,
> +ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
> +ACPI_IORT_NODE_SMMU = 0x03,
> +ACPI_IORT_NODE_SMMU_V3 = 0x04
> +};
> +
> +struct AcpiIortIdMapping {
> +uint32_t input_base;
> +uint32_t id_count;
> +uint32_t output_base;
> +uint32_t output_reference;
> +uint32_t flags;
> +} QEMU_PACKED;
> +typedef struct AcpiIortIdMapping AcpiIortIdMapping;
> +
> +/* Masks for Flags field above for IORT subtable */
> +#define ACPI_IORT_ID_SINGLE_MAPPING (1)
We don't need to introduce defines/enums for all flags. Sometimes
it makes the code easier to read, but sometimes it's just cruft.
Everything is already documented in the spec, so a comment at
assignment time is usually enough. See the SPCR generation for an
example of attempting to minimize a reproduction of the spec.
> +
> +struct AcpiIortMemoryAccess {
> +uint32_t cache_coherency;
> +uint8_t hints;
> +uint16_t reserved;
> +uint8_t memory_flags;
> +} QEMU_PACKED;
> +typedef struct AcpiIortMemoryAccess AcpiIortMemoryAccess;
> +
> +/* Values for cache_coherency field above */
> +#define ACPI_IORT_NODE_COHERENT 0x0001
> +#define ACPI_IORT_NODE_NOT_COHERENT 0x
I'd replace these defines with comments at assignment time.
> +
> +/* Masks for Hints field above */
> +#define ACPI_IORT_HT_TRANSIENT (1)
> +#define ACPI_IORT_HT_WRITE (1 << 1)
> +#define ACPI_IORT_HT_READ (1 << 2)
> +#define ACPI_IORT_HT_OVERRIDE (1 << 3)
I'd drop these, I see they're not used anyway.
> +
> +/* Masks for memory_flags field above */
> +#define ACPI_IORT_MF_COHERENCY (1)
> +#define ACPI_IORT_MF_ATTRIBUTES (1 << 1)
And these can go.
> +
> +/*
> + * IORT node specific subtables
> + */
No need for the above header, we're already under
/* IORT Nodes */
> +
> +struct AcpiIortItsGroup {
> +AcpiIortNode iort_node;
ACPI_IORT_NODE_HEADER_DEF
> +uint32_t its_count;
> +uint32_t identifiers[0];
> +} QEMU_PACKED;
> +typedef struct AcpiIortItsGroup AcpiIortItsGroup;
> +
> +struct AcpiIortRC {
> +AcpiIortNode iort_node;
ACPI_IORT_NODE_HEADER_DEF
> +AcpiIortMemoryAccess memory_properties;
> +uint32_t ats_attribute;
> +uint32_t pci_segment_number;
> +AcpiIortIdMapping id_mapping_array[0];
> +} QEMU_PACKED;
> +typedef struct AcpiIortRC AcpiIortRC;
> +
> #endif
> --
> 2.5.5
>
>
Thanks,
drew
[Qemu-devel] [PATCH 1/2] ACPI: Add IORT Structure definition
From: Prem Mallappa
ACPI Spec 6.0 introduces IO Remapping Table Structure. This patch
introduces the definitions required to describe the IO relationship
between the PCIe root complex and the ITS.
This conforms to:
"IO Remapping Table System Software on ARM Platforms",
Document number: ARM DEN 0049B, October 2015.
Signed-off-by: Prem Mallappa
Signed-off-by: Eric Auger
---
include/hw/acpi/acpi-defs.h | 91 +
1 file changed, 91 insertions(+)
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 9c1b7cb..9ad3c01 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -609,4 +609,95 @@ typedef struct AcpiDmarHardwareUnit AcpiDmarHardwareUnit;
/* Masks for Flags field above */
#define ACPI_DMAR_INCLUDE_PCI_ALL 1
+/*
+ * Input Output Remapping Table (IORT)
+ * Conforms to "IO Remapping Table System Software on ARM Platforms",
+ * Document number: ARM DEN 0049B, October 2015
+ */
+
+struct AcpiIortTable {
+ACPI_TABLE_HEADER_DEF /* ACPI common table header */
+uint32_t node_count;
+uint32_t node_offset;
+uint32_t reserved;
+} QEMU_PACKED;
+typedef struct AcpiIortTable AcpiIortTable;
+
+/*
+ * IORT subtables
+ */
+
+struct AcpiIortNode {
+uint8_t type;
+uint16_t length;
+uint8_t revision;
+uint32_t reserved;
+uint32_t mapping_count;
+uint32_t mapping_offset;
+} QEMU_PACKED;
+typedef struct AcpiIortNode AcpiIortNode;
+
+/* Values for subtable Type above */
+enum {
+ACPI_IORT_NODE_ITS_GROUP = 0x00,
+ACPI_IORT_NODE_NAMED_COMPONENT = 0x01,
+ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
+ACPI_IORT_NODE_SMMU = 0x03,
+ACPI_IORT_NODE_SMMU_V3 = 0x04
+};
+
+struct AcpiIortIdMapping {
+uint32_t input_base;
+uint32_t id_count;
+uint32_t output_base;
+uint32_t output_reference;
+uint32_t flags;
+} QEMU_PACKED;
+typedef struct AcpiIortIdMapping AcpiIortIdMapping;
+
+/* Masks for Flags field above for IORT subtable */
+#define ACPI_IORT_ID_SINGLE_MAPPING (1)
+
+struct AcpiIortMemoryAccess {
+uint32_t cache_coherency;
+uint8_t hints;
+uint16_t reserved;
+uint8_t memory_flags;
+} QEMU_PACKED;
+typedef struct AcpiIortMemoryAccess AcpiIortMemoryAccess;
+
+/* Values for cache_coherency field above */
+#define ACPI_IORT_NODE_COHERENT 0x0001
+#define ACPI_IORT_NODE_NOT_COHERENT 0x
+
+/* Masks for Hints field above */
+#define ACPI_IORT_HT_TRANSIENT (1)
+#define ACPI_IORT_HT_WRITE (1 << 1)
+#define ACPI_IORT_HT_READ (1 << 2)
+#define ACPI_IORT_HT_OVERRIDE (1 << 3)
+
+/* Masks for memory_flags field above */
+#define ACPI_IORT_MF_COHERENCY (1)
+#define ACPI_IORT_MF_ATTRIBUTES (1 << 1)
+
+/*
+ * IORT node specific subtables
+ */
+
+struct AcpiIortItsGroup {
+AcpiIortNode iort_node;
+uint32_t its_count;
+uint32_t identifiers[0];
+} QEMU_PACKED;
+typedef struct AcpiIortItsGroup AcpiIortItsGroup;
+
+struct AcpiIortRC {
+AcpiIortNode iort_node;
+AcpiIortMemoryAccess memory_properties;
+uint32_t ats_attribute;
+uint32_t pci_segment_number;
+AcpiIortIdMapping id_mapping_array[0];
+} QEMU_PACKED;
+typedef struct AcpiIortRC AcpiIortRC;
+
#endif
--
2.5.5
