[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] ACPI: Add IORT Structure definition
From: |
Auger Eric |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] ACPI: Add IORT Structure definition |
Date: |
Thu, 13 Oct 2016 19:12:44 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
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
>