Re: [Qemu-devel] [PATCH v2 9/9] [optional] arm: smmu-v3: ACPI IORT initial support

[Auger Eric]

Hi Prem,

On 22/08/2016 18:17, Prem Mallappa wrote:
> Added ACPI IORT tables, was needed for internal project purpose, but
> posting here for anyone looking for testing ACPI on ARM platforms.
> (P.S: Linux side IORT patches are WIP)
I am also interested in IORT ITS group and currently prototyping
something, hence my few comments/questions.
> 
> Signed-off-by: Prem Mallappa <address@hidden>
> ---
>  hw/arm/virt-acpi-build.c    | 43 +++++++++++++++++++++++
>  include/hw/acpi/acpi-defs.h | 84 
> +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1fa0581..d5fb69e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -382,6 +382,45 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
> unsigned rsdt_tbl_offset)
>      return rsdp_table;
>  }
>  
> +/*
> + * TODO: Simple IORT for now, will add ID mappings as we go
> + * basic idea is to instantiate SMMU from ACPI
> + */
> +static void
> +build_iort(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info)
> +{
> +    int iort_start = table_data->len;
> +    AcpiIortTable *iort;
> +    AcpiIortNode *iort_node;
> +    AcpiIortSmmu3 *smmu;
> +    AcpiIortRC *rc;
> +    const MemMapEntry *memmap = guest_info->memmap;
> +
> +    iort = acpi_data_push(table_data, sizeof(*iort));
> +
> +    iort->length = sizeof(*iort);
Isn't is supposed to be the length of the whole IORT (including the node
cumulated sizes?)
> +    iort->node_offset = table_data->len - iort_start;
> +    iort->num_nodes++;
> +
> +    smmu = acpi_data_push(table_data, sizeof(*smmu));
> +    iort_node = &smmu->iort_node;
> +    iort_node->type = 0x04;          /* SMMUv3 */
To match existing code (include/hw/acpi/acpi-defs.h), maybe enum values
can be created (ACPI_IORT_NODE_SMMU_V3). This also matches kernel enum.

More generally Shannon advised to use the same field names as the ones
used in the kernel header: acpi_iort_node_type in include/acpi/actbl2.h
> +    iort_node->length = sizeof(*smmu);
> +    smmu->base_addr = cpu_to_le64(memmap[VIRT_SMMU].base);
> +
> +    iort->num_nodes++;
> +
> +    rc = acpi_data_push(table_data, sizeof(*rc));
> +    iort_node = &rc->iort_node;
> +    iort_node->type = 0x02;          /* RC */
> +    iort_node->length = sizeof(*rc);
I think the mem_access_prop field should be set to 1 now the host
controller is assumed to be cache coherent.
> +    rc->ats_attr = 1;
no ATS support instead?
> +    rc->pci_seg_num = 0;
ID mappings are mandated for me to support MSIs with ITS.
> +
> +    build_header(linker, table_data, (void *)(table_data->data + iort_start),
> +                 "IORT", table_data->len - iort_start, 0, NULL, NULL);
> +}
> +
>  static void
>  build_spcr(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info)
>  {
> @@ -667,6 +706,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info, 
> AcpiBuildTables *tables)
>       * MADT
>       * MCFG
>       * DSDT
> +     * IORT = ACPI 6.0
>       */
>  
>      /* DSDT is pointed to by FADT */
> @@ -694,6 +734,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, 
> AcpiBuildTables *tables)
>          build_srat(tables_blob, tables->linker, guest_info);
>      }
>  
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_iort(tables_blob, tables->linker, guest_info);
> +
>      /* RSDT is pointed to by RSDP */
>      rsdt = tables_blob->len;
>      build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 850a962..d60f390 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -259,6 +259,90 @@ typedef struct AcpiFacsDescriptorRev1 
> AcpiFacsDescriptorRev1;
>   */
>  
>  /*
> + * IORT Table
> + */
> +struct AcpiIortTable
> +{
> +    ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
> +    uint32_t num_nodes;
> +    uint32_t node_offset;
> +    uint32_t reserved;
> +} QEMU_PACKED;
> +typedef struct AcpiIortTable AcpiIortTable;
> +
> +struct AcpiIortIdMapping
> +{
> +    uint32_t input_base;
> +    uint32_t num_ids;
> +    uint32_t output_base;
> +    uint32_t output_ref;
> +    uint32_t flags;
> +} QEMU_PACKED;
> +typedef struct AcpiIortIdMapping AcpiIortIdMapping;
Shannon told me we should match the kernel datatypes & fields

for instance in include/acpi/actbl2.h we have:

struct acpi_iort_id_mapping {
        u32 input_base;         /* Lowest value in input range */
        u32 id_count;           /* Number of IDs */
        u32 output_base;        /* Lowest value in output range */
        u32 output_reference;   /* A reference to the output node */
        u32 flags;
};

This also holds for other struct definitions.

Thanks!

Eric

> +
> +struct AcpiIortNode
> +{
> +    uint8_t  type;
> +    uint16_t length;
> +    uint8_t  revision;
> +    uint32_t reserved1;
> +    uint32_t num_id_maps;
> +    uint32_t id_array_offset;
> +} QEMU_PACKED;
> +typedef struct AcpiIortNode AcpiIortNode;
> +
> +struct AcpiIortSmmu2
> +{
> +    AcpiIortNode iort_node;
> +    uint64_t base_addr;
> +    uint64_t span;
> +    uint32_t model;
> +    uint32_t flags;
> +    uint32_t gbl_intr_array_off;
> +    uint32_t ctx_intr_cnt;
> +    uint32_t ctx_intr_array_off;
> +    uint32_t pmr_intr_cnt;
> +    uint32_t pmr_intr_array_off;
> +
> +    // Global interrupt array
> +    uint32_t gintr;
> +    uint32_t gintr_flags;
> +    uint32_t gcfgintr;
> +    uint32_t gcfgintr_flags;
> +
> +    //AcpiIortIdMapping id_mapping_array[0];
> +} QEMU_PACKED;
> +typedef struct AcpiIortSmmu2 AcpiIortSmmu2;
> +
> +struct AcpiIortSmmu3
> +{
> +    AcpiIortNode iort_node;
> +    uint64_t base_addr;
> +    uint32_t flags;
> +    uint32_t reserved2;
> +    uint64_t vatos_addr;
> +    uint32_t model;
> +    uint32_t event_irq;
> +    uint32_t pri_irq;
> +    uint32_t gerr_irq;
> +    uint32_t sync_irq;
> +
> +    //AcpiIortIdMapping id_mapping_array[0];
> +} QEMU_PACKED;
> +typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
> +
> +struct AcpiIortRC
> +{
> +    AcpiIortNode iort_node;
> +    uint64_t mem_access_prop;
> +    uint32_t ats_attr;
> +    uint32_t pci_seg_num;
> +
> +    AcpiIortIdMapping id_mapping_array[0];
> +} QEMU_PACKED;
> +typedef struct AcpiIortRC AcpiIortRC;
> +
> +/*
>   * MADT values and structures
>   */
>  
>