Re: [PATCH v3 29/35] acpi: arm/virt: convert build_iort() to endian agnostic build_append_FOO() API

[Igor Mammedov <imammedo@redhat.com>]

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?

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