qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [Qemu-devel] [RFC v6 18/22] hw/arm/virt-acpi-build: Add v


From: Auger Eric
Subject: Re: [Qemu-arm] [Qemu-devel] [RFC v6 18/22] hw/arm/virt-acpi-build: Add virtio-iommu node in IORT table
Date: Tue, 13 Feb 2018 14:22:08 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0

Hi Drew,
On 13/02/18 13:24, Andrew Jones wrote:
> On Mon, Feb 12, 2018 at 06:58:20PM +0000, 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-0x10000
>> maps to streamid space 0x0-0x10000 in smmuv3, which in turn
>> maps to deviceid space 0x0-0x10000 in the ITS group.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> 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 = &iommu->id_mapping_array[0];
>> +        idmap->input_base = 0;
>> +        idmap->id_count = cpu_to_le32(0xFFFF);
>> +        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(0xFFFF);
>>      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)
ok
> 
>>  
>>      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", &error_fatal);
>>  
>> +    vms->iommu_info.type = VIRT_IOMMU_VIRTIO;
>> +    vms->iommu_info.reg.base = vms->memmap[VIRT_IOMMU].base;
>> +    vms->iommu_info.reg.size = vms->memmap[VIRT_IOMMU].size;
>> +    vms->iommu_info.irq_base = vms->irqmap[VIRT_IOMMU];
>> +
>>      qemu_fdt_setprop_cells(fdt, vms->pcie_host_nodename, "iommu-map",
>>                             0x0, vms->iommu_phandle, 0x0, 0x10000);
>>  }
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index 80c8099..57b9cf9 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -673,7 +673,8 @@ enum {
>>          ACPI_IORT_NODE_NAMED_COMPONENT = 0x01,
>>          ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
>>          ACPI_IORT_NODE_SMMU = 0x03,
>> -        ACPI_IORT_NODE_SMMU_V3 = 0x04
>> +        ACPI_IORT_NODE_SMMU_V3 = 0x04,
>> +        ACPI_IORT_NODE_PARAVIRT = 0x80
> 
> I recommend putting a comma on the last line too, to avoid having to touch
> the line in the future if new enums are added later (as must be done in
> this patch). It's nicer for git-blame
ok
> 
>>  };
>>  
>>  struct AcpiIortIdMapping {
>> @@ -700,6 +701,24 @@ struct AcpiIortItsGroup {
>>  } QEMU_PACKED;
>>  typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>>  
>> +struct AcpiIortPVIommu {
>> +    ACPI_IORT_NODE_HEADER_DEF
>> +    uint64_t base_address;
>> +    uint64_t span;
>> +    uint32_t model;
>> +    uint32_t flags;
>> +    uint64_t gsiv;
>> +    uint64_t reserved2;
>> +    AcpiIortIdMapping id_mapping_array[0];
>> +} QEMU_PACKED;
>> +typedef struct AcpiIortPVIommu AcpiIortPVIommu;
>> +
>> +enum {
>> +    ACPI_IORT_NODE_PV_VIRTIO_IOMMU = 0x00,
>> +};
>> +
>> +#define ACPI_IORT_NODE_PV_CACHE_COHERENT    (1 << 0)
>> +
>>  struct AcpiIortRC {
>>      ACPI_IORT_NODE_HEADER_DEF
>>      AcpiIortMemoryAccess memory_properties;
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index a13b895..2e1e907 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -89,6 +89,18 @@ typedef struct {
>>      bool claim_edge_triggered_timers;
>>  } VirtMachineClass;
>>  
>> +typedef enum VirtIOMMUType {
>> +    VIRT_IOMMU_NONE,
>> +    VIRT_IOMMU_SMMUV3,
>> +    VIRT_IOMMU_VIRTIO,
>> +} VirtIOMMUType;
>> +
>> +typedef struct VirtIOMMUInfo {
>> +    VirtIOMMUType type;
>> +    MemMapEntry reg;
>> +    int irq_base;
>> +} VirtIOMMUInfo;
>> +
>>  typedef struct {
>>      MachineState parent;
>>      Notifier machine_done;
>> @@ -98,6 +110,7 @@ typedef struct {
>>      bool highmem;
>>      bool its;
>>      bool virt;
>> +    VirtIOMMUInfo iommu_info;
>>      int32_t gic_version;
>>      struct arm_boot_info bootinfo;
>>      const MemMapEntry *memmap;
>> -- 
>> 1.9.1
>>
>>
> 
> I just caught the above comments while skimming. I'm afraid I didn't have
> time to review this to the spec yet.

Thank you for the review!

Eric
> 
> Thanks,
> drew
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]