On Mon, Jul 4, 2016 at 11:33 PM, Michael S. Tsirkin <m...@redhat.com> wrote: > On Wed, Jun 15, 2016 at 03:21:52PM +0300, David Kiarie wrote: >> Add IVRS table for AMD IOMMU. Generate IVRS or DMAR >> depending on emulated IOMMU. >> >> Signed-off-by: David Kiarie <davidkiar...@gmail.com> >> --- >> hw/acpi/aml-build.c | 2 +- >> hw/i386/acpi-build.c | 95 >> +++++++++++++++++++++++++++++++++++++++------ >> include/hw/acpi/acpi-defs.h | 13 +++++++ >> include/hw/acpi/aml-build.h | 1 + >> 4 files changed, 99 insertions(+), 12 deletions(-) >> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >> index 123160a..9ce10aa 100644 >> --- a/hw/acpi/aml-build.c >> +++ b/hw/acpi/aml-build.c >> @@ -226,7 +226,7 @@ static void build_extop_package(GArray *package, uint8_t >> op) >> build_prepend_byte(package, 0x5B); /* ExtOpPrefix */ >> } >> >> -static void build_append_int_noprefix(GArray *table, uint64_t value, int >> size) >> +void build_append_int_noprefix(GArray *table, uint64_t value, int size) >> { >> int i; >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index 8ca2032..ecdb15d 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -51,6 +51,7 @@ >> #include "hw/pci/pci_bus.h" >> #include "hw/pci-host/q35.h" >> #include "hw/i386/intel_iommu.h" >> +#include "hw/i386/amd_iommu.h" >> #include "hw/timer/hpet.h" >> >> #include "hw/acpi/aml-build.h" >> @@ -116,6 +117,12 @@ typedef struct AcpiBuildPciBusHotplugState { >> bool pcihp_bridge_en; >> } AcpiBuildPciBusHotplugState; >> >> +typedef enum IommuType { >> + TYPE_INTEL, >> + TYPE_AMD, >> + TYPE_NONE >> +} IommuType; >> + >> static void acpi_get_pm_info(AcpiPmInfo *pm) >> { >> Object *piix = piix4_pm_find(); >> @@ -2439,6 +2446,77 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker) >> "DMAR", table_data->len - dmar_start, 1, NULL, NULL); >> } >> >> +static void >> +build_amd_iommu(GArray *table_data, BIOSLinker *linker) >> +{ >> + int iommu_start = table_data->len; >> + bool iommu_ambig; >> + >> + /* IVRS definition - table header has an extra 2-byte field */ > > Pls document the spec where this comes from, version and relevant chapters.
Misleading comment here. I have fixed the rest. > >> + acpi_data_push(table_data, sizeof(AcpiTableHeader)); >> + /* common virtualization information */ > > Please change comments to match the spec exactly, case and all. E.g. /* > IVinfo - I/O > virtualization information common to all IOMMU units in a > system */ > >> + build_append_int_noprefix(table_data, AMD_IOMMU_HOST_ADDRESS_WIDTH << >> 8, 4); >> + /* reserved */ >> + build_append_int_noprefix(table_data, 0, 8); >> + >> + AMDVIState *s = (AMDVIState *)object_resolve_path_type("", >> + TYPE_AMD_IOMMU_DEVICE, &iommu_ambig); >> + >> + /* IVDB definition - type 10h */ >> + if (!iommu_ambig) { >> + /* IVHD definition - type 10h */ >> + build_append_int_noprefix(table_data, 0x10, 1); >> + /* virtualization flags */ >> + build_append_int_noprefix(table_data, (IVHD_HT_TUNEN | >> + IVHD_PPRSUP | IVHD_IOTLBSUP | IVHD_PREFSUP), 1); > > Just open-code it, and add a comment matching spec. > This is how we do it in ACPI since no constant is ever > reused anywhere. > E.g. > > (1 << 0) /* HtTunEn */ | (1 << 7) /* PPRSup */ ..... > > if you do this, you will also see it's not ordered, so > you can sort by bit #. > > >> + /* ivhd length */ >> + build_append_int_noprefix(table_data, 0x20, 2); >> + /* iommu device id */ >> + build_append_int_noprefix(table_data, s->devid, 2); >> + /* offset of capability registers */ >> + build_append_int_noprefix(table_data, s->capab_offset, 2); >> + /* mmio base register */ >> + build_append_int_noprefix(table_data, s->mmio.addr, 8); >> + /* pci segment */ >> + build_append_int_noprefix(table_data, 0, 2); >> + /* interrupt numbers */ >> + build_append_int_noprefix(table_data, 0, 2); >> + /* feature reporting */ >> + build_append_int_noprefix(table_data, (IVHD_EFR_GTSUP | >> + IVHD_EFR_HATS | IVHD_EFR_GATS), 4); >> + /* Add device flags here >> + * These are 4-byte device entries currently reporting the range >> of >> + * devices 00h - ffffh; all devices >> + * Device setting affecting all devices should be made here >> + * >> + * Refer to >> + * (http://support.amd.com/TechDocs/48882_IOMMU.pdf) > > link should go on top, also pls list document name > since links are often broken. > >> + * Table 95 > > name of table please. > >> + */ >> + /* start of device range, 4-byte entries */ >> + build_append_int_noprefix(table_data, 0x00000003, 4); >> + /* end of device range */ >> + build_append_int_noprefix(table_data, 0x00ffff04, 4); >> + } >> + >> + build_header(linker, table_data, (void *)(table_data->data + >> iommu_start), >> + "IVRS", table_data->len - iommu_start, 1, NULL, NULL); >> +} >> + >> +static IommuType has_iommu(void) >> +{ >> + bool ambiguous; >> + >> + if (object_resolve_path_type("", TYPE_AMD_IOMMU_DEVICE, &ambiguous) >> + && !ambiguous) >> + return TYPE_AMD; >> + else if (object_resolve_path_type("", TYPE_INTEL_IOMMU_DEVICE, >> &ambiguous) >> + && !ambiguous) >> + return TYPE_INTEL; >> + else >> + return TYPE_NONE; >> +} >> + >> static GArray * >> build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset) >> { >> @@ -2498,16 +2576,6 @@ 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(AcpiBuildTables *tables, MachineState *machine) >> { >> @@ -2520,6 +2588,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState >> *machine) >> AcpiMcfgInfo mcfg; >> PcPciInfo pci; >> uint8_t *u; >> + IommuType IOMMUType = has_iommu(); >> size_t aml_len = 0; >> GArray *tables_blob = tables->table_data; >> AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL }; >> @@ -2586,7 +2655,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState >> *machine) >> acpi_add_table(table_offsets, tables_blob); >> build_mcfg_q35(tables_blob, tables->linker, &mcfg); >> } >> - if (acpi_has_iommu()) { >> + if (IOMMUType == TYPE_AMD) { >> + acpi_add_table(table_offsets, tables_blob); >> + build_amd_iommu(tables_blob, tables->linker); >> + } >> + if (IOMMUType == TYPE_INTEL) { >> acpi_add_table(table_offsets, tables_blob); >> build_dmar_q35(tables_blob, tables->linker); >> } >> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h >> index 850a962..3de7f82 100644 >> --- a/include/hw/acpi/acpi-defs.h >> +++ b/include/hw/acpi/acpi-defs.h >> @@ -583,4 +583,17 @@ typedef struct AcpiDmarHardwareUnit >> AcpiDmarHardwareUnit; >> /* Masks for Flags field above */ >> #define ACPI_DMAR_INCLUDE_PCI_ALL 1 >> >> +/* IVRS constants */ >> +#define AMD_IOMMU_HOST_ADDRESS_WIDTH 40UL >> + >> +/* flags in the IVHD headers */ >> +#define IVHD_HT_TUNEN (1UL << 0) /* recommended setting for HtTunEn */ >> +#define IVHD_IOTLBSUP (1UL << 4) /* remote IOTLB support */ >> +#define IVHD_PREFSUP (1UL << 6) /* page prefetch support */ >> +#define IVHD_PPRSUP (1UL << 7) /* peripheral page service support */ >> + >> +#define IVHD_EFR_HATS 48 /* host address translation size */ >> +#define IVHD_EFR_GATS 48 /* guest address translation size */ >> +#define IVHD_EFR_GTSUP (1UL << 2) /* guest translation support */ >> + >> #endif >> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h >> index 10c09ca..ef874ff 100644 >> --- a/include/hw/acpi/aml-build.h >> +++ b/include/hw/acpi/aml-build.h >> @@ -364,6 +364,7 @@ Aml *aml_derefof(Aml *arg); >> Aml *aml_sizeof(Aml *arg); >> Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target); >> >> +void build_append_int_noprefix(GArray *table, uint64_t value, int size); >> void >> build_header(BIOSLinker *linker, GArray *table_data, >> AcpiTableHeader *h, const char *sig, int len, uint8_t rev, >> -- >> 2.1.4