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

[Auger Eric <eric.auger@redhat.com>]

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 <prem.mallappa@broadcom.com>
>>
>> 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 <prem.mallappa@broadcom.com>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  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 = &its->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 = &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 */
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 = &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?
single mapping flag == 0, that's the spec ;-) I guess we don't want a
single RID output per input RID. I will add a comment saying that this
corresponds to an identity mapping covering the whole input RID range (16b)
> 
>> +
>> +    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.
OK
> 
>>  
>>      /* 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.
OK

Thanks for the detailed review.

Eric
> 
>> +
>>      /* 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 
>