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

[Andrew Jones <drjones@redhat.com>]

On Fri, Oct 14, 2016 at 10:54:54AM +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>
> 
> ---
> 
> v1 -> v2:
> - took into account Drew's comments:
>   cleanup comments, remove most defines, add ACPI_IORT_NODE_HEADER_DEF
> ---
>  include/hw/acpi/acpi-defs.h | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 9c1b7cb..1cd1e69 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -609,4 +609,72 @@ 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 node types
> + */
> +
> +#define ACPI_IORT_NODE_HEADER_DEF   /* Node format common fields */ \
> +    uint8_t  type;          \
> +    uint16_t length;        \
> +    uint8_t  revision;      \
> +    uint32_t reserved;      \
> +    uint32_t mapping_count; \
> +    uint32_t mapping_offset;\

Last line shouldn't have the \

> +
> +/* Values for node 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;
> +
> +struct AcpiIortMemoryAccess {
> +    uint32_t cache_coherency;
> +    uint8_t  hints;
> +    uint16_t reserved;
> +    uint8_t  memory_flags;
> +} QEMU_PACKED;
> +typedef struct AcpiIortMemoryAccess AcpiIortMemoryAccess;
> +
> +struct AcpiIortItsGroup {
> +    ACPI_IORT_NODE_HEADER_DEF
> +    uint32_t its_count;
> +    uint32_t identifiers[0];
> +} QEMU_PACKED;
> +typedef struct AcpiIortItsGroup AcpiIortItsGroup;
> +
> +struct AcpiIortRC {
> +    ACPI_IORT_NODE_HEADER_DEF
> +    AcpiIortMemoryAccess memory_properties;
> +    uint32_t ats_attribute;
> +    uint32_t pci_segment_number;

I think just 'pci_segment' like in a couple other structs, is
a descriptive enough name, i.e. _number could be dropped.

> +    AcpiIortIdMapping id_mapping_array[0];
> +} QEMU_PACKED;
> +typedef struct AcpiIortRC AcpiIortRC;
> +
>  #endif
> -- 
> 2.5.5
>

Thanks,
drew