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

[Auger Eric <eric.auger@redhat.com>]

Hi Drew,

On 14/10/2016 13:30, Andrew Jones wrote:
> 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 \
OK
> 
>> +
>> +/* 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.
In the past Shannon told me to use the same field names as the ones in
the linux header, hence that choice.

Thanks

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