Re: [Qemu-devel] [PATCH 2/2] ARM: Virt: ACPI: Build an IORT table with RC and ITS nodes

[Andrew Jones]

On Thu, Oct 13, 2016 at 12:55:43PM +0200, Eric Auger wrote:
> From: Prem Mallappa <address@hidden>
> 
> This patch builds an IORT table that features a root complex node and
> an ITS node. This complements the ITS description in the ACPI MADT
> table and allows vhost-net on ACPI guest.
> 
> Signed-off-by: Prem Mallappa <address@hidden>
> Signed-off-by: Eric Auger <address@hidden>
> ---
>  hw/arm/virt-acpi-build.c | 71 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index fa0655a..373630a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -384,6 +384,73 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
> unsigned rsdt_tbl_offset)
>  }
>  
>  static void
> +build_iort(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info)
> +{
> +    int iort_start = table_data->len;
> +    AcpiIortTable *iort;
> +    AcpiIortNode *iort_node;
> +    AcpiIortItsGroup *its;
> +    AcpiIortRC *rc;
> +    AcpiIortIdMapping *idmap;
> +    size_t node_size;
> +
> +    /* at the moment if there is no its, no need to build the IORT */
> +    if (!its_class_name() || guest_info->no_its) {
> +        return;
> +    }

This should wrap the calls to acpi_add_table and build_iort down in
virt_acpi_build, like what is done for the SRAT.

> +
> +    iort = acpi_data_push(table_data, sizeof(*iort));
> +
> +    iort->length = sizeof(*iort);

Missing cpu_to_le* here and many places below.

> +    iort->node_offset = table_data->len - iort_start;
> +
> +    /* ITS group node featuring a single ITS identifier */
> +    iort->node_count++;

Let's just set node_count to 2 at the beginning, under a
comment describing what's being built; an IORT with two
nodes, one ITS group and one RC.

> +    node_size =  sizeof(*its) + sizeof(uint32_t);
> +    its = acpi_data_push(table_data, node_size);
> +
> +    iort_node = &its->iort_node;
> +    iort_node->type = ACPI_IORT_NODE_ITS_GROUP;

I think
 its->type = 0; /* 0: ITS Group */ 
would be fine here.

> +    iort_node->length = node_size;

As mentioned in the previous patch this separate struct for the
node header isn't necessary, and it's actually making this code
confusing.

> +    iort->length += iort_node->length;
> +
> +    iort_node->mapping_count = 0;  /* ITS groups do not have IDs */
> +    iort_node->mapping_offset = 0; /* no ID array */

These assignments aren't necessary and just reproduce the documentation
in the spec.

> +    its->its_count = 1;            /* single ITS identifier */

The comment here just describes the code, I'd drop it.

> +    its->identifiers[0] = 0;       /* ID = 0 as described in the MADT */

Might be nice to point precisely at the madt 'translation_id'
in the comment.

> +
> +    /* Root Complex Node with a single ID mapping*/
> +    iort->node_count++;
> +    node_size = sizeof(*rc) + sizeof(*idmap);
> +    rc = acpi_data_push(table_data, node_size);
> +
> +    iort_node = &rc->iort_node;
> +    iort_node->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
> +    iort_node->length = node_size;
> +    iort->length += iort_node->length;
> +
> +    iort_node->mapping_count = 1;
> +    iort_node->mapping_offset = sizeof(*rc);
> +
> +    rc->memory_properties.cache_coherency = ACPI_IORT_NODE_COHERENT;

cache_coherency = cpu_to_le32(1); /* device is fully coherent */

> +    rc->memory_properties.hints = 0;

No need to explicitly zero hints.

> +    rc->memory_properties.memory_flags = 0;

I have a feeling we'll be revisiting these flags some day, resolving
cache coherency issues... Hmm, actually it appears per Table 15 of
the spec that this configuration is illegal. We can't have CCA 1 and
CPM 0. When this gets sorted out I think we need a comment explaining
how whatever the final selection is was selected.

> +    rc->ats_attribute = 0;      /* does not support ATS */

Can probably just drop the above assignment.

> +    rc->pci_segment_number = 0; /* MCFG _SEG */

Maybe comment pointing precisely to mcfg 'pci_segment'

> +
> +    /* fill array of ID mappings */
> +    idmap = &rc->id_mapping_array[0];
> +    idmap->input_base = 0;
> +    idmap->id_count = 0xFFFF;
> +    idmap->output_base = 0;
> +    idmap->output_reference = iort->node_offset;
> +    idmap->flags = 0;

Comments for all the above would be good. Why base of zero? Why count of
0xffff? "output reference points to the offset of the ITS group node"
Why 'single mapping' flag is zero?

> +
> +    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)
>  {
>      AcpiSerialPortConsoleRedirection *spcr;
> @@ -676,6 +743,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info, 
> AcpiBuildTables *tables)
>       * MADT
>       * MCFG
>       * DSDT
> +     * IORT = ACPI 6.0
>       */

I think the whole comment block above should just be removed. I'm not sure
what it adds besides a burden of maintenance.

>  
>      /* DSDT is pointed to by FADT */
> @@ -703,6 +771,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);

As mentioned above, this should be where we have the !its_class_name()
guard.

> +
>      /* RSDT is pointed to by RSDP */
>      rsdt = tables_blob->len;
>      build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> -- 
> 2.5.5
> 
>

Thanks,
drew