Re: [Qemu-devel] [PATCH v10 08/26] acpi: add DMAR scope definition for root IOAPIC

[Michael S. Tsirkin]

On Tue, Jun 21, 2016 at 03:47:36PM +0800, Peter Xu wrote:
> To enable interrupt remapping for intel IOMMU device, each IOAPIC device
> in the system reported via ACPI MADT must be explicitly enumerated under
> one specific remapping hardware unit. This patch adds the root-complex
> IOAPIC into the default DMAR device.
> 
> Please refer to VT-d spec 8.3.1.1 for more information.
> 
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  hw/i386/acpi-build.c        | 19 ++++++++++++++++---
>  include/hw/acpi/acpi-defs.h | 15 +++++++++++++++
>  include/hw/pci-host/q35.h   |  8 ++++++++
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 961ccd6a..eec022e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -77,6 +77,9 @@
>  #define ACPI_BUILD_DPRINTF(fmt, ...)
>  #endif
>  
> +/* Default IOAPIC ID */
> +#define ACPI_BUILD_IOAPIC_ID 0x0
> +
>  typedef struct AcpiMcfgInfo {
>      uint64_t mcfg_base;
>      uint32_t mcfg_size;
> @@ -370,7 +373,6 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
> PCMachineState *pcms)
>      io_apic = acpi_data_push(table_data, sizeof *io_apic);
>      io_apic->type = ACPI_APIC_IO;
>      io_apic->length = sizeof(*io_apic);
> -#define ACPI_BUILD_IOAPIC_ID 0x0
>      io_apic->io_apic_id = ACPI_BUILD_IOAPIC_ID;
>      io_apic->address = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS);
>      io_apic->interrupt = cpu_to_le32(0);
> @@ -2425,6 +2427,9 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker)
>      AcpiDmarHardwareUnit *drhd;
>      uint8_t dmar_flags = 0;
>      X86IOMMUState *iommu = x86_iommu_get_default();
> +    AcpiDmarDeviceScope *scope = NULL;
> +    /* Root complex IOAPIC use one path[0] only */
> +    uint8_t ioapic_scope_size = sizeof(*scope) + sizeof(scope->path[0]);

just use int or unsigned or size_t for types like this.

>  
>      assert(iommu);
>      if (iommu->intr_supported) {
> @@ -2437,13 +2442,21 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker)
>      dmar->flags = dmar_flags;
>  
>      /* DMAR Remapping Hardware Unit Definition structure */
> -    drhd = acpi_data_push(table_data, sizeof(*drhd));
> +    drhd = acpi_data_push(table_data, sizeof(*drhd) + ioapic_scope_size);
>      drhd->type = cpu_to_le16(ACPI_DMAR_TYPE_HARDWARE_UNIT);
> -    drhd->length = cpu_to_le16(sizeof(*drhd));   /* No device scope now */
> +    drhd->length = cpu_to_le16(sizeof(*drhd) + ioapic_scope_size);
>      drhd->flags = ACPI_DMAR_INCLUDE_PCI_ALL;
>      drhd->pci_segment = cpu_to_le16(0);
>      drhd->address = cpu_to_le64(Q35_HOST_BRIDGE_IOMMU_ADDR);
>  
> +    /* Scope definition for the root-complex IOAPIC */
> +    scope = &drhd->scope[0];
> +    scope->entry_type = ACPI_DMAR_DEV_SCOPE_TYPE_IOAPIC;
> +    scope->length = ioapic_scope_size;
> +    scope->enumeration_id = ACPI_BUILD_IOAPIC_ID;
> +    scope->bus = Q35_PSEUDO_BUS_PLATFORM;
> +    scope->path[0] = cpu_to_le16(Q35_PSEUDO_DEVFN_IOAPIC);
> +
>      build_header(linker, table_data, (void *)(table_data->data + dmar_start),
>                   "DMAR", table_data->len - dmar_start, 1, NULL, NULL);
>  }
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index ea9be0b..0dbdde3 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -571,6 +571,20 @@ enum {
>  /*
>   * Sub-structures for DMAR
>   */
> +
> +#define ACPI_DMAR_DEV_SCOPE_TYPE_IOAPIC     (0x03)

Again, pls use literal with comment in code.

> +
> +/* Device scope structure for DRHD. */
> +struct AcpiDmarDeviceScope {
> +    uint8_t entry_type;
> +    uint8_t length;
> +    uint16_t reserved;
> +    uint8_t enumeration_id;
> +    uint8_t bus;
> +    uint16_t path[0];           /* list of dev:func pairs */
> +} QEMU_PACKED;
> +typedef struct AcpiDmarDeviceScope AcpiDmarDeviceScope;
> +
>  /* Type 0: Hardware Unit Definition */
>  struct AcpiDmarHardwareUnit {
>      uint16_t type;
> @@ -579,6 +593,7 @@ struct AcpiDmarHardwareUnit {
>      uint8_t reserved;
>      uint16_t pci_segment;   /* The PCI Segment associated with this unit */
>      uint64_t address;   /* Base address of remapping hardware register-set */
> +    AcpiDmarDeviceScope scope[0];
>  } QEMU_PACKED;
>  typedef struct AcpiDmarHardwareUnit AcpiDmarHardwareUnit;
>  




> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index c5c073d..312b47f 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -175,4 +175,12 @@ typedef struct Q35PCIHost {
>  
>  uint64_t mch_mcfg_base(void);
>  
> +/*
> + * Arbitary but unique BNF number for IOAPIC device.
> + *
> + * TODO: make sure there would have no conflict with real PCI bus

How are you going to do this?

> + */
> +#define Q35_PSEUDO_BUS_PLATFORM         (0xff)
> +#define Q35_PSEUDO_DEVFN_IOAPIC         (0x00)
> +
>  #endif /* HW_Q35_H */
> -- 
> 2.4.11