Re: [Qemu-devel] [RFC v6 18/22] hw/arm/virt-acpi-build: Add virtio-iommu node in IORT table

[Auger Eric]

Hi Drew,
On 13/02/18 13:24, Andrew Jones wrote:
> On Mon, Feb 12, 2018 at 06:58:20PM +, Eric Auger wrote:
>> This patch builds the virtio-iommu node in the ACPI IORT table.
>> The dt node creation function fills the information used by
>> the IORT table generation function (base address, base irq,
>> type of the smmu).
>>
>> The RID space of the root complex, which spans 0x0-0x1
>> maps to streamid space 0x0-0x1 in smmuv3, which in turn
>> maps to deviceid space 0x0-0x1 in the ITS group.
>>
>> Signed-off-by: Eric Auger 
>>
>> ---
>>
>> v5 -> v6:
>> - use type=128
>> - new gsiv and reserved2 fields
>> ---
>>  hw/arm/virt-acpi-build.c| 54 
>> +++--
>>  hw/arm/virt.c   |  5 +
>>  include/hw/acpi/acpi-defs.h | 21 +-
>>  include/hw/arm/virt.h   | 13 +++
>>  4 files changed, 85 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index f7fa795..24efb69 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -42,6 +42,7 @@
>>  #include "hw/acpi/aml-build.h"
>>  #include "hw/pci/pcie_host.h"
>>  #include "hw/pci/pci.h"
>> +#include "hw/virtio/virtio-iommu.h"
>>  #include "hw/arm/virt.h"
>>  #include "sysemu/numa.h"
>>  #include "kvm_arm.h"
>> @@ -393,18 +394,26 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
>> unsigned xsdt_tbl_offset)
>>  }
>>  
>>  static void
>> -build_iort(GArray *table_data, BIOSLinker *linker)
>> +build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>  {
>> -int iort_start = table_data->len;
>> +int nb_nodes, iort_start = table_data->len;
>>  AcpiIortIdMapping *idmap;
>>  AcpiIortItsGroup *its;
>>  AcpiIortTable *iort;
>> -size_t node_size, iort_length;
>> +AcpiIortPVIommu *iommu;
>> +size_t node_size, iort_length, iommu_offset = 0;
>>  AcpiIortRC *rc;
>>  
>>  iort = acpi_data_push(table_data, sizeof(*iort));
>>  
>> +if (vms->iommu_info.type) {
>> +nb_nodes = 3; /* RC, ITS, IOMMU */
>> +} else {
>> +nb_nodes = 2; /* RC, ITS */
>> +}
>> +
>>  iort_length = sizeof(*iort);
>> +iort->node_count = cpu_to_le32(nb_nodes);
>>  iort->node_count = cpu_to_le32(2); /* RC and ITS nodes */
> 
> The above line should be removed, as you're replacing it with the previous
> one. I wonder how the guest was able to parse this table without seeing
> the appropriate node count?
thanks for spotting this.

hum me too. Need to double check the acpi test.
> 
>>  iort->node_offset = cpu_to_le32(sizeof(*iort));
>>  
>> @@ -418,6 +427,31 @@ build_iort(GArray *table_data, BIOSLinker *linker)
>>  its->its_count = cpu_to_le32(1);
>>  its->identifiers[0] = 0; /* MADT translation_id */
>>  
>> +if (vms->iommu_info.type == VIRT_IOMMU_VIRTIO) {
>> +
> 
> extra blank line
> 
>> +/* Para-virtualized IOMMU node */
>> +iommu_offset = cpu_to_le32(iort->node_offset + node_size);
>> +node_size = sizeof(*iommu) + sizeof(*idmap);
>> +iort_length += node_size;
>> +iommu = acpi_data_push(table_data, node_size);
>> +
>> +iommu->type = ACPI_IORT_NODE_PARAVIRT;
>> +iommu->length = cpu_to_le16(node_size);
>> +iommu->base_address = cpu_to_le64(vms->iommu_info.reg.base);
>> +iommu->span = cpu_to_le64(vms->iommu_info.reg.size);
>> +iommu->model = ACPI_IORT_NODE_PV_VIRTIO_IOMMU;
>> +iommu->flags = ACPI_IORT_NODE_PV_CACHE_COHERENT;
> 
> model and flags are both larger than a byte, so they need cpu_to_le*'s
ok
> 
>> +iommu->gsiv = cpu_to_le64(vms->iommu_info.irq_base);
>> +
>> +/* Identity RID mapping covering the whole input RID range */
>> +idmap = >id_mapping_array[0];
>> +idmap->input_base = 0;
>> +idmap->id_count = cpu_to_le32(0x);
>> +idmap->output_base = 0;
>> +/* output IORT node is the ITS group node (the first node) */
>> +idmap->output_reference = cpu_to_le32(iort->node_offset);
>> +}
>> +
>>  /* Root Complex Node */
>>  node_size = sizeof(*rc) + sizeof(*idmap);
>>  iort_length += node_size;
>> @@ -438,10 +472,16 @@ build_iort(GArray *table_data, BIOSLinker *linker)
>>  idmap->input_base = 0;
>>  idmap->id_count = cpu_to_le32(0x);
>>  idmap->output_base = 0;
>> -/* output IORT node is the ITS group node (the first node) */
>> -idmap->output_reference = cpu_to_le32(iort->node_offset);
>>  
>> -iort->length = cpu_to_le32(iort_length);
>> +if (vms->iommu_info.type) {
>> +/* output IORT node is the smmuv3 node */
>> +idmap->output_reference = cpu_to_le32(iommu_offset);
>> +} else {
>> +/* output IORT node is the ITS group node (the first node) */
>> +idmap->output_reference = cpu_to_le32(iort->node_offset);
>> +}
>> +
>> +   iort->length = cpu_to_le32(iort_length);
> 
> This 

Re: [Qemu-devel] [RFC v6 18/22] hw/arm/virt-acpi-build: Add virtio-iommu node in IORT table

[Andrew Jones]

On Mon, Feb 12, 2018 at 06:58:20PM +, Eric Auger wrote:
> This patch builds the virtio-iommu node in the ACPI IORT table.
> The dt node creation function fills the information used by
> the IORT table generation function (base address, base irq,
> type of the smmu).
> 
> The RID space of the root complex, which spans 0x0-0x1
> maps to streamid space 0x0-0x1 in smmuv3, which in turn
> maps to deviceid space 0x0-0x1 in the ITS group.
> 
> Signed-off-by: Eric Auger 
> 
> ---
> 
> v5 -> v6:
> - use type=128
> - new gsiv and reserved2 fields
> ---
>  hw/arm/virt-acpi-build.c| 54 
> +++--
>  hw/arm/virt.c   |  5 +
>  include/hw/acpi/acpi-defs.h | 21 +-
>  include/hw/arm/virt.h   | 13 +++
>  4 files changed, 85 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f7fa795..24efb69 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -42,6 +42,7 @@
>  #include "hw/acpi/aml-build.h"
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
> +#include "hw/virtio/virtio-iommu.h"
>  #include "hw/arm/virt.h"
>  #include "sysemu/numa.h"
>  #include "kvm_arm.h"
> @@ -393,18 +394,26 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
> unsigned xsdt_tbl_offset)
>  }
>  
>  static void
> -build_iort(GArray *table_data, BIOSLinker *linker)
> +build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> -int iort_start = table_data->len;
> +int nb_nodes, iort_start = table_data->len;
>  AcpiIortIdMapping *idmap;
>  AcpiIortItsGroup *its;
>  AcpiIortTable *iort;
> -size_t node_size, iort_length;
> +AcpiIortPVIommu *iommu;
> +size_t node_size, iort_length, iommu_offset = 0;
>  AcpiIortRC *rc;
>  
>  iort = acpi_data_push(table_data, sizeof(*iort));
>  
> +if (vms->iommu_info.type) {
> +nb_nodes = 3; /* RC, ITS, IOMMU */
> +} else {
> +nb_nodes = 2; /* RC, ITS */
> +}
> +
>  iort_length = sizeof(*iort);
> +iort->node_count = cpu_to_le32(nb_nodes);
>  iort->node_count = cpu_to_le32(2); /* RC and ITS nodes */

The above line should be removed, as you're replacing it with the previous
one. I wonder how the guest was able to parse this table without seeing
the appropriate node count?

>  iort->node_offset = cpu_to_le32(sizeof(*iort));
>  
> @@ -418,6 +427,31 @@ build_iort(GArray *table_data, BIOSLinker *linker)
>  its->its_count = cpu_to_le32(1);
>  its->identifiers[0] = 0; /* MADT translation_id */
>  
> +if (vms->iommu_info.type == VIRT_IOMMU_VIRTIO) {
> +

extra blank line

> +/* Para-virtualized IOMMU node */
> +iommu_offset = cpu_to_le32(iort->node_offset + node_size);
> +node_size = sizeof(*iommu) + sizeof(*idmap);
> +iort_length += node_size;
> +iommu = acpi_data_push(table_data, node_size);
> +
> +iommu->type = ACPI_IORT_NODE_PARAVIRT;
> +iommu->length = cpu_to_le16(node_size);
> +iommu->base_address = cpu_to_le64(vms->iommu_info.reg.base);
> +iommu->span = cpu_to_le64(vms->iommu_info.reg.size);
> +iommu->model = ACPI_IORT_NODE_PV_VIRTIO_IOMMU;
> +iommu->flags = ACPI_IORT_NODE_PV_CACHE_COHERENT;

model and flags are both larger than a byte, so they need cpu_to_le*'s

> +iommu->gsiv = cpu_to_le64(vms->iommu_info.irq_base);
> +
> +/* Identity RID mapping covering the whole input RID range */
> +idmap = >id_mapping_array[0];
> +idmap->input_base = 0;
> +idmap->id_count = cpu_to_le32(0x);
> +idmap->output_base = 0;
> +/* output IORT node is the ITS group node (the first node) */
> +idmap->output_reference = cpu_to_le32(iort->node_offset);
> +}
> +
>  /* Root Complex Node */
>  node_size = sizeof(*rc) + sizeof(*idmap);
>  iort_length += node_size;
> @@ -438,10 +472,16 @@ build_iort(GArray *table_data, BIOSLinker *linker)
>  idmap->input_base = 0;
>  idmap->id_count = cpu_to_le32(0x);
>  idmap->output_base = 0;
> -/* output IORT node is the ITS group node (the first node) */
> -idmap->output_reference = cpu_to_le32(iort->node_offset);
>  
> -iort->length = cpu_to_le32(iort_length);
> +if (vms->iommu_info.type) {
> +/* output IORT node is the smmuv3 node */
> +idmap->output_reference = cpu_to_le32(iommu_offset);
> +} else {
> +/* output IORT node is the ITS group node (the first node) */
> +idmap->output_reference = cpu_to_le32(iort->node_offset);
> +}
> +
> +   iort->length = cpu_to_le32(iort_length);

This line appears to have moved down for no reason, but what happened
was the indentation of it got messed up (missing a space)

>  
>  build_header(linker, table_data, (void *)(table_data->data + iort_start),
>   "IORT", table_data->len - 

[Qemu-devel] [RFC v6 18/22] hw/arm/virt-acpi-build: Add virtio-iommu node in IORT table

[Eric Auger]

This patch builds the virtio-iommu node in the ACPI IORT table.
The dt node creation function fills the information used by
the IORT table generation function (base address, base irq,
type of the smmu).

The RID space of the root complex, which spans 0x0-0x1
maps to streamid space 0x0-0x1 in smmuv3, which in turn
maps to deviceid space 0x0-0x1 in the ITS group.

Signed-off-by: Eric Auger 

---

v5 -> v6:
- use type=128
- new gsiv and reserved2 fields
---
 hw/arm/virt-acpi-build.c| 54 +++--
 hw/arm/virt.c   |  5 +
 include/hw/acpi/acpi-defs.h | 21 +-
 include/hw/arm/virt.h   | 13 +++
 4 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f7fa795..24efb69 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -42,6 +42,7 @@
 #include "hw/acpi/aml-build.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
+#include "hw/virtio/virtio-iommu.h"
 #include "hw/arm/virt.h"
 #include "sysemu/numa.h"
 #include "kvm_arm.h"
@@ -393,18 +394,26 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
unsigned xsdt_tbl_offset)
 }
 
 static void
-build_iort(GArray *table_data, BIOSLinker *linker)
+build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
-int iort_start = table_data->len;
+int nb_nodes, iort_start = table_data->len;
 AcpiIortIdMapping *idmap;
 AcpiIortItsGroup *its;
 AcpiIortTable *iort;
-size_t node_size, iort_length;
+AcpiIortPVIommu *iommu;
+size_t node_size, iort_length, iommu_offset = 0;
 AcpiIortRC *rc;
 
 iort = acpi_data_push(table_data, sizeof(*iort));
 
+if (vms->iommu_info.type) {
+nb_nodes = 3; /* RC, ITS, IOMMU */
+} else {
+nb_nodes = 2; /* RC, ITS */
+}
+
 iort_length = sizeof(*iort);
+iort->node_count = cpu_to_le32(nb_nodes);
 iort->node_count = cpu_to_le32(2); /* RC and ITS nodes */
 iort->node_offset = cpu_to_le32(sizeof(*iort));
 
@@ -418,6 +427,31 @@ build_iort(GArray *table_data, BIOSLinker *linker)
 its->its_count = cpu_to_le32(1);
 its->identifiers[0] = 0; /* MADT translation_id */
 
+if (vms->iommu_info.type == VIRT_IOMMU_VIRTIO) {
+
+/* Para-virtualized IOMMU node */
+iommu_offset = cpu_to_le32(iort->node_offset + node_size);
+node_size = sizeof(*iommu) + sizeof(*idmap);
+iort_length += node_size;
+iommu = acpi_data_push(table_data, node_size);
+
+iommu->type = ACPI_IORT_NODE_PARAVIRT;
+iommu->length = cpu_to_le16(node_size);
+iommu->base_address = cpu_to_le64(vms->iommu_info.reg.base);
+iommu->span = cpu_to_le64(vms->iommu_info.reg.size);
+iommu->model = ACPI_IORT_NODE_PV_VIRTIO_IOMMU;
+iommu->flags = ACPI_IORT_NODE_PV_CACHE_COHERENT;
+iommu->gsiv = cpu_to_le64(vms->iommu_info.irq_base);
+
+/* Identity RID mapping covering the whole input RID range */
+idmap = >id_mapping_array[0];
+idmap->input_base = 0;
+idmap->id_count = cpu_to_le32(0x);
+idmap->output_base = 0;
+/* output IORT node is the ITS group node (the first node) */
+idmap->output_reference = cpu_to_le32(iort->node_offset);
+}
+
 /* Root Complex Node */
 node_size = sizeof(*rc) + sizeof(*idmap);
 iort_length += node_size;
@@ -438,10 +472,16 @@ build_iort(GArray *table_data, BIOSLinker *linker)
 idmap->input_base = 0;
 idmap->id_count = cpu_to_le32(0x);
 idmap->output_base = 0;
-/* output IORT node is the ITS group node (the first node) */
-idmap->output_reference = cpu_to_le32(iort->node_offset);
 
-iort->length = cpu_to_le32(iort_length);
+if (vms->iommu_info.type) {
+/* output IORT node is the smmuv3 node */
+idmap->output_reference = cpu_to_le32(iommu_offset);
+} else {
+/* output IORT node is the ITS group node (the first node) */
+idmap->output_reference = cpu_to_le32(iort->node_offset);
+}
+
+   iort->length = cpu_to_le32(iort_length);
 
 build_header(linker, table_data, (void *)(table_data->data + iort_start),
  "IORT", table_data->len - iort_start, 0, NULL, NULL);
@@ -786,7 +826,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables 
*tables)
 
 if (its_class_name() && !vmc->no_its) {
 acpi_add_table(table_offsets, tables_blob);
-build_iort(tables_blob, tables->linker);
+build_iort(tables_blob, tables->linker, vms);
 }
 
 /* XSDT is pointed to by RSDP */
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index cf81716..80740ac 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -969,6 +969,11 @@ static void virtio_iommu_notifier(Notifier *notifier, void 
*data)
 
 object_property_set_bool(obj, false, "msi_bypass", _fatal);
 
+vms->iommu_info.type = VIRT_IOMMU_VIRTIO;
+