From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53312) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKAYd-0006Nr-GE for qemu-devel@nongnu.org; Mon, 04 Jul 2016 16:33:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKAYX-0007XK-G9 for qemu-devel@nongnu.org; Mon, 04 Jul 2016 16:33:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34599) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKAYX-0007XG-73 for qemu-devel@nongnu.org; Mon, 04 Jul 2016 16:33:25 -0400 Date: Mon, 4 Jul 2016 23:33:21 +0300 From: "Michael S. Tsirkin" Message-ID: <20160704232621-mutt-send-email-mst@redhat.com> References: <1465993312-18119-1-git-send-email-davidkiarie4@gmail.com> <1465993312-18119-5-git-send-email-davidkiarie4@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1465993312-18119-5-git-send-email-davidkiarie4@gmail.com> Subject: Re: [Qemu-devel] [V12 4/4] hw/i386: AMD IOMMU IVRS table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Kiarie Cc: qemu-devel@nongnu.org, marcel@redhat.com, peterx@redhat.com, alex.williamson@redhat.com, valentine.sinitsyn@gmail.com, jan.kiszka@web.de, ehabkost@redhat.com 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 > --- > 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. > + 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