From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Subject: Re: [kvm-unit-tests PATCH 03/14] x86: intel-iommu: add vt-d init test Date: Thu, 20 Oct 2016 11:30:30 +0200 Message-ID: <20161020093030.bte5i65rfoszozal@kamzik.brq.redhat.com> References: <1476448852-30062-1-git-send-email-peterx@redhat.com> <1476448852-30062-4-git-send-email-peterx@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, jan.kiszka@web.de, agordeev@redhat.com, rkrcmar@redhat.com, pbonzini@redhat.com To: Peter Xu Return-path: Received: from mx1.redhat.com ([209.132.183.28]:41700 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755954AbcJTJag (ORCPT ); Thu, 20 Oct 2016 05:30:36 -0400 Content-Disposition: inline In-Reply-To: <1476448852-30062-4-git-send-email-peterx@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > --- > 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 , > + * > + * 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 , > + * > + * 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 , > + * > + * 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