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.malla...@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.malla...@broadcom.com> >> Signed-off-by: Eric Auger <eric.au...@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 >