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.malla...@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.malla...@broadcom.com>
>> Signed-off-by: Eric Auger <eric.au...@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
>
