| Message ID | 1506049330-11196-6-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + 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? > + > + 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. Thanks, Roger.
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.
On Thu, Oct 19, 2017 at 04:09:02PM +0800, Lan Tianyu wrote: > On 2017年10月18日 23:12, Roger Pau Monné wrote: > >> 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. Did you check whether including stdbool is actually required? AFAICT hvmloader util.h already includes it, and you would only have to introduce it in libxl if it's not there yet. Roger.
>>> On 19.10.17 at 10:09, <tianyu.lan@intel.com> wrote: > On 2017年10月18日 23:12, Roger Pau Monné wrote: >> On Thu, Sep 21, 2017 at 11:01:46PM -0400, Lan Tianyu wrote: >>> --- 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. Please don't - if you mean boolean, please use bool. Roger's remark, aiui, wasn't about you using bool, but how you do the header inclusion. Jan
On 2017年10月19日 16:40, Roger Pau Monné wrote: > On Thu, Oct 19, 2017 at 04:09:02PM +0800, Lan Tianyu wrote: >> On 2017年10月18日 23:12, Roger Pau Monné wrote: >>>> 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. > > Did you check whether including stdbool is actually required? AFAICT > hvmloader util.h already includes it, and you would only have to > introduce it in libxl if it's not there yet. > Yes, you are right. stdbool.h has introduced in both libxl(libxl.h) and hvmloader(util.h). We just need to adjust include order.
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]); + + 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 = 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; + + 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 = 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> + #define ACPI_HAS_COM1 (1<<0) #define ACPI_HAS_COM2 (1<<1) #define ACPI_HAS_LPT1 (1<<2) @@ -96,8 +98,18 @@ struct acpi_config { uint32_t ioapic_base_address; uint16_t pci_isa_irq_mask; uint8_t ioapic_id; + + /* Emulated IOMMU features, location and IOAPIC under the scope of IOMMU */ + bool iommu_intremap_supported; + bool iommu_x2apic_supported; + uint8_t host_addr_width; + uint8_t ioapic_bus; + uint16_t ioapic_devfn; + 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__ */