From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [kvm-unit-tests PATCH 03/14] x86: intel-iommu: add vt-d init test Date: Fri, 21 Oct 2016 17:52:50 +0800 Message-ID: <20161021095250.GQ15168@pxdev.xzpeter.org> References: <1476448852-30062-1-git-send-email-peterx@redhat.com> <1476448852-30062-4-git-send-email-peterx@redhat.com> <20161020093030.bte5i65rfoszozal@kamzik.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: kvm@vger.kernel.org, jan.kiszka@web.de, agordeev@redhat.com, rkrcmar@redhat.com, pbonzini@redhat.com To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51290 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751175AbcJUJw4 (ORCPT ); Fri, 21 Oct 2016 05:52:56 -0400 Content-Disposition: inline In-Reply-To: <20161020093030.bte5i65rfoszozal@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Oct 20, 2016 at 11:30:30AM +0200, Andrew Jones wrote: [...] > > +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 Sure! Will fix. > > > + > > +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. Here I used wrapper since I don't want to remember all the register names, and (especially) I don't want to remember the size of the registers. For example, when I want to read ECAP register, calling vtd_ecap() will let me make sure the return value is 8 bytes (compiler will help to make sure of it, and I can simply know it from seeing the return type of the wrapper function, which is uint64_t), and I don't need to bother whether I should use vtd_readl() or vtd_readq() for it. If I don't have vtd_ecap() wrapper, I can't see the length from the register name only, and I need to check the spec every time, or with comment like: #define DMAR_ECAP_REG XXX /* 8 bytes */ In that case, I'd prefer with a wrapper for the registers (maybe I can move the functions into header files as well, with inline static). > > > + > > +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... The last 12 bits have other means (I didn't check it here, it's explained in chap 10.4.29 in vt-d spec in case anyone is interested), so it's just coincident that it looks like a page mask. However, I think I can use PAGE_MASK here. > > > + > > +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 :-) I am not sure whether I got the point... but real hardware should be async, so I don't know whether the code can work on real hardware. For QEMU, it is sync, so it's safe for kvm-unit-tests. > > > + * 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.) Yeah. Besides the case that I mentioned above (which I am still not quite sure), I agree that I can use register names directly in most cases. Will fix. > > > + > > +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. Ok. > > > + > > +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. Will fix. > > > +} > > 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 Will add. [...] > > +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. Oops... Thanks. :) > > > + > > + 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? I think it should work. Will take your advice. Thanks! -- peterx