Re: [PATCH V3 5/29] tools/libacpi: Add new fields in acpi_config for DMAR table

[Lan Tianyu <tianyu.lan@intel.com>]

On 2017年10月18日 23:12, Roger Pau Monné wrote:
> On Thu, Sep 21, 2017 at 11:01:46PM -0400, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>>
>> 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
>                  ^ introduced
>> 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@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>> v3:
>>  - Remove chip-set specific IOAPIC BDF. Instead, let IOAPIC-related
>>  info be passed by struct acpi_config.
>>
>> ---
>>  tools/libacpi/build.c   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  tools/libacpi/libacpi.h | 12 +++++++++++
>>  2 files changed, 65 insertions(+)
>>
>> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
>> index f9881c9..5ee8fcd 100644
>> --- a/tools/libacpi/build.c
>> +++ b/tools/libacpi/build.c
>> @@ -303,6 +303,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]);
> 
> I'm not sure I follow why you need to add the size of a uint16_t here.
> 
>> +
>> +    size = sizeof(*dmar) + sizeof(*drhd) + ioapic_scope_size;
> 
> size can be initialized at declaration time.
> 
>> +
>> +    dmar = ctxt->mem_ops.alloc(ctxt, size, 16);
> 
> Even dmar can be initialized at declaration time.
> 

OK. Will update.

>> +    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 = config->host_addr_width - 1;
>> +    if ( config->iommu_intremap_supported )
>> +        dmar->flags |= ACPI_DMAR_INTR_REMAP;
>> +    if ( !config->iommu_x2apic_supported )
>> +        dmar->flags |= ACPI_DMAR_X2APIC_OPT_OUT;
> 
> Is there any reason why we would want to create a guest with a vIOMMU
> but not x2APIC support?

Will remove this.

> 
>> +
>> +    drhd = (struct acpi_dmar_hardware_unit *)((void*)dmar + sizeof(*dmar));
>                                                       ^ space
>> +    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 = config->ioapic_bus;
>> +    scope->path[0] = config->ioapic_devfn;
>> +
>> +    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 a2efd23..fdd6a78 100644
>> --- a/tools/libacpi/libacpi.h
>> +++ b/tools/libacpi/libacpi.h
>> @@ -20,6 +20,8 @@
>>  #ifndef __LIBACPI_H__
>>  #define __LIBACPI_H__
>>  
>> +#include <stdbool.h>
> 
> I'm quite sure you shouldn't add this here, see how headers are added
> using LIBACPI_STDUTILS.
> 

We may replace bool with uint8_t xxx:1 to avoid introduce new head file.

-- 
Best regards
Tianyu Lan

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