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

[Auger Eric]

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 <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/
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 
>