Re: [Qemu-arm] [Qemu-devel] [PATCH 1/2] ACPI: Add IORT Structure definition

[Andrew Jones]

On Thu, Oct 13, 2016 at 12:55:42PM +0200, Eric Auger wrote:
> From: Prem Mallappa <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Eric Auger <address@hidden>
> ---
>  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