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

[Le Tan]

Hi Michael,

2014-08-14 19:06 GMT+08:00 Michael S. Tsirkin <address@hidden>:
> 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 ...

For now it is just used once. I wrote this because I thought that it
is convenient to add other sub-tables in the future. To make it
cleaner, I will remove it later.
Thanks very much!

Le

>> +
>> +/* 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