[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 29/35] acpi: arm/virt: convert build_iort() to endian agno
From: |
Eric Auger |
Subject: |
Re: [PATCH v3 29/35] acpi: arm/virt: convert build_iort() to endian agnostic build_append_FOO() API |
Date: |
Wed, 22 Sep 2021 17:31:49 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 |
Hi Igor,
On 9/22/21 3:54 PM, Igor Mammedov wrote:
> On Wed, 22 Sep 2021 15:26:49 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
> I'll fix patch up as suggested,
> though there is a question, see below
>
>> On 9/7/21 4:48 PM, Igor Mammedov wrote:
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> v3:
>>> * practically rewritten, due to conflicts wiht bypass iommu feature
>>>
>>> CC: drjones@redhat.com
>>> CC: peter.maydell@linaro.org
>>> CC: shannon.zhaosl@gmail.com
>>> CC: qemu-arm@nongnu.org
>>> CC: wangxingang5@huawei.com
>>> CC: Eric Auger <eric.auger@redhat.com>
>>> ---
>>> include/hw/acpi/acpi-defs.h | 71 ---------------
>>> hw/arm/virt-acpi-build.c | 167 ++++++++++++++++++++----------------
>>> 2 files changed, 91 insertions(+), 147 deletions(-)
>>>
>>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>> index 195f90caf6..6f2f08a9de 100644
>>> --- a/include/hw/acpi/acpi-defs.h
>>> +++ b/include/hw/acpi/acpi-defs.h
>>> @@ -188,75 +188,4 @@ struct AcpiGenericTimerTable {
>>> } QEMU_PACKED;
>>> typedef struct AcpiGenericTimerTable AcpiGenericTimerTable;
>>>
>>> -/*
>>> - * 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;
>>> -
>>> -/* 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[];
>>> -} QEMU_PACKED;
>>> -typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>>> -
>>> -#define ACPI_IORT_SMMU_V3_COHACC_OVERRIDE 1
>>> -
>>> -struct AcpiIortSmmu3 {
>>> - ACPI_IORT_NODE_HEADER_DEF
>>> - uint64_t base_address;
>>> - uint32_t flags;
>>> - uint32_t reserved2;
>>> - uint64_t vatos_address;
>>> - uint32_t model;
>>> - uint32_t event_gsiv;
>>> - uint32_t pri_gsiv;
>>> - uint32_t gerr_gsiv;
>>> - uint32_t sync_gsiv;
>>> - AcpiIortIdMapping id_mapping_array[];
>>> -} QEMU_PACKED;
>>> -typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
>>> -
>>> -struct AcpiIortRC {
>>> - ACPI_IORT_NODE_HEADER_DEF
>>> - AcpiIortMemoryAccess memory_properties;
>>> - uint32_t ats_attribute;
>>> - uint32_t pci_segment_number;
>>> - AcpiIortIdMapping id_mapping_array[];
>>> -} QEMU_PACKED;
>>> -typedef struct AcpiIortRC AcpiIortRC;
>>> -
>>> #endif
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index bceb1b3b7e..4c682e7b09 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -240,6 +240,28 @@ static void acpi_dsdt_add_tpm(Aml *scope,
>>> VirtMachineState *vms)
>>> }
>>> #endif
>>>
>>> +#define ID_MAPPING_ENTRY_SIZE 20
>>> +#define SMMU_V3_ENTRY_SIZE 60
>>> +#define ROOT_COMPLEX_ENTRY_SIZE 32
>>> +#define IORT_NODE_OFFSET 48
>>> +
>>> +static void build_iort_id_mapping(GArray *table_data, uint32_t input_base,
>>> + uint32_t id_count, uint32_t out_ref)
>>> +{
>>> + /* Identity RID mapping covering the whole input RID range */
>>> + build_append_int_noprefix(table_data, input_base, 4); /* Input base */
>>> + build_append_int_noprefix(table_data, id_count, 4); /* Number of IDs */
>>> + build_append_int_noprefix(table_data, input_base, 4); /* Output base */
>>> + build_append_int_noprefix(table_data, out_ref, 4); /* Output Reference
>>> */
>>> + build_append_int_noprefix(table_data, 0, 4); /* Flags */
>>> +}
>>> +
>>> +struct AcpiIortIdMapping {
>>> + uint32_t input_base;
>>> + uint32_t id_count;
>>> +};
>>> +typedef struct AcpiIortIdMapping AcpiIortIdMapping;
>>> +
>>> /* Build the iort ID mapping to SMMUv3 for a given PCI host bridge */
>>> static int
>>> iort_host_bridges(Object *obj, void *opaque)
>>> @@ -281,18 +303,17 @@ static int iort_idmap_compare(gconstpointer a,
>>> gconstpointer b)
>>> static void
>>> build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>> {
>>> - int i, nb_nodes, rc_mapping_count;
>>> - AcpiIortIdMapping *idmap;
>>> - AcpiIortItsGroup *its;
>>> - AcpiIortSmmu3 *smmu;
>>> - AcpiIortRC *rc;
>>> + int i, rc_mapping_count;
>>> const uint32_t iort_node_offset = 48;
>> can be replaced by IORT_NODE_OFFSET now
>>> size_t node_size, smmu_offset = 0;
>>> + AcpiIortIdMapping *idmap;
>>> GArray *smmu_idmaps = g_array_new(false, true,
>>> sizeof(AcpiIortIdMapping));
>>> GArray *its_idmaps = g_array_new(false, true,
>>> sizeof(AcpiIortIdMapping));
>>>
>>> AcpiTable table = { .sig = "IORT", .rev = 0, .oem_id = vms->oem_id,
>>> .oem_table_id = vms->oem_table_id };
>>> + /* Table 2 The IORT */
>>> + acpi_table_begin(&table, table_data);
>> nit: you could have done this move in previous patch.
>>>
>>> if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>>>
>>> @@ -325,106 +346,100 @@ build_iort(GArray *table_data, BIOSLinker *linker,
>>> VirtMachineState *vms)
>>> g_array_append_val(its_idmaps, next_range);
>>> }
>>>
>>> - nb_nodes = 3; /* RC, ITS, SMMUv3 */
>>> rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
>>> + /* Number of IORT Nodes */
>>> + build_append_int_noprefix(table_data, 3 /* RC, ITS, SMMUv3 */, 4);
>>> } else {
>>> - nb_nodes = 2; /* RC, ITS */
>>> rc_mapping_count = 1;
>>> + /* Number of IORT Nodes */
>>> + build_append_int_noprefix(table_data, 2 /* RC, ITS */, 4);
>> I think I would prefer to keep the nb_nodes variable and do the
>> corresponding build_append_int_noprefix only once. There may be
>> additional nodes added with increased complexity and potential extra
>> duplication.
>>> }
>>>
>>> - /* Table 2 The IORT */
>>> - acpi_table_begin(&table, table_data);
>>> - /* Number of IORT Nodes */
>>> - build_append_int_noprefix(table_data, nb_nodes, 4);
>>> /* Offset to Array of IORT Nodes */
>>> - build_append_int_noprefix(table_data, iort_node_offset, 4);
>>> + build_append_int_noprefix(table_data, IORT_NODE_OFFSET, 4);
>>> build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>>>
>>> - /* ITS group node */
>>> - node_size = sizeof(*its) + sizeof(uint32_t);
>>> - its = acpi_data_push(table_data, node_size);
>>> -
>>> - its->type = ACPI_IORT_NODE_ITS_GROUP;
>>> - its->length = cpu_to_le16(node_size);
>>> - its->its_count = cpu_to_le32(1);
>>> - its->identifiers[0] = 0; /* MADT translation_id */
>>> + /* 3.1.1.3 ITS group node */
>>> + build_append_int_noprefix(table_data, 0 /* ITS Group */, 1); /* Type */
>>> + node_size = 20 /* fixed header size */ + 4 /* 1 GIC ITS Identifier */;
>>> + build_append_int_noprefix(table_data, node_size, 2); /* Length */
>>> + build_append_int_noprefix(table_data, 0, 1); /* Revision */
>>> + build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>>> + build_append_int_noprefix(table_data, 0, 4); /* Number of ID mappings
>>> */
>>> + build_append_int_noprefix(table_data, 0, 4); /* Reference to ID Array
>>> */
>>> + build_append_int_noprefix(table_data, 1, 4); /* Number of ITSs */
>>> + /* GIC ITS Identifier Array */
>>> + build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
>>>
>>> if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>>> int irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
>>>
>>> - /* SMMUv3 node */
>>> - smmu_offset = iort_node_offset + node_size;
>>> - node_size = sizeof(*smmu) + sizeof(*idmap);
>>> - smmu = acpi_data_push(table_data, node_size);
>>> -
>>> - smmu->type = ACPI_IORT_NODE_SMMU_V3;
>>> - smmu->length = cpu_to_le16(node_size);
>>> - smmu->mapping_count = cpu_to_le32(1);
>>> - smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
>>> - smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
>>> - smmu->flags = cpu_to_le32(ACPI_IORT_SMMU_V3_COHACC_OVERRIDE);
>>> - smmu->event_gsiv = cpu_to_le32(irq);
>>> - smmu->pri_gsiv = cpu_to_le32(irq + 1);
>>> - smmu->sync_gsiv = cpu_to_le32(irq + 2);
>>> - smmu->gerr_gsiv = cpu_to_le32(irq + 3);
>>> -
>>> - /* Identity RID mapping covering the whole input RID range */
>>> - idmap = &smmu->id_mapping_array[0];
>>> - idmap->input_base = 0;
>>> - idmap->id_count = cpu_to_le32(0xFFFF);
>>> - idmap->output_base = 0;
>>> - /* output IORT node is the ITS group node (the first node) */
>>> - idmap->output_reference = cpu_to_le32(iort_node_offset);
>>> + smmu_offset = table_data->len - table.table_offset;
>>> + /* 3.1.1.2 SMMUv3 */
>>> + build_append_int_noprefix(table_data, 4 /* SMMUv3 */, 1); /* Type
>>> */
>>> + node_size = SMMU_V3_ENTRY_SIZE + ID_MAPPING_ENTRY_SIZE;
>>> + build_append_int_noprefix(table_data, node_size, 2); /* Length */
>>> + build_append_int_noprefix(table_data, 0, 1); /* Revision */
>>> + build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>>> + build_append_int_noprefix(table_data, 1, 4); /* Number of ID
>>> mappings */
>>> + /* Reference to ID Array */
>>> + build_append_int_noprefix(table_data, SMMU_V3_ENTRY_SIZE, 4);
>>> + /* Base address */
>>> + build_append_int_noprefix(table_data, vms->memmap[VIRT_SMMU].base,
>>> 8);
>>> + /* Flags */
>>> + build_append_int_noprefix(table_data, 1 /* COHACC OverrideNote */,
>>> 4);
>>> + build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>>> + build_append_int_noprefix(table_data, 0, 8); /* VATOS address */
>>> + /* Model */
>>> + build_append_int_noprefix(table_data, 0 /* Generic SMMU-v3 */, 4);
>>> + build_append_int_noprefix(table_data, irq, 4); /* Event */
>>> + build_append_int_noprefix(table_data, irq + 1, 4); /* PRI */
>>> + build_append_int_noprefix(table_data, irq + 3, 4); /* GERR */
>>> + build_append_int_noprefix(table_data, irq + 2, 4); /* Sync */
>> Please keep that comment. I think it helps
>> /* output IORT node is the ITS group node (the first node) */
>>> +
>>> + build_iort_id_mapping(table_data, 0, 0xFFFF, IORT_NODE_OFFSET);
>>> }
>>>
>>> - /* Root Complex Node */
>>> - node_size = sizeof(*rc) + sizeof(*idmap) * rc_mapping_count;
>>> - rc = acpi_data_push(table_data, node_size);
>>> -
>>> - rc->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
>>> - rc->length = cpu_to_le16(node_size);
>>> - rc->mapping_count = cpu_to_le32(rc_mapping_count);
>>> - rc->mapping_offset = cpu_to_le32(sizeof(*rc));
>>> -
>>> + /* Table 16 Root Complex Node */
>>> + build_append_int_noprefix(table_data, 2 /* Root complex */, 1); /*
>>> Type */
>>> + node_size = ROOT_COMPLEX_ENTRY_SIZE +
>>> + ID_MAPPING_ENTRY_SIZE * rc_mapping_count;
>>> + build_append_int_noprefix(table_data, node_size, 2); /* Length */
>>> + build_append_int_noprefix(table_data, 0, 1); /* Revision */
>>> + build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>>> + /* Number of ID mappings */
>>> + build_append_int_noprefix(table_data, rc_mapping_count, 4);
>>> + /* Reference to ID Array */
>>> + build_append_int_noprefix(table_data, ROOT_COMPLEX_ENTRY_SIZE, 4);
>>> /* fully coherent device */
>> this comment should become /* Memory access properties */ for homogenity
>>> - rc->memory_properties.cache_coherency = cpu_to_le32(1);
>>> - rc->memory_properties.memory_flags = 0x3; /* CCA = CPM = DCAS = 1 */
>>> - rc->pci_segment_number = 0; /* MCFG pci_segment */
>>> -
>>> + build_append_int_noprefix(table_data,
>>> + 1 | /* CCA: Cache Coherent Attribute, The device is fully coherent
>>> */
>>> + (3ULL << 7 * 8) /* MAF: Memory Access Flags, CCA = CPM = DCAS = 1
>>> */,
>>> + 8);
>> I think we would gain in readability if we were to split into multiple
>>
>> build_append_int_noprefix according to Table 13 fields
> in spec I've got it's "Table 14: Memory access properties"
> Do you have any preference which spec/rev/ we should use for IORT?
I looked at Document number: ARM DEN 0049B
as said in the comment Document number: ARM DEN 0049B, October 2015
Found
https://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=&ved=2ahUKEwjD3q_E0JLzAhVtgP0HHRuxDtYQFnoECAIQAQ&url=https%3A%2F%2Fdocumentation-service.arm.com%2Fstatic%2F5f8eb168f86e16515cdbe414%3Ftoken%3D&usg=AOvVaw1tQQk9lJwojw_hM6ZJUbat
Table 13 Memory access properties Thanks Eric
>
>>> + build_append_int_noprefix(table_data, 0, 4); /* ATS Attribute */
>>> + /* MCFG pci_segment */
>>> + build_append_int_noprefix(table_data, 0, 4); /* PCI Segment number */
>>> +
>>> + /* Output Reference */
>>> if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>>> AcpiIortIdMapping *range;
>>>
>>> /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
>>> for (i = 0; i < smmu_idmaps->len; i++) {
>>> - idmap = &rc->id_mapping_array[i];
>>> range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
>>> -
>>> - idmap->input_base = cpu_to_le32(range->input_base);
>>> - idmap->id_count = cpu_to_le32(range->id_count);
>>> - idmap->output_base = cpu_to_le32(range->input_base);
>>> - /* output IORT node is the smmuv3 node */
>>> - idmap->output_reference = cpu_to_le32(smmu_offset);
>> please keep the original comment
>>> + build_iort_id_mapping(table_data, range->input_base,
>>> + range->id_count, smmu_offset);
>>> }
>>>
>>> /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
>>> for (i = 0; i < its_idmaps->len; i++) {
>>> - idmap = &rc->id_mapping_array[smmu_idmaps->len + i];
>>> range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
>>> -
>>> - idmap->input_base = cpu_to_le32(range->input_base);
>>> - idmap->id_count = cpu_to_le32(range->id_count);
>>> - idmap->output_base = cpu_to_le32(range->input_base);
>>> - /* output IORT node is the ITS group node (the first node) */
>>> - idmap->output_reference = cpu_to_le32(iort_node_offset);
>> same, at least it helps me ;-)
>>> + build_iort_id_mapping(table_data, range->input_base,
>>> + range->id_count, iort_node_offset);
>>> }
>>> } else {
>>> - /* Identity RID mapping covering the whole input RID range */
>>> - idmap = &rc->id_mapping_array[0];
>>> - idmap->input_base = cpu_to_le32(0);
>>> - idmap->id_count = cpu_to_le32(0xFFFF);
>>> - idmap->output_base = cpu_to_le32(0);
>>> /* output IORT node is the ITS group node (the first node) */
>>> - idmap->output_reference = cpu_to_le32(iort_node_offset);
>>> + build_iort_id_mapping(table_data, 0, 0xFFFF, IORT_NODE_OFFSET);
>>> }
>>>
>>> acpi_table_end(linker, &table);
>> Thanks
>>
>> Eric
>>
- [PATCH v3 25/35] acpi: arm/virt: madt: use build_append_int_noprefix() API to compose MADT table, (continued)
- [PATCH v3 25/35] acpi: arm/virt: madt: use build_append_int_noprefix() API to compose MADT table, Igor Mammedov, 2021/09/07
- [PATCH v3 26/35] acpi: build_dsdt_microvm: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 27/35] acpi: arm: virt: build_dsdt: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 28/35] acpi: arm: virt: build_iort: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 29/35] acpi: arm/virt: convert build_iort() to endian agnostic build_append_FOO() API, Igor Mammedov, 2021/09/07
[PATCH v3 30/35] acpi: arm/virt: build_spcr: fix invalid cast, Igor Mammedov, 2021/09/07
[PATCH v3 31/35] acpi: arm/virt: build_spcr: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
[PATCH v3 32/35] acpi: arm/virt: build_gtdt: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
[PATCH v3 33/35] acpi: build_facs: use build_append_int_noprefix() API to compose table, Igor Mammedov, 2021/09/07
[PATCH v3 35/35] acpi: AcpiGenericAddress no longer used to map/access fields of MMIO, drop packed attribute, Igor Mammedov, 2021/09/07
[PATCH v3 34/35] acpi: remove no longer used build_header(), Igor Mammedov, 2021/09/07