On Fri, Oct 14, 2016 at 08:40:41PM +0800, Peter Xu wrote:
> Adding fundamental init test for Intel IOMMU. This includes basic
> initialization of Intel IOMMU device, like DMAR (DMA Remapping),
> IR (Interrupt Remapping), QI (Queue Invalidation), etc.
>
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
> lib/x86/intel-iommu.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++++++
> lib/x86/intel-iommu.h | 106 ++++++++++++++++++++++++++++++++++++++
> x86/Makefile.x86_64 | 2 +
> x86/intel-iommu.c | 41 +++++++++++++++
> 4 files changed, 287 insertions(+)
> create mode 100644 lib/x86/intel-iommu.c
> create mode 100644 lib/x86/intel-iommu.h
> create mode 100644 x86/intel-iommu.c
>
> diff --git a/lib/x86/intel-iommu.c b/lib/x86/intel-iommu.c
> new file mode 100644
> index 0000000..9f55394
> --- /dev/null
> +++ b/lib/x86/intel-iommu.c
> @@ -0,0 +1,138 @@
> +/*
> + * Intel IOMMU APIs
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * Authors:
> + * Peter Xu <peterx@xxxxxxxxxx>,
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + */
> +
> +#include "intel-iommu.h"
> +
> +void vtd_reg_write_4(unsigned int reg, uint32_t value)
> +{
> + *(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> +}
> +
> +void vtd_reg_write_8(unsigned int reg, uint64_t value)
> +{
> + *(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> +}
> +
> +uint32_t vtd_reg_read_4(unsigned int reg)
> +{
> + return *(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> +}
> +
> +uint64_t vtd_reg_read_8(unsigned int reg)
> +{
> + return *(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> +}
The above could be static inlines in intel-iommu.h. How about
calling them vtd_readl, vtd_readq, vtd_writel, vtd_writeq? That
would make them more consistent with the readl, readq... in
lib/asm-generic/io.h
> +
> +uint32_t vtd_version(void)
> +{
> + return vtd_reg_read_4(DMAR_VER_REG);
> +}
> +
> +uint64_t vtd_cap(void)
> +{
> + return vtd_reg_read_8(DMAR_CAP_REG);
> +}
> +
> +uint64_t vtd_ecap(void)
> +{
> + return vtd_reg_read_8(DMAR_ECAP_REG);
> +}
> +
> +uint32_t vtd_status(void)
> +{
> + return vtd_reg_read_4(DMAR_GSTS_REG);
> +}
> +
> +uint32_t vtd_fault_status(void)
> +{
> + return vtd_reg_read_4(DMAR_FSTS_REG);
> +}
> +
> +uint64_t vtd_root_table(void)
> +{
> + /* No extend root table support yet */
> + return vtd_reg_read_8(DMAR_RTADDR_REG);
> +}
I'd drop all the above and use vtd_readl/q(DMAR_CAP_REG) directly,
the wrappers don't add anything and take the unit test writer
further away from the register names that they should be more
familiar with.
> +
> +uint64_t vtd_ir_table(void)
> +{
> + return vtd_reg_read_8(DMAR_IRTA_REG) & 0xfffffffffffff000;
> +}
This one has a mask, so makes sense to keep the wrapper. But the
mask looks like PAGE_MASK to me. Why not use that? Or even drop
this wrapper and just use PAGE_MASK when necessary directly...
> +
> +void vtd_gcmd_or(uint32_t cmd)
> +{
> + uint32_t status = vtd_status();
> + /*
> + * Logically, we need to read DMAR_GSTS_REG until IOMMU
> + * handles the write request. However for QEMU/KVM, this write
> + * is always sync, so when this returns, we should be sure
should always be in sync. Sounds like something to unit test :-)
> + * that the GCMD write is done.
> + */
> + vtd_reg_write_4(DMAR_GCMD_REG, status | cmd);
> +}
> +
> +void vtd_dump_init_info(void)
> +{
> + printf("VT-d version: 0x%x\n", vtd_version());
> + printf(" cap: 0x%016lx\n", vtd_cap());
> + printf(" ecap: 0x%016lx\n", vtd_ecap());
> +}
> +
> +void vtd_enable_qi(void)
> +{
> + vtd_gcmd_or(VTD_GCMD_QI);
> +}
I'd drop this wrapper and use a comment where it's needed like
vtd_gcmd_or(VTD_GCMD_QI); /* enable QI */
(By now you see a theme. Keep wrappers to a minimal in order
to keep the unit test writer closer to the spec.)
> +
> +void vtd_setup_root_table(void)
> +{
> + void *root = alloc_page();
> +
> + memset(root, 0, PAGE_SIZE);
> + vtd_reg_write_8(DMAR_RTADDR_REG, virt_to_phys(root));
> + vtd_gcmd_or(VTD_GCMD_ROOT);
> + printf("DMAR table address: 0x%016lx\n", vtd_root_table());
> +}
> +
> +void vtd_setup_ir_table(void)
> +{
> + void *root = alloc_page();
> +
> + memset(root, 0, PAGE_SIZE);
> + vtd_reg_write_8(DMAR_IRTA_REG, virt_to_phys(root));
> + /* 0xf stands for table size (2^(0xf+1) == 65536) */
> + vtd_gcmd_or(VTD_GCMD_IR_TABLE | 0xf);
> + printf("IR table address: 0x%016lx\n", vtd_ir_table());
> +}
> +
> +void vtd_enable_dmar(void)
> +{
> + vtd_gcmd_or(VTD_GCMD_DMAR);
> +}
> +
> +void vtd_enable_ir(void)
> +{
> + vtd_gcmd_or(VTD_GCMD_IR);
> +}
More wrappers to drop.
> +
> +void vtd_init(void)
> +{
> + setup_vm();
> + smp_init();
> + setup_idt();
> +
> + vtd_dump_init_info();
> + vtd_enable_qi();
> + vtd_setup_root_table();
> + vtd_setup_ir_table();
> + vtd_enable_dmar();
> + vtd_enable_ir();
I'd open code all the above here. The functions aren't really
gaining us anything that comments can't do for us.
> +}
> diff --git a/lib/x86/intel-iommu.h b/lib/x86/intel-iommu.h
> new file mode 100644
> index 0000000..0f687d1
> --- /dev/null
> +++ b/lib/x86/intel-iommu.h
> @@ -0,0 +1,106 @@
> +/*
> + * Intel IOMMU header
Please point out the kernel source these defines come from,
include/linux/intel-iommu.h
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * Authors:
> + * Peter Xu <peterx@xxxxxxxxxx>,
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + */
> +
> +#ifndef __INTEL_IOMMU_H__
> +#define __INTEL_IOMMU_H__
> +
> +#include "libcflat.h"
> +#include "vm.h"
> +#include "isr.h"
> +#include "smp.h"
> +#include "desc.h"
> +
> +#define Q35_HOST_BRIDGE_IOMMU_ADDR 0xfed90000ULL
> +
> +/*
> + * Intel IOMMU register specification
> + */
> +#define DMAR_VER_REG 0x0 /* Arch version supported by this IOMMU */
> +#define DMAR_CAP_REG 0x8 /* Hardware supported capabilities */
> +#define DMAR_CAP_REG_HI 0xc /* High 32-bit of DMAR_CAP_REG */
> +#define DMAR_ECAP_REG 0x10 /* Extended capabilities supported */
> +#define DMAR_ECAP_REG_HI 0X14
> +#define DMAR_GCMD_REG 0x18 /* Global command */
> +#define DMAR_GSTS_REG 0x1c /* Global status */
> +#define DMAR_RTADDR_REG 0x20 /* Root entry table */
> +#define DMAR_RTADDR_REG_HI 0X24
> +#define DMAR_CCMD_REG 0x28 /* Context command */
> +#define DMAR_CCMD_REG_HI 0x2c
> +#define DMAR_FSTS_REG 0x34 /* Fault status */
> +#define DMAR_FECTL_REG 0x38 /* Fault control */
> +#define DMAR_FEDATA_REG 0x3c /* Fault event interrupt data */
> +#define DMAR_FEADDR_REG 0x40 /* Fault event interrupt addr */
> +#define DMAR_FEUADDR_REG 0x44 /* Upper address */
> +#define DMAR_AFLOG_REG 0x58 /* Advanced fault control */
> +#define DMAR_AFLOG_REG_HI 0X5c
> +#define DMAR_PMEN_REG 0x64 /* Enable protected memory region */
> +#define DMAR_PLMBASE_REG 0x68 /* PMRR low addr */
> +#define DMAR_PLMLIMIT_REG 0x6c /* PMRR low limit */
> +#define DMAR_PHMBASE_REG 0x70 /* PMRR high base addr */
> +#define DMAR_PHMBASE_REG_HI 0X74
> +#define DMAR_PHMLIMIT_REG 0x78 /* PMRR high limit */
> +#define DMAR_PHMLIMIT_REG_HI 0x7c
> +#define DMAR_IQH_REG 0x80 /* Invalidation queue head */
> +#define DMAR_IQH_REG_HI 0X84
> +#define DMAR_IQT_REG 0x88 /* Invalidation queue tail */
> +#define DMAR_IQT_REG_HI 0X8c
> +#define DMAR_IQA_REG 0x90 /* Invalidation queue addr */
> +#define DMAR_IQA_REG_HI 0x94
> +#define DMAR_ICS_REG 0x9c /* Invalidation complete status */
> +#define DMAR_IRTA_REG 0xb8 /* Interrupt remapping table addr */
> +#define DMAR_IRTA_REG_HI 0xbc
> +#define DMAR_IECTL_REG 0xa0 /* Invalidation event control */
> +#define DMAR_IEDATA_REG 0xa4 /* Invalidation event data */
> +#define DMAR_IEADDR_REG 0xa8 /* Invalidation event address */
> +#define DMAR_IEUADDR_REG 0xac /* Invalidation event address */
> +#define DMAR_PQH_REG 0xc0 /* Page request queue head */
> +#define DMAR_PQH_REG_HI 0xc4
> +#define DMAR_PQT_REG 0xc8 /* Page request queue tail*/
> +#define DMAR_PQT_REG_HI 0xcc
> +#define DMAR_PQA_REG 0xd0 /* Page request queue address */
> +#define DMAR_PQA_REG_HI 0xd4
> +#define DMAR_PRS_REG 0xdc /* Page request status */
> +#define DMAR_PECTL_REG 0xe0 /* Page request event control */
> +#define DMAR_PEDATA_REG 0xe4 /* Page request event data */
> +#define DMAR_PEADDR_REG 0xe8 /* Page request event address */
> +#define DMAR_PEUADDR_REG 0xec /* Page event upper address */
> +#define DMAR_MTRRCAP_REG 0x100 /* MTRR capability */
> +#define DMAR_MTRRCAP_REG_HI 0x104
> +#define DMAR_MTRRDEF_REG 0x108 /* MTRR default type */
> +#define DMAR_MTRRDEF_REG_HI 0x10c
> +
> +#define VTD_GCMD_IR_TABLE (0x1000000)
> +#define VTD_GCMD_IR (0x2000000)
> +#define VTD_GCMD_QI (0x4000000)
> +#define VTD_GCMD_ROOT (0x40000000)
> +#define VTD_GCMD_DMAR (0x80000000)
> +
> +void vtd_reg_write_4(unsigned int reg, uint32_t value);
> +void vtd_reg_write_8(unsigned int reg, uint64_t value);
> +uint32_t vtd_reg_read_4(unsigned int reg);
> +uint64_t vtd_reg_read_8(unsigned int reg);
> +uint32_t vtd_version(void);
> +uint64_t vtd_cap(void);
> +uint64_t vtd_ecap(void);
> +uint32_t vtd_status(void);
> +uint32_t vtd_fault_status(void);
> +uint64_t vtd_root_table(void);
> +uint64_t vtd_ir_table(void);
> +void vtd_dump_init_info(void);
> +void vtd_enable_qi(void);
> +void vtd_setup_root_table(void);
> +void vtd_setup_ir_table(void);
> +void vtd_enable_dmar(void);
> +void vtd_enable_ir(void);
> +void vtd_init(void);
> +
> +#endif
> diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
> index e166911..a8e9445 100644
> --- a/x86/Makefile.x86_64
> +++ b/x86/Makefile.x86_64
> @@ -4,6 +4,7 @@ ldarch = elf64-x86-64
> CFLAGS += -mno-red-zone
>
> cflatobjs += lib/x86/setjmp64.o
> +cflatobjs += lib/x86/intel-iommu.o
>
> tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
> $(TEST_DIR)/emulator.flat $(TEST_DIR)/idt_test.flat \
> @@ -14,6 +15,7 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
> tests += $(TEST_DIR)/svm.flat
> tests += $(TEST_DIR)/vmx.flat
> tests += $(TEST_DIR)/tscdeadline_latency.flat
> +tests += $(TEST_DIR)/intel-iommu.flat
>
> include $(TEST_DIR)/Makefile.common
>
> diff --git a/x86/intel-iommu.c b/x86/intel-iommu.c
> new file mode 100644
> index 0000000..60d512a
> --- /dev/null
> +++ b/x86/intel-iommu.c
> @@ -0,0 +1,41 @@
> +/*
> + * Intel IOMMU unit test.
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * Authors:
> + * Peter Xu <peterx@xxxxxxxxxx>,
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + */
> +
> +#include "intel-iommu.h"
> +
> +int main(int argc, char *argv[])
> +{
> + setup_vm();
> + smp_init();
> + setup_idt();
> +
> + vtd_dump_init_info();
> + report("init status check", vtd_status() == 0);
> + report("fault status check", vtd_fault_status() == 0);
> +
> + vtd_enable_qi();
> + report("QI enablement", vtd_status() | VTD_GCMD_QI);
> +
> + vtd_setup_root_table();
> + report("DMAR table setup", vtd_status() | VTD_GCMD_ROOT);
> +
> + vtd_setup_ir_table();
> + report("IR table setup", vtd_status() | VTD_GCMD_IR_TABLE);
The above three report() calls will always pass since you're using OR.
> +
> + vtd_enable_dmar();
> + report("DMAR enablement", vtd_status() & VTD_GCMD_DMAR);
> +
> + vtd_enable_ir();
> + report("IR enablement", vtd_status() & VTD_GCMD_IR);
You've reproduced vtd_init() here. Why not just call it and then
do a sequence of report() calls where you check each register
has the expected value? I.e.
vtd_init();
status = vtd_readl(DMAR_GSTS_REG);
report("QI enablement", status & VTD_GCMD_QI);
report("DMAR table setup", status & VTD_GCMD_ROOT);
...
Would that not work with this device?
> +
> + return report_summary();
> +}
> --
> 2.7.4
>
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
