Re: [kvm-unit-tests PATCH 03/14] x86: intel-iommu: add vt-d init test

[Andrew Jones <drjones@redhat.com>]

On Fri, Oct 21, 2016 at 05:52:50PM +0800, Peter Xu wrote:
> 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.

You won't. You'll have the spec open reading them and then use them
in the code. Reviewers will also open the spec and then search for
them in the code. Nobody will memorize them, they'll just match them.

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

That comment sounds good to me :-)

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

Yeah, I have no idea bout the mask, /me didn't open the spec. Use
whatever is correct, maybe defining a new mask if this mask is not
always == PAGE_MASK, or even if it is, e.g.

 #define MY_MASK PAGE_MASK

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

I just mean if there are assumptions on how something should work,
then either it should be confirmed and asserted, or even tested
with an explicit try-report 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.)
> 
> 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,
drew