[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [V8 2/4] hw/i386: ACPI table for AMD IOMMU
|
From: |
Igor Mammedov |
|
Subject: |
Re: [Qemu-devel] [V8 2/4] hw/i386: ACPI table for AMD IOMMU |
|
Date: |
Wed, 6 Apr 2016 18:25:58 +0200 |
On Fri, 1 Apr 2016 21:39:52 +0300
David Kiarie <address@hidden> wrote:
> Add IVRS table for AMD IOMMU. Generate IVRS or DMAR
> depending on emulated IOMMU
>
> Signed-off-by: David Kiarie <address@hidden>
> ---
> hw/i386/acpi-build.c | 98
> ++++++++++++++++++++++++++++++++++++++-----
> include/hw/acpi/acpi-defs.h | 55 ++++++++++++++++++++++++
> include/hw/i386/intel_iommu.h | 1 +
> 3 files changed, 143 insertions(+), 11 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 35180ef..a54a22c 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"
> @@ -118,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
> bool pcihp_bridge_en;
> } AcpiBuildPciBusHotplugState;
>
> +typedef enum iommu_type {
> + TYPE_AMD,
> + TYPE_INTEL,
> + TYPE_NONE
> +} iommu_type;
> +
> static void acpi_get_pm_info(AcpiPmInfo *pm)
> {
> Object *piix = piix4_pm_find();
> @@ -2588,6 +2595,78 @@ build_dmar_q35(GArray *table_data, GArray *linker)
> "DMAR", table_data->len - dmar_start, 1, NULL, NULL);
> }
>
> +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);
s/ACPI_IOMMU_IVRS_TYPE/1/ it's the only usage site and
well it's not type but revision number,
Pls try to keep names close to spec, so that is would be easy
to review code.
> + ivrs->length = cpu_to_le16((sizeof(*ivrs) + sizeof(*iommu)));
you are expecting length be ^^^ from starters so push above should be for
this size.
> + ivrs->v_common_info = cpu_to_le64(AMD_IOMMU_HOST_ADDRESS_WIDTH << 8);
shouldn't above block go below (AMDIOMMUState *)object_resolve_path_type(...,
i.e. is there point in pushing empty table?
s/v_common_info/iv_info/ as in spec
also it's 32 bit in size so using here cpu_to_le64() is wrong.
> +
> + AMDIOMMUState *s = (AMDIOMMUState *)object_resolve_path_type("",
> + TYPE_AMD_IOMMU_DEVICE, &iommu_ambig);
Maybe it should assert on iommu_ambig == true
and get rid of if(!iommu_ambig) condition
> +
> + /* IVDB definition - type 10h */
s/IVDB definition - type 10h/Create IVHD entry/
> + iommu = acpi_data_push(table_data, sizeof(*iommu));
> + if (!iommu_ambig) {
> + 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);
double assignment looks rather weird here, pls explain why are you doing it.
Actually I'd prefer if you'd use table declaration style for
creating IVRS table instead of AcpiAMDIOMMUIVRS/AcpiAMDIOMMUHardwareUnit
structures, that would simplify review greatly as it would be mostly
comparison with spec's tables.
You won't have to care about endianess, it also will help to avoid many
mistakes with usage of wrong cpu_to_le* functions as it's the most common
mistake in this patch.
something like this:
/* IVRS fields */
uint8_t *ivrs_tbl = acpi_data_push(table_data, sizeof(acpi_table_header)); /*
reserve space for header */
build_append_int_noprefix(table_data, AMD_IOMMU_HOST_ADDRESS_WIDTH << 8, 4); /*
IVinfo */
build_append_int_noprefix(table_data, 0, 8); /* Reserved */
/* build IVHD entry */
build_append_int_noprefix(table_data, 0x10 /* IVHD entry */, 1); /* Type */
build_append_int_noprefix(table_data, iommu->flags, 1); /* Flags */
build_append_int_noprefix(table_data, ivhd_len, 2); /* Length */
...
build_4byte_ivhd_device_entry(table_data,...); /* if it's only one entry no
need in separate function, just inline it */
...
build_header(linker, table_data, (void *), ivrs_tbl,
"IVRS", ivrs_tbl - table_data->data, 1 /* IVRS Revision */, NULL,
NULL);
> + /* device entries */
> + memset(iommu->dev_entries, 0, 20);
> + /* Add device flags here
> + * This is 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)
> + * 5.2.2.1
in linked pdf it's not 5.2.2 but
"5.3 IOMMU ACPI Table Definitions"
> + */
> + iommu->dev_entries[12] = 3;
> + iommu->dev_entries[16] = 4;
> + iommu->dev_entries[17] = 0xff;
> + iommu->dev_entries[18] = 0xff;
this totally unreadable, I can't tell which fields you are filling in
without doing math, see above table style declaration suggestion,
with comments matching spec along with it, it should be easy to verify entry vs
spec.
Also maybe use "all devices entry" (1, as per Table 75: IVHD device entry type
codes)
instead of declaring a range with help of several entries.
> + }
> +
> + build_header(linker, table_data, (void *)(table_data->data +
> iommu_start),
> + "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
> +}
> +
> +static iommu_type has_iommu(void)
s/has_iommu/iommu_type/
> +{
> + bool ambiguous;
> +
> + if (object_resolve_path_type("", TYPE_AMD_IOMMU_DEVICE, &ambiguous)
> + && !ambiguous)
you probably should assert on ambiguous == true
> + 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, GArray *linker, unsigned rsdt)
> {
> @@ -2646,16 +2725,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)
> {
> @@ -2668,6 +2737,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState
> *machine)
> AcpiMcfgInfo mcfg;
> PcPciInfo pci;
> uint8_t *u;
> + iommu_type type = has_iommu();
s/iommu_type/IOMMUType/
s/type/iommu_type/
> size_t aml_len = 0;
> GArray *tables_blob = tables->table_data;
> AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
> @@ -2733,7 +2803,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 (type == TYPE_AMD) {
> + acpi_add_table(table_offsets, tables_blob);
> + build_amd_iommu(tables_blob, tables->linker);
> + }
> +
> + if (type == 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 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)
some of above defines aren't used in this patch,
pls move them to patch where they are first used or drop them.
> +
> +/* 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;
this field shouldn't be here, per spec efr_register is the last before
dev_entries
> + uint8_t dev_entries[20];
> +} QEMU_PACKED;
> +typedef struct AcpiAMDIOMMUHardwareUnit AcpiAMDIOMMUHardwareUnit;
> +
> #endif
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index b024ffa..7e511e1 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -24,6 +24,7 @@
> #include "hw/qdev.h"
> #include "sysemu/dma.h"
>
> +#define INTEL_IOMMU_STR "intel"
not related to patch/series change?
> #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
> #define INTEL_IOMMU_DEVICE(obj) \
> OBJECT_CHECK(IntelIOMMUState, (obj), TYPE_INTEL_IOMMU_DEVICE)