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

[Auger Eric]

Drew,

On 13/10/2016 17:11, Andrew Jones wrote:
> On Thu, Oct 13, 2016 at 12:55:43PM +0200, Eric Auger wrote:
>> From: Prem Mallappa 
>>
>> 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 
>> Signed-off-by: Eric Auger 
>> ---
>>  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.
OK
> 
>> +
>> +iort = acpi_data_push(table_data, sizeof(*iort));
>> +
>> +iort->length = sizeof(*iort);
> 
> Missing cpu_to_le* here and many places below.
hum my bad
> 
>> +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.
OK
> 
>> +node_size =  sizeof(*its) + sizeof(uint32_t);
>> +its = acpi_data_push(table_data, node_size);
>> +
>> +iort_node = >iort_node;
>> +iort_node->type = ACPI_IORT_NODE_ITS_GROUP;
> 
> I think
>  its->type = 0; /* 0: ITS Group */ 
> would be fine here.
Well I just keep that define. Looks clearer to me.
> 
>> +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.
Indeed I acknowledge looking at the code now. thanks for the hint.
> 
>> +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.
OK
> 
>> +its->its_count = 1;/* single ITS identifier */
> 
> The comment here just describes the code, I'd drop it.
OK
> 
>> +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.
OK
> 
>> +
>> +/* 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 = >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 */
OK
> 
>> +rc->memory_properties.hints = 0;
> 
> No need to explicitly zero hints.
OK
> 
>> +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.
You're right. Did not pay too much attention to that since the
functional behavior looked ok. Looks like CCA = CPM = DACS = 1 is the
preferred solution then since DACS == 0 would rely on an smmu override.
> 
>> +rc->ats_attribute = 0;  /* does not support ATS */
> 
> Can probably just drop the above assignment.
OK
> 
>> +rc->pci_segment_number = 0; /* MCFG _SEG */
> 
> Maybe comment pointing precisely to mcfg 'pci_segment'
OK
> 
>> +
>> +/* fill array of ID mappings */
>> +idmap = >id_mapping_array[0];
>> +idmap->input_base = 0;
>> +idmap->id_count = 0x;
>> +idmap->output_base = 0;
>> +idmap->output_reference = iort->node_offset;
>> +idmap->flags = 0;
> 
> Comments for all the above would be good. Why 

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 
> 
> 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 
> Signed-off-by: Eric Auger 
> ---
>  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 = >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 = >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 = >id_mapping_array[0];
> +idmap->input_base = 0;
> +idmap->id_count = 0x;
> +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
0x? "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