Re: [Qemu-devel] [PATCH v3 3/5] intel-iommu: add DMAR table to ACPI tables

[Michael S. Tsirkin]

On Mon, Aug 11, 2014 at 03:05:00PM +0800, Le Tan wrote:
> Expose Intel IOMMU to the BIOS. If object of TYPE_INTEL_IOMMU_DEVICE exists,
> add DMAR table to ACPI RSDT table. For now the DMAR table indicates that there
> is only one hardware unit without INTR_REMAP capability on the platform.
> 
> Signed-off-by: Le Tan <address@hidden>

Could you add a unit test please?

> ---
>  hw/i386/acpi-build.c | 41 ++++++++++++++++++++++++++++++
>  hw/i386/acpi-defs.h  | 70 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 816c6d9..595f501 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -47,6 +47,7 @@
>  #include "hw/i386/ich9.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/pci-host/q35.h"
> +#include "hw/i386/intel_iommu.h"
>  
>  #include "hw/i386/q35-acpi-dsdt.hex"
>  #include "hw/i386/acpi-dsdt.hex"
> @@ -1350,6 +1351,31 @@ build_mcfg_q35(GArray *table_data, GArray *linker, 
> AcpiMcfgInfo *info)
>  }
>  
>  static void
> +build_dmar_q35(GArray *table_data, GArray *linker)
> +{
> +    int dmar_start = table_data->len;
> +
> +    AcpiTableDmar *dmar;
> +    AcpiDmarHardwareUnit *drhd;
> +
> +    dmar = acpi_data_push(table_data, sizeof(*dmar));
> +    dmar->host_address_width = VTD_HOST_ADDRESS_WIDTH - 1;
> +    dmar->flags = 0;    /* No intr_remap for now */
> +
> +    /* DMAR Remapping Hardware Unit Definition structure */
> +    drhd = acpi_data_push(table_data, sizeof(*drhd));
> +    drhd->type = cpu_to_le16(ACPI_DMAR_TYPE_HARDWARE_UNIT);
> +    drhd->length = cpu_to_le16(sizeof(*drhd));   /* No device scope now */
> +    drhd->flags = ACPI_DMAR_INCLUDE_PCI_ALL;
> +    drhd->pci_segment = cpu_to_le16(0);
> +    drhd->address = cpu_to_le64(Q35_HOST_BRIDGE_IOMMU_ADDR);
> +
> +    build_header(linker, table_data, (void *)(table_data->data + dmar_start),
> +                 "DMAR", table_data->len - dmar_start, 1);
> +}
> +
> +
> +static void
>  build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc)
>  {
>      AcpiTableHeader *dsdt;
> @@ -1470,6 +1496,17 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      return true;
>  }
>  
> +static bool acpi_has_iommu(void)
> +{
> +    bool ambiguous;
> +    Object *intel_iommu;
> +
> +    intel_iommu = object_resolve_path_type("", TYPE_INTEL_IOMMU_DEVICE,
> +                                           &ambiguous);
> +    return intel_iommu && !ambiguous;
> +}
> +
> +
>  static
>  void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>  {
> @@ -1539,6 +1576,10 @@ void acpi_build(PcGuestInfo *guest_info, 
> AcpiBuildTables *tables)
>          acpi_add_table(table_offsets, tables->table_data);
>          build_mcfg_q35(tables->table_data, tables->linker, &mcfg);
>      }
> +    if (acpi_has_iommu()) {
> +        acpi_add_table(table_offsets, tables->table_data);
> +        build_dmar_q35(tables->table_data, tables->linker);
> +    }
>  
>      /* Add tables supplied by user (if any) */
>      for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
> diff --git a/hw/i386/acpi-defs.h b/hw/i386/acpi-defs.h
> index e93babb..9674825 100644
> --- a/hw/i386/acpi-defs.h
> +++ b/hw/i386/acpi-defs.h
> @@ -314,4 +314,74 @@ struct AcpiTableMcfg {
>  } QEMU_PACKED;
>  typedef struct AcpiTableMcfg AcpiTableMcfg;
>  
> +/* DMAR - DMA Remapping table r2.2 */
> +struct AcpiTableDmar {
> +    ACPI_TABLE_HEADER_DEF
> +    uint8_t host_address_width; /* Maximum DMA physical addressability */
> +    uint8_t flags;
> +    uint8_t reserved[10];
> +} QEMU_PACKED;
> +typedef struct AcpiTableDmar AcpiTableDmar;
> +
> +/* Masks for Flags field above */
> +#define ACPI_DMAR_INTR_REMAP    (1)
> +#define ACPI_DMAR_X2APIC_OPT_OUT    (2)
> +
> +/*
> + * DMAR sub-structures (Follow DMA Remapping table)
> + */
> +#define ACPI_DMAR_SUB_HEADER_DEF /* Common ACPI DMAR sub-structure header */\
> +    uint16_t type;  \
> +    uint16_t length;

Really necessary? cleaner to just open-code, it's
only used once ...

> +
> +/* Values for sub-structure type for DMAR */
> +enum {
> +    ACPI_DMAR_TYPE_HARDWARE_UNIT = 0,   /* DRHD */
> +    ACPI_DMAR_TYPE_RESERVED_MEMORY = 1, /* RMRR */
> +    ACPI_DMAR_TYPE_ATSR = 2,    /* ATSR */
> +    ACPI_DMAR_TYPE_HARDWARE_AFFINITY = 3,   /* RHSR */
> +    ACPI_DMAR_TYPE_ANDD = 4,    /* ANDD */
> +    ACPI_DMAR_TYPE_RESERVED = 5 /* Reserved for furture use */
> +};
> +
> +/*
> + * Sub-structures for DMAR, correspond to Type in ACPI_DMAR_SUB_HEADER_DEF
> + */
> +
> +/* DMAR Device Scope structures */
> +struct AcpiDmarDeviceScope {
> +    uint8_t type;
> +    uint8_t length;
> +    uint16_t reserved;
> +    uint8_t enumeration_id;
> +    uint8_t start_bus_number;
> +    uint8_t path[0];
> +} QEMU_PACKED;
> +typedef struct AcpiDmarDeviceScope AcpiDmarDeviceScope;
> +
> +/* Values for type in struct AcpiDmarDeviceScope */
> +enum {
> +    ACPI_DMAR_SCOPE_TYPE_NOT_USED = 0,
> +    ACPI_DMAR_SCOPE_TYPE_ENDPOINT = 1,
> +    ACPI_DMAR_SCOPE_TYPE_BRIDGE = 2,
> +    ACPI_DMAR_SCOPE_TYPE_IOAPIC = 3,
> +    ACPI_DMAR_SCOPE_TYPE_HPET = 4,
> +    ACPI_DMAR_SCOPE_TYPE_ACPI = 5,
> +    ACPI_DMAR_SCOPE_TYPE_RESERVED = 6 /* Reserved for future use */
> +};
> +
> +/* 0: Hardware Unit Definition */
> +struct AcpiDmarHardwareUnit {
> +    ACPI_DMAR_SUB_HEADER_DEF
> +    uint8_t flags;
> +    uint8_t reserved;
> +    uint16_t pci_segment;   /* The PCI Segment associated with this unit */
> +    uint64_t address;   /* Base address of remapping hardware register-set */
> +} QEMU_PACKED;
> +typedef struct AcpiDmarHardwareUnit AcpiDmarHardwareUnit;
> +
> +/* Masks for Flags field above */
> +#define ACPI_DMAR_INCLUDE_PCI_ALL (1)
> +
> +

Don't add two empty lines in a row.
Same in other places.

>  #endif
> -- 
> 1.9.1