diff mbox

[1/2] ACPI: Add IORT Structure definition

Message ID 1476356143-10577-2-git-send-email-eric.auger@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Auger Oct. 13, 2016, 10:55 a.m. UTC 
From: Prem Mallappa <prem.mallappa@broadcom.com>

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 <prem.mallappa@broadcom.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/hw/acpi/acpi-defs.h | 91 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

Comments

Andrew Jones Oct. 13, 2016, 3 p.m. UTC | #1 
On Thu, Oct 13, 2016 at 12:55:42PM +0200, Eric Auger wrote:
> From: Prem Mallappa <prem.mallappa@broadcom.com>
> 
> 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 <prem.mallappa@broadcom.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  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         0x00000001
> +#define ACPI_IORT_NODE_NOT_COHERENT     0x00000000

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
Eric Auger Oct. 13, 2016, 5:12 p.m. UTC | #2 
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 <prem.mallappa@broadcom.com>
>>
>> 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 <prem.mallappa@broadcom.com>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  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         0x00000001
>> +#define ACPI_IORT_NODE_NOT_COHERENT     0x00000000
> 
> 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 
>
diff mbox

Patch

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         0x00000001
+#define ACPI_IORT_NODE_NOT_COHERENT     0x00000000
+
+/* 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