From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38991) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUwCM-0001I0-BZ for qemu-devel@nongnu.org; Sun, 14 Feb 2016 07:54:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aUwCF-0005pG-T2 for qemu-devel@nongnu.org; Sun, 14 Feb 2016 07:54:46 -0500 Received: from mail-wm0-x22e.google.com ([2a00:1450:400c:c09::22e]:37803) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUwCF-0005pA-Hn for qemu-devel@nongnu.org; Sun, 14 Feb 2016 07:54:39 -0500 Received: by mail-wm0-x22e.google.com with SMTP id g62so75820711wme.0 for ; Sun, 14 Feb 2016 04:54:39 -0800 (PST) References: <1453130745-25793-1-git-send-email-davidkiarie4@gmail.com> <1453130745-25793-4-git-send-email-davidkiarie4@gmail.com> From: Marcel Apfelbaum Message-ID: <56C0790C.2040402@gmail.com> Date: Sun, 14 Feb 2016 14:54:36 +0200 MIME-Version: 1.0 In-Reply-To: <1453130745-25793-4-git-send-email-davidkiarie4@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V4 3/4] hw/i386: ACPI table for AMD IO MMU Reply-To: marcel@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Kiarie , qemu-devel@nongnu.org Cc: marcel@redhat.com, crosthwaitepeter@gmail.com, jan.kiszka@web.de, valentine.sinitsyn@gmail.com, mst@redhat.com On 01/18/2016 05:25 PM, David Kiarie wrote: > Add IVRS table for AMD IO MMU. > > Signed-off-by: David Kiarie > --- > hw/i386/acpi-build.c | 70 +++++++++++++++++++++++++++++++++++++++++++++ > include/hw/acpi/acpi-defs.h | 55 +++++++++++++++++++++++++++++++++++ > 2 files changed, 125 insertions(+) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 78758e2..5c0d6b7 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -52,6 +52,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" > @@ -2424,6 +2425,70 @@ build_dmar_q35(GArray *table_data, GArray *linker) > } > > static void > +build_amd_iommu(GArray *table_data, GArray *linker) > +{ > + int iommu_start = table_data->len; > + bool iommu_ambig; > + > + AcpiAMDIOMMUIVRS *ivrs; > + AcpiAMDIOMMUHardwareUnit *iommu; > + > + /* IVRS definition */ > + ivrs = acpi_data_push(table_data, sizeof(*ivrs)); > + ivrs->revision = cpu_to_le16(ACPI_IOMMU_IVRS_TYPE); > + ivrs->length = cpu_to_le16((sizeof(*ivrs) + sizeof(*iommu))); > + ivrs->v_common_info = cpu_to_le64(AMD_IOMMU_HOST_ADDRESS_WIDTH << 8); > + > + AMDIOMMUState *s = (AMDIOMMUState *)object_resolve_path_type("", > + TYPE_AMD_IOMMU_DEVICE, &iommu_ambig); > + > + /* IVDB definition */ > + iommu = acpi_data_push(table_data, sizeof(*iommu)); > + if (!iommu_ambig) { Hi, If the reference to AMD IOMMU is ambiguous and the table is not added to ACPI I think we should report the error to user, something like error_report. > + iommu->type = cpu_to_le16(0x10); > + /* IVHD flags */ > + iommu->flags = cpu_to_le16(iommu->flags); > + iommu->flags = cpu_to_le16(IVHD_HT_TUNEN | IVHD_PPRSUP | IVHD_IOTLBSUP > + | IVHD_PREFSUP); > + iommu->length = cpu_to_le16(sizeof(*iommu)); > + iommu->device_id = cpu_to_le16(PCI_DEVICE_ID_RD890_IOMMU); > + iommu->capability_offset = cpu_to_le16(s->capab_offset); > + iommu->mmio_base = cpu_to_le64(s->mmio.addr); > + iommu->pci_segment = 0; > + iommu->interrupt_info = 0; > + /* EFR features */ > + iommu->efr_register = cpu_to_le64(IVHD_EFR_GTSUP | IVHD_EFR_HATS > + | IVHD_EFR_GATS); > + iommu->efr_register = cpu_to_le64(iommu->efr_register); > + /* device entries */ > + memset(iommu->dev_entries, 0, 20); > + /* Add device flags here > + * create entries for devices to be treated specially by IO MMU, > + * currently we report all devices to IO MMU with no special flags > + * DTE settings made here apply to all devices > + * Refer to AMD IOMMU spec Table 97 > + */ > + iommu->dev_entries[12] = 3; > + iommu->dev_entries[16] = 4; > + iommu->dev_entries[17] = 0xff; > + iommu->dev_entries[18] = 0xff; > + } > + > + build_header(linker, table_data, (void *)(table_data->data + iommu_start), > + "IVRS", table_data->len - iommu_start, 1, NULL); > +} > + > +static bool acpi_has_amd_iommu(void) > +{ > + bool ambiguous; > + Object *amd_iommu; > + > + amd_iommu = object_resolve_path_type("", TYPE_AMD_IOMMU_DEVICE, > + &ambiguous); > + return amd_iommu && !ambiguous; > +} > + > +static void > build_dsdt(GArray *table_data, GArray *linker, > AcpiPmInfo *pm, AcpiMiscInfo *misc) > { > @@ -2691,6 +2756,11 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) > build_dmar_q35(tables_blob, tables->linker); > } > > + if (acpi_has_amd_iommu() && !acpi_has_iommu()) { Since we have the acpi_has_amd_iommu function now, maybe is time to rename acpi_has_iommu to acpi_has_intel_iommu or better have a standard has_iommu that returns an enum type (NONE/INTEL/AMD). By the way, do we really need to check !acpi_has_iommu() if there is an AMD IOMMU in the system? Having both in the same system is a not (yet) supported configuration, right? Thanks, Marcel > + acpi_add_table(table_offsets, tables_blob); > + build_amd_iommu(tables_blob, tables->linker); > + } > + > if (acpi_has_nvdimm()) { > nvdimm_build_acpi(table_offsets, tables_blob, tables->linker); > } > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index c7a03d4..a161358 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -570,4 +570,59 @@ typedef struct AcpiDmarHardwareUnit AcpiDmarHardwareUnit; > /* Masks for Flags field above */ > #define ACPI_DMAR_INCLUDE_PCI_ALL 1 > > +/* IVRS constants */ > +#define ACPI_IOMMU_HARDWAREUNIT_TYPE 0x10 > +#define ACPI_IOMMU_IVRS_TYPE 0x1 > +#define AMD_IOMMU_HOST_ADDRESS_WIDTH 39UL > + > +/* AMD IOMMU IVRS table */ > +struct AcpiAMDIOMMUIVRS { > + ACPI_TABLE_HEADER_DEF > + uint32_t v_common_info; /* common virtualization information */ > + uint64_t reserved; /* reserved */ > +} QEMU_PACKED; > +typedef struct AcpiAMDIOMMUIVRS AcpiAMDIOMMUIVRS; > + > +/* flags in the IVHD headers */ > +#define IVHD_HT_TUNEN (1UL << 0) > +#define IVHD_PASS_PW (1UL << 1) > +#define IVHD_RESPASS_PW (1UL << 2) > +#define IVHD_ISOC (1UL << 3) > +#define IVHD_IOTLBSUP (1UL << 4) > +#define IVHD_COHERENT (1UL << 5) > +#define IVHD_PREFSUP (1UL << 6) > +#define IVHD_PPRSUP (1UL << 7) > + > +/* features in the IVHD headers */ > +#define IVHD_EFR_HATS 48 > +#define IVHD_EFR_GATS 48 > +#define IVHD_EFR_MSI_NUM > +#define IVHD_EFR_PNBANKS > +#define IVHD_EFR_PNCOUNTERS > +#define IVHD_EFR_PASMAX > +#define IVHD_EFR_HESUP (1UL << 7) > +#define IVHD_EFR_GASUP (1UL << 6) > +#define IVHD_EFR_IASUP (1UL << 5) > +#define IVHD_EFR_GLXSUP (3UL << 3) > +#define IVHD_EFR_GTSUP (1UL << 2) > +#define IVHD_EFR_NXSUP (1UL << 1) > +#define IVHD_EFR_XTSUP (1UL << 0) > + > +/* IVDB type 10h */ > +struct AcpiAMDIOMMUHardwareUnit { > + uint8_t type; > + uint8_t flags; > + uint16_t length; > + uint16_t device_id; > + uint16_t capability_offset; > + uint64_t mmio_base; > + uint16_t pci_segment; > + uint16_t interrupt_info; > + uint32_t features; > + uint64_t efr_register; > + uint64_t reserved; > + uint8_t dev_entries[20]; > +} QEMU_PACKED; > +typedef struct AcpiAMDIOMMUHardwareUnit AcpiAMDIOMMUHardwareUnit; > + > #endif >