Re: [PATCH RFC RESEND v2 4/6] hw/arm/virt-acpi-build: Add explicit idmap info in IORT table

[Xingang Wang]

Hi Eric,

On 2021/4/13 17:03, Auger Eric wrote:
> Hi Xingang,
>
> On 3/25/21 8:22 AM, Wang Xingang wrote:
> > From: Xingang Wang <wangxingang5@huawei.com>
> >
> > The idmap of smmuv3 and root complex covers the whole RID space for now,
> > this patch add explicit idmap info according to root bus number range.
> > This add smmuv3 idmap for certain bus which has enabled the iommu property.
> >
> > Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> > Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> > ---
> >   hw/arm/virt-acpi-build.c | 103 ++++++++++++++++++++++++++++++---------
> >   1 file changed, 81 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index f5a2b2d4cb..5491036c86 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -44,6 +44,7 @@
> >   #include "hw/acpi/tpm.h"
> >   #include "hw/pci/pcie_host.h"
> >   #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bus.h"
> >   #include "hw/pci-host/gpex.h"
> >   #include "hw/arm/virt.h"
> >   #include "hw/mem/nvdimm.h"
> > @@ -237,6 +238,41 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState 
> > *vms)
> >       aml_append(scope, dev);
> >   }
> >   
> > +typedef
> > +struct AcpiIortMapping {
> > +    AcpiIortIdMapping idmap;
> > +    bool iommu;
> > +} AcpiIortMapping;
> > +
> > +/* For all PCI host bridges, walk and insert DMAR scope */
> this comment should rather be in the caller
> also DMAR is not the ARM vocable.
>
> I would add the comment for this function:
> build the ID mapping for aa given PCI host bridge

Thanks, I will fix the comment.

> > +static int
> > +iort_host_bridges(Object *obj, void *opaque)
> > +{
> > +    GArray *map_blob = opaque;
> > +    AcpiIortMapping map;
> > +    AcpiIortIdMapping *idmap = &map.idmap;
> > +    int bus_num, max_bus;
> > +
> > +    if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> > +        PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
> > +
> > +        if (bus) {
> > +            bus_num = pci_bus_num(bus);
> > +            max_bus = pci_root_bus_max_bus(bus);
> > +
> > +            idmap->input_base = cpu_to_le32(bus_num << 8);
> > +            idmap->id_count = cpu_to_le32((max_bus - bus_num + 1) << 8);
> > +            idmap->output_base = cpu_to_le32(bus_num << 8);
> > +            idmap->flags = cpu_to_le32(0);
> > +
> > +            map.iommu = pci_root_bus_has_iommu(bus);
> if iommu is not set, we don't need to populate the idmap and we may even
> directly continue, ie. not add the element the map_bap_blob, no?
Thanks, if we leave the whole range when iommu is not set, this indeed 
should be dropped in map_blob.
> > +            g_array_append_val(map_blob, map);
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   static void
> >   build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >   {
> > @@ -247,6 +283,21 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> > VirtMachineState *vms)
> >       AcpiIortSmmu3 *smmu;
> >       size_t node_size, iort_node_offset, iort_length, smmu_offset = 0;
> >       AcpiIortRC *rc;
> > +    int smmu_mapping_count;
> > +    GArray *map_blob = g_array_new(false, true, sizeof(AcpiIortMapping));
> > +    AcpiIortMapping *map;
> > +
> > +    /* pci_for_each_bus(vms->bus, insert_map, map_blob); */
> comment to be removed
Done.
> > +    object_child_foreach_recursive(object_get_root(),
> > +                                   iort_host_bridges, map_blob);
> > +
> > +    smmu_mapping_count = 0;
> > +    for (int i = 0; i < map_blob->len; i++) {
> > +        map = &g_array_index(map_blob, AcpiIortMapping, i);
> > +        if (map->iommu) {
> > +            smmu_mapping_count++;
> > +        }
> > +    }
> >   
> >       iort = acpi_data_push(table_data, sizeof(*iort));
> >   
> > @@ -280,13 +331,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >   
> >           /* SMMUv3 node */
> >           smmu_offset = iort_node_offset + node_size;
> > -        node_size = sizeof(*smmu) + sizeof(*idmap);
> > +        node_size = sizeof(*smmu) + sizeof(*idmap) * smmu_mapping_count;
> >           iort_length += node_size;
> >           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_count = cpu_to_le32(smmu_mapping_count);
> >           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);
> > @@ -295,23 +346,28 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> > VirtMachineState *vms)
> >           smmu->gerr_gsiv = cpu_to_le32(irq + 2);
> >           smmu->sync_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);
> > +        for (int i = 0, j = 0; i < map_blob->len; i++) {
> > +            map = &g_array_index(map_blob, AcpiIortMapping, i);
> > +
> > +            if (!map->iommu) {
> > +                continue;
> > +            }
> > +
> > +            idmap = &smmu->id_mapping_array[j++];
> > +            *idmap = map->idmap;
> > +            /* output IORT node is the ITS group node (the first node) */
> > +            idmap->output_reference = cpu_to_le32(iort_node_offset);
> > +        }
> >       }
> >   
> >       /* Root Complex Node */
> > -    node_size = sizeof(*rc) + sizeof(*idmap);
> > +    node_size = sizeof(*rc) + sizeof(*idmap) * map_blob->len;
> >       iort_length += node_size;
> >       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(1);
> > +    rc->mapping_count = cpu_to_le32(map_blob->len);
> >       rc->mapping_offset = cpu_to_le32(sizeof(*rc));
> >   
> >       /* fully coherent device */
> > @@ -319,20 +375,23 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> > VirtMachineState *vms)
> >       rc->memory_properties.memory_flags = 0x3; /* CCA = CPM = DCAS = 1 */
> >       rc->pci_segment_number = 0; /* MCFG pci_segment */
> >   
> > -    /* Identity RID mapping covering the whole input RID range */
> > -    idmap = &rc->id_mapping_array[0];
> > -    idmap->input_base = 0;
> > -    idmap->id_count = cpu_to_le32(0xFFFF);
> > -    idmap->output_base = 0;
> > +    for (int i = 0; i < map_blob->len; i++) {
> > +        map = &g_array_index(map_blob, AcpiIortMapping, i);
> > +        idmap = &rc->id_mapping_array[i];
> >   
> > -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> > -        /* output IORT node is the smmuv3 node */
> > -        idmap->output_reference = cpu_to_le32(smmu_offset);
> > -    } else {
> > -        /* output IORT node is the ITS group node (the first node) */
> > -        idmap->output_reference = cpu_to_le32(iort_node_offset);
> > +        *idmap = map->idmap;
> We we descibe the link between SMMU SID and ITS deviceids, shouldn't we
> leave the whole range? Even if an RID is not translated, it may use MSIs?
Thanks, I did not take into account of this situation, I will fix this.
> > +
> > +        if (vms->iommu == VIRT_IOMMU_SMMUV3 && map->iommu) {
> > +            /* output IORT node is the smmuv3 node */
> > +            idmap->output_reference = cpu_to_le32(smmu_offset);
> > +        } else {
> > +            /* output IORT node is the ITS group node (the first node) */
> > +            idmap->output_reference = cpu_to_le32(iort_node_offset);
> is that case we have a direct mapping between RC and ITS (no
> instantiated SMMU). Shouldn't the whole RID range being used in that case?
>
> Thanks
>
> Eric

Thanks, I will change the idmap strategy to cover the whole RID range.
> > +        }
> >       }
> >   
> > +    g_array_free(map_blob, true);
> > +
> >       /*
> >        * Update the pointer address in case table_data->data moves during above
> >        * acpi_data_push operations.
>
> .

Xingang

.