On Thu, Dec 01, 2016 at 02:14:53PM +0100, Alexander Gordeev wrote:
[...]
> > +static void vtd_gcmd_or(uint32_t cmd)
> > +{
> > + uint32_t status;
> > +
> > + /* We only allow set one bit for each time */
> > + assert(is_power_of_2(cmd));
> > +
> > + status = vtd_readl(DMAR_GSTS_REG);
> > + vtd_writel(DMAR_GCMD_REG, status | cmd);
> > +
> > + if (cmd & VTD_GCMD_ONE_SHOT_BITS) {
> > + /* One-shot bits are taking effect immediately */
> > + return;
> > + }
> > +
> > + /* Make sure IOMMU handled our command request */
> > + while (!(vtd_readl(DMAR_GSTS_REG) & cmd))
> > + cpu_relax();
>
> So I am hitting an endless loop here.
> Probably, an IO delay + counter would help.
Are you running with the following parameter appended?
-device intel-iommu,intremap=on,eim=off
If so, IMHO either QEMU should quit with error (for old QEMUs that
don't support "eim" property, or even without an Intel IOMMU), or it
should success the test if it's not that case.
>
> > +}
> > +
> > +static void vtd_dump_init_info(void)
> > +{
> > + printf("VT-d version: 0x%x\n", vtd_readl(DMAR_VER_REG));
> > + printf(" cap: 0x%016lx\n", vtd_readq(DMAR_CAP_REG));
> > + printf(" ecap: 0x%016lx\n", vtd_readq(DMAR_ECAP_REG));
> > +}
> > +
> > +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).
>
> > + vtd_writeq(DMAR_RTADDR_REG, virt_to_phys(root));
> > + vtd_gcmd_or(VTD_GCMD_ROOT);
> > + printf("DMAR table address: 0x%016lx\n", vtd_root_table());
> > +}
> > +
> > +static void vtd_setup_ir_table(void)
> > +{
> > + void *root = alloc_page();
> > +
> > + memset(root, 0, PAGE_SIZE);
>
> Same here.
Same here. I was intended to use zeros to init all interrupt remapping
table entries.
>
> > + /* 0xf stands for table size (2^(0xf+1) == 65536) */
> > + vtd_writeq(DMAR_IRTA_REG, virt_to_phys(root) | 0xf);
> > + vtd_gcmd_or(VTD_GCMD_IR_TABLE);
> > + printf("IR table address: 0x%016lx\n", vtd_ir_table());
> > +}
> > +
> > +void vtd_init(void)
> > +{
> > + setup_vm();
> > + smp_init();
>
> I would say these two are the generic env. setup, not VT-d
> setup. Thus, would better outside of this scope, as far as
> I am concerned.
Here I just want to make sure we have these things inited before hand,
because VT-d init codes needs these.
I have tried to post patches to allow these init functions be called
more than once, but Drew's opinion is that the caller should just
avoid calling it more than once. I think that's fair enough.
If user called these functions more than once, either:
- it'll have no side effect (e.g., smp_init())
- it'll assert() fail so user will know he/she did something wrong
(e.g., setup_vm())
So IMHO it does not hurt to have these two lines here.
>
> > +
> > + 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).
[...]
> > +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.
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
