From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57585) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bEFQl-00082I-Dg for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:32:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bEFQh-00080D-6I for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:32:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41890) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bEFQg-000807-Up for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:32:51 -0400 Date: Sat, 18 Jun 2016 20:32:44 +0800 From: Peter Xu Message-ID: <20160618123244.GL27635@pxdev.xzpeter.org> References: <1463912514-12658-1-git-send-email-davidkiarie4@gmail.com> <1463912514-12658-3-git-send-email-davidkiarie4@gmail.com> <20160524065432.GF8247@pxdev.xzpeter.org> <5743FD5C.8060002@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [V11 2/4] hw/i386: ACPI IVRS table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Kiarie Cc: Valentine Sinitsyn , QEMU Developers , Jan Kiszka , "Michael S. Tsirkin" , Marcel Apfelbaum On Sat, Jun 18, 2016 at 11:18:29AM +0300, David Kiarie wrote: > On Tue, May 24, 2016 at 10:06 AM, Valentine Sinitsyn > wrote: > > Hi all, > > > > > > On 24.05.2016 11:54, Peter Xu wrote: > >> > >> On Sun, May 22, 2016 at 01:21:52PM +0300, David Kiarie wrote: > >> [...] > >>> > >>> +static void > >>> +build_amd_iommu(GArray *table_data, GArray *linker) > >>> +{ > >>> + int iommu_start = table_data->len; > >>> + bool iommu_ambig; > >>> + > >>> + /* IVRS definition - table header has an extra 2-byte field */ > >>> + acpi_data_push(table_data, (sizeof(AcpiTableHeader))); > >>> + /* common virtualization information */ > >>> + 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); > >>> + /* ivhd length */ > >>> + build_append_int_noprefix(table_data, 0x20, 2); > >>> + /* iommu device id */ > >>> + build_append_int_noprefix(table_data, PCI_DEVICE_ID_RD890_IOMMU, > >>> 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://developer.amd.com/wordpress/media/2012/10/488821.pdf) > >>> + * Table 95 > >> > >> > >> I failed to find Table 95 in the document. Is that typo? > > > > I guess it should be "Table 75". David, am I right? > > On a side note, 2.0 specification you mention is rather outdated. > > Please consider referencing something newer, like 2.6. > > > > > >> > >> [...] > >> > >>> static > >>> void acpi_build(AcpiBuildTables *tables, MachineState *machine) > >>> { > >>> @@ -2657,6 +2721,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 }; > >>> @@ -2722,7 +2787,13 @@ 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); > >>> } > >> > >> > >> Nit: I'd prefer: > >> > >> if (type == Intel) { > >> ... > >> } else if (type == AMD) { > >> ... > >> } > >> > > I missed this is the last version of the patch I should fix it in next version. > > On taking a closer look at this there might be larger problem where > with the advent of -device users can possibly emulate two > IOMMUs at the same time ? A proposed solution was to have > pci_setup_iommu check that DMA hook as not been setup yet and fail if > yes. I should send a fix for that too. Currently we should only support single vIOMMU. If you are going to rebase to x86-iommu codes, there is a patch that includes the check: "[PATCH v9 02/25] x86-iommu: provide x86_iommu_get_default" by: assert(x86_iommu_default == NULL); Maybe we should print something more readable, like "multiple vIOMMUs are not supported yet", rather than an assertion fail. -- peterx