From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57716) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bEFST-0000rP-9X for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:34:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bEFSP-0008Gd-2K for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:34:40 -0400 Received: from mout.web.de ([212.227.15.14]:64576) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bEFSO-0008GW-NX for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:34:36 -0400 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> <20160618123244.GL27635@pxdev.xzpeter.org> From: Jan Kiszka Message-ID: <57653FD2.4040303@web.de> Date: Sat, 18 Jun 2016 14:34:26 +0200 MIME-Version: 1.0 In-Reply-To: <20160618123244.GL27635@pxdev.xzpeter.org> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="FIvGH4IGfESEbOrUsmDjKPUQPGNNnvJ64" 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: Peter Xu , David Kiarie Cc: Valentine Sinitsyn , QEMU Developers , "Michael S. Tsirkin" , Marcel Apfelbaum This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --FIvGH4IGfESEbOrUsmDjKPUQPGNNnvJ64 From: Jan Kiszka To: Peter Xu , David Kiarie Cc: Valentine Sinitsyn , QEMU Developers , "Michael S. Tsirkin" , Marcel Apfelbaum Message-ID: <57653FD2.4040303@web.de> Subject: Re: [V11 2/4] hw/i386: ACPI IVRS table 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> <20160618123244.GL27635@pxdev.xzpeter.org> In-Reply-To: <20160618123244.GL27635@pxdev.xzpeter.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 2016-06-18 14:32, Peter Xu wrote: > 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 =3D 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_W= IDTH >>>>> << 8, 4); >>>>> + /* reserved */ >>>>> + build_append_int_noprefix(table_data, 0, 8); >>>>> + >>>>> + AMDVIState *s =3D (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 t= he >>>>> range of >>>>> + * devices 00h - ffffh; all devices >>>>> + * Device setting affecting all devices should be made h= ere >>>>> + * >>>>> + * 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 =3D has_iommu(); >>>>> size_t aml_len =3D 0; >>>>> GArray *tables_blob =3D tables->table_data; >>>>> AcpiSlicOem slic_oem =3D { .id =3D NULL, .table_id =3D 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 =3D=3D TYPE_AMD) { >>>>> + acpi_add_table(table_offsets, tables_blob); >>>>> + build_amd_iommu(tables_blob, tables->linker); >>>>> + } >>>>> + >>>>> + if (IOMMUType =3D=3D TYPE_INTEL) { >>>>> acpi_add_table(table_offsets, tables_blob); >>>>> build_dmar_q35(tables_blob, tables->linker); >>>>> } >>>> >>>> >>>> Nit: I'd prefer: >>>> >>>> if (type =3D=3D Intel) { >>>> ... >>>> } else if (type =3D=3D 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. >=20 > 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: >=20 > "[PATCH v9 02/25] x86-iommu: provide x86_iommu_get_default" >=20 > by: >=20 > assert(x86_iommu_default =3D=3D NULL); >=20 > Maybe we should print something more readable, like "multiple vIOMMUs > are not supported yet", rather than an assertion fail. You need proper error handling and a readable error message because nothing else will stop the user from doing -device intel-iommu -device amd-iommu. Jan --FIvGH4IGfESEbOrUsmDjKPUQPGNNnvJ64 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAldlP9IACgkQitSsb3rl5xQfMgCgudPPC+Tp/JQt97ZHEveAimDX 29oAn25uuw3iMQm5js3qWEgd7exKOo1O =z1IM -----END PGP SIGNATURE----- --FIvGH4IGfESEbOrUsmDjKPUQPGNNnvJ64--