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

[Peter Xu <peterx@redhat.com>]

On Fri, Oct 21, 2016 at 02:18:04PM +0200, Andrew Jones wrote:
> 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.

Ok.

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

I see that these registers already have comments, I think it'll be
good to make these lines exactly the same as original header in
Linux/QEMU source, so I didn't do that. :)

However, I'll remove all the useless wrappers and use direct accesses
to registers when necessary. 

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

Yeah this looks clean. Will use it.

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

I see. Maybe it'll be nicer I just re-write this function to not do
such an assumption. After all, it is not hard to add one more step to
check status.

Will fix. Thanks!

-- peterx