Re: [Qemu-devel] [PATCH RFC v3 14/14] intel_iommu: enable vfio devices

[Peter Xu]

On Mon, Jan 16, 2017 at 05:54:55PM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月16日 17:18, Peter Xu wrote:
> >>>  static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t 
> >>> domain_id,
> >>>                                        hwaddr addr, uint8_t am)
> >>>  {
> >>>@@ -1222,6 +1251,7 @@ static void 
> >>>vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
> >>>      info.addr = addr;
> >>>      info.mask = ~((1 << am) - 1);
> >>>      g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, 
> >>> &info);
> >>>+    vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
> >>Is the case of GLOBAL or DSI flush missed, or we don't care it at all?
> >IMHO we don't. For device assignment, since we are having CM=1 here,
> >we should have explicit page invalidations even if guest sends
> >global/domain invalidations.
> >
> >Thanks,
> >
> >-- peterx
> 
> Is this spec required?

I think not. IMO the spec is very coarse grained on describing cache
mode...

> Btw, it looks to me that both DSI and GLOBAL are
> indeed explicit flush.

Actually when cache mode is on, it is unclear to me on how we should
treat domain/global invalidations, at least from the spec (as
mentioned earlier). My understanding is that they are not "explicit
flushes", which IMHO should only mean page selective IOTLB
invalidations.

> 
> Just have a quick go through on driver codes and find this something
> interesting in intel_iommu_flush_iotlb_psi():
> 
> ...
>     /*
>      * Fallback to domain selective flush if no PSI support or the size is
>      * too big.
>      * PSI requires page size to be 2 ^ x, and the base address is naturally
>      * aligned to the size
>      */
>     if (!cap_pgsel_inv(iommu->cap) || mask > cap_max_amask_val(iommu->cap))
>         iommu->flush.flush_iotlb(iommu, did, 0, 0,
>                         DMA_TLB_DSI_FLUSH);
>     else
>         iommu->flush.flush_iotlb(iommu, did, addr | ih, mask,
>                         DMA_TLB_PSI_FLUSH);
> ...

I think this is interesting... and I doubt its correctness while with
cache mode enabled.

If so (sending domain invalidation instead of a big range of page
invalidations), how should we capture which pages are unmapped in
emulated IOMMU?

> 
> It looks like DSI_FLUSH is possible even for CM on.
> 
> And in flush_unmaps():
> 
> ...
>         /* In caching mode, global flushes turn emulation expensive */
>         if (!cap_caching_mode(iommu->cap))
>             iommu->flush.flush_iotlb(iommu, 0, 0, 0,
>                      DMA_TLB_GLOBAL_FLUSH);
> ...
> 
> If I understand the comments correctly, GLOBAL is ok for CM too (though the
> code did not do it for performance reason).

I think it should be okay to send global flush for CM, but not sure
whether we should notify anything when we receive it. Hmm, anyway, I
think I need some more reading to make sure I understand the whole
thing correctly. :)

For example, when I see this commit:

commit 78d5f0f500e6ba8f6cfd0673475ff4d941d705a2
Author: Nadav Amit <address@hidden>
Date:   Thu Apr 8 23:00:41 2010 +0300

    intel-iommu: Avoid global flushes with caching mode.
    
    While it may be efficient on real hardware, emulation of global
    invalidations is very expensive as all shadow entries must be examined.
    This patch changes the behaviour when caching mode is enabled (which is
    the case when IOMMU emulation takes place). In this case, page specific
    invalidation is used instead.

Before I ask someone else besides qemu-devel, I am curious about
whether there is existing VT-d emulation code (outside QEMU, of
course) so that I can have a reference? Quick search didn't answer me.

Thanks,

-- peterx