| Message ID | 1476356143-10577-3-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + > + 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
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 >
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; + } + + iort = acpi_data_push(table_data, sizeof(*iort)); + + iort->length = sizeof(*iort); + iort->node_offset = table_data->len - iort_start; + + /* ITS group node featuring a single ITS identifier */ + iort->node_count++; + 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; + iort_node->length = node_size; + iort->length += iort_node->length; + + iort_node->mapping_count = 0; /* ITS groups do not have IDs */ + iort_node->mapping_offset = 0; /* no ID array */ + its->its_count = 1; /* single ITS identifier */ + its->identifiers[0] = 0; /* ID = 0 as described in the MADT */ + + /* 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; + rc->memory_properties.hints = 0; + rc->memory_properties.memory_flags = 0; + rc->ats_attribute = 0; /* does not support ATS */ + rc->pci_segment_number = 0; /* MCFG _SEG */ + + /* 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; + + 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 */ /* 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); + /* RSDT is pointed to by RSDP */ rsdt = tables_blob->len; build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);