On Fri, Dec 02, 2016 at 11:12:34AM +0800, Peter Xu wrote:
> > > +static void vtd_setup_root_table(void)
> > > +{
> > > + void *root = alloc_page();
> > > +
> > > + memset(root, 0, PAGE_SIZE);
> >
> > As it is a placeholder at this stage I would suggest less
> > meaningful fill pattern, i.e. all 1s or 0xdeadbeef may be.
>
> I was intended to have it all zero here. The data is root entries on
> the page, and I need to make sure they are invalid root entries (e.g.,
> "reserved" bits should be zeroed, and "present" bit should be
> cleared).
I see. Thanks!
[...]
> > > + 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?
> [...]
>
> > > +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 ;)
> 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
