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
--
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
