Re: [Xen-devel] [PATCH V2 7/25] tools/libacpi: Add new fields in acpi_config for DMAR table

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Aug 09, 2017 at 04:34:08PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> The BIOS reports the remapping hardware units in a platform to system software
> through the DMA Remapping Reporting (DMAR) ACPI table.
> New fields are introduces for DMAR table. These new fields are set by
                          ^ s/s/d/ to libacpi
> toolstack through parsing guest's config file. construct_dmar() is added to
> build DMAR table according to the new fields.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  tools/libacpi/build.c   | 57 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/libacpi/libacpi.h |  9 ++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
> index f9881c9..c7cc784 100644
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -28,6 +28,10 @@
>  
>  #define ACPI_MAX_SECONDARY_TABLES 16

A comment about the meaning of the defines below might be helpful.

> +#define VTD_HOST_ADDRESS_WIDTH 39
> +#define I440_PSEUDO_BUS_PLATFORM 0xff
> +#define I440_PSEUDO_DEVFN_IOAPIC 0x0
> +
>  #define align16(sz)        (((sz) + 15) & ~15)
>  #define fixed_strcpy(d, s) strncpy((d), (s), sizeof(d))
>  
> @@ -303,6 +307,59 @@ static struct acpi_20_slit *construct_slit(struct 
> acpi_ctxt *ctxt,
>      return slit;
>  }
>  
> +/*
> + * Only one DMA remapping hardware unit is exposed and all devices
> + * are under the remapping hardware unit. I/O APIC should be explicitly
> + * enumerated.
> + */
> +struct acpi_dmar *construct_dmar(struct acpi_ctxt *ctxt,
> +                                 const struct acpi_config *config)
> +{
> +    struct acpi_dmar *dmar;
> +    struct acpi_dmar_hardware_unit *drhd;
> +    struct dmar_device_scope *scope;
> +    unsigned int size;
> +    unsigned int ioapic_scope_size = sizeof(*scope) + sizeof(scope->path[0]);
> +
> +    size = sizeof(*dmar) + sizeof(*drhd) + ioapic_scope_size;
> +
> +    dmar = ctxt->mem_ops.alloc(ctxt, size, 16);
> +    if ( !dmar )
> +        return NULL;
> +
> +    memset(dmar, 0, size);
> +    dmar->header.signature = ACPI_2_0_DMAR_SIGNATURE;
> +    dmar->header.revision = ACPI_2_0_DMAR_REVISION;
> +    dmar->header.length = size;
> +    fixed_strcpy(dmar->header.oem_id, ACPI_OEM_ID);
> +    fixed_strcpy(dmar->header.oem_table_id, ACPI_OEM_TABLE_ID);
> +    dmar->header.oem_revision = ACPI_OEM_REVISION;
> +    dmar->header.creator_id   = ACPI_CREATOR_ID;
> +    dmar->header.creator_revision = ACPI_CREATOR_REVISION;
> +    dmar->host_address_width = VTD_HOST_ADDRESS_WIDTH - 1;
> +    if ( config->iommu_intremap_supported )
> +        dmar->flags = ACPI_DMAR_INTR_REMAP;

Since you initialize flags to 0 I would use |= here, in case this gets
moved later and this is not the first flag to be set.

> +    if ( !config->iommu_x2apic_supported )
> +        dmar->flags |= ACPI_DMAR_X2APIC_OPT_OUT;

I'm not sure of the reason behind not supporting x2APIC mode (I've
already commented in another patch).

> +    drhd = (struct acpi_dmar_hardware_unit *)((void*)dmar + sizeof(*dmar));
> +    drhd->type = ACPI_DMAR_TYPE_HARDWARE_UNIT;
> +    drhd->length = sizeof(*drhd) + ioapic_scope_size;
> +    drhd->flags = ACPI_DMAR_INCLUDE_PCI_ALL;
> +    drhd->pci_segment = 0;
> +    drhd->base_address = config->iommu_base_addr;
> +
> +    scope = &drhd->scope[0];
> +    scope->type = ACPI_DMAR_DEVICE_SCOPE_IOAPIC;
> +    scope->length = ioapic_scope_size;
> +    scope->enumeration_id = config->ioapic_id;
> +    scope->bus = I440_PSEUDO_BUS_PLATFORM;
> +    scope->path[0] = I440_PSEUDO_DEVFN_IOAPIC;

I'm not sure whether this constants should instead be fields in the
acpi_config struct passed down from libxl. libxc shouldn't really need
to know anything about which chipset a VM is using.

> +    set_checksum(dmar, offsetof(struct acpi_header, checksum), size);
> +    return dmar;
> +}
> +
>  static int construct_passthrough_tables(struct acpi_ctxt *ctxt,
>                                          unsigned long *table_ptrs,
>                                          int nr_tables,
> diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
> index 2ed1ecf..74778a5 100644
> --- a/tools/libacpi/libacpi.h
> +++ b/tools/libacpi/libacpi.h
> @@ -20,6 +20,8 @@
>  #ifndef __LIBACPI_H__
>  #define __LIBACPI_H__
>  
> +#include <stdbool.h>
> +
>  #define ACPI_HAS_COM1              (1<<0)
>  #define ACPI_HAS_COM2              (1<<1)
>  #define ACPI_HAS_LPT1              (1<<2)
> @@ -96,8 +98,15 @@ struct acpi_config {
>      uint32_t ioapic_base_address;
>      uint16_t pci_isa_irq_mask;
>      uint8_t ioapic_id;
> +
> +    /* Emulated IOMMU features and location */
> +    bool iommu_intremap_supported;
> +    bool iommu_x2apic_supported;
> +    uint64_t iommu_base_addr;
>  };
>  
> +struct acpi_dmar *construct_dmar(struct acpi_ctxt *ctxt,
> +                                 const struct acpi_config *config);
>  int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config);
>  
>  #endif /* __LIBACPI_H__ */
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel