Re: [PATCH v7 04/14] x86: intel-iommu: add vt-d init test

[Peter Xu <peterx@redhat.com>]

On Fri, Dec 02, 2016 at 08:50:17AM +0100, Alexander Gordeev wrote:

[...]

> > > > +	vtd_dump_init_info();
> > > 
> > > Should we check an iommu is there indeed? My environment
> > > returns all zeroes (which is wrong I guess) and attempts
> > > to proceed.
> > 
> > How about an assertion on the version? We just let it quit with error
> > if with a wrong version (in this case, version is all zeros).
> 
> Yep. I guess (!max && max >= min) should fit?

>From vt-d spec 10.4.1, "MAX" is "Major Version number", and "MIN" is
"Minor Version number". So looks like it is possible we have max=1 and
min>1 - minor version increases, while major version keeps. To make it
simpler, I'll use (max >= 1) directly.

> 
> > [...]
> > 
> > > > +static inline uint64_t vtd_readq(unsigned int reg)
> > > > +{
> > > > +	return __raw_readq(vtd_reg(reg));
> > > > +}
> > > 
> > > The accessors above should use ioremap'ped pointer, not direct
> > > access to Q35_HOST_BRIDGE_IOMMU_ADDR.
> > 
> > Hmm... This issue applies to EDU device register accesses as well.
> > Will fix.
> > 
> > Here I think I can use phys_to_virt() directly since x86_64 has
> > already mapped the first 4G memory as 1:1. However for EDU register
> > access I'd better use ioremap() since it might be used outside x86_64
> > in the future.
> 
> Actually, I think for x86_64 you need to ioremap as well. Because (a)
> accessing MMIO with no prior call to ioremap() is rather confusing
> and (b) we never know what machine this code might run in, let's say
> 10 years ;)

Ok. Let me fix. Thanks!

-- peterx