Re: [RFC v2 13/22] intel_iommu: add PASID cache management infrastructure

[Peter Xu]

On Thu, Oct 24, 2019 at 08:34:34AM -0400, Liu Yi L wrote:
> This patch adds a PASID cache management infrastructure based on
> new added structure VTDPASIDAddressSpace, which is used to track
> the PASID usage and future PASID tagged DMA address translation
> support in vIOMMU.
> 
>     struct VTDPASIDAddressSpace {
>         VTDBus *vtd_bus;
>         uint8_t devfn;
>         AddressSpace as;
>         uint32_t pasid;
>         IntelIOMMUState *iommu_state;
>         VTDContextCacheEntry context_cache_entry;
>         QLIST_ENTRY(VTDPASIDAddressSpace) next;
>         VTDPASIDCacheEntry pasid_cache_entry;
>     };
> 
> Ideally, a VTDPASIDAddressSpace instance is created when a PASID
> is bound with a DMA AddressSpace. Intel VT-d spec requires guest
> software to issue pasid cache invalidation when bind or unbind a
> pasid with an address space under caching-mode. However, as
> VTDPASIDAddressSpace instances also act as pasid cache in this
> implementation, its creation also happens during vIOMMU PASID
> tagged DMA translation. The creation in this path will not be
> added in this patch since no PASID-capable emulated devices for
> now.

So is this patch an incomplete version even for the pasid caching
layer for emulated device?

IMHO it would be considered as acceptable to merge something that is
even not ready from hardware pov but at least from software pov it is
complete (so when the hardware is ready we should logically run the
binary directly on them, bugs can happen but that's another story).
However if for this case:

  - it's not even complete as is (in translation functions it seems
    that we don't ever use this cache layer at all),

  - we don't have emulated device supported for pasid yet at all, so
    even further to have this code start to make any sense, and,

  - this is a 400 line patch as standalone :)  Which means that we
    need to start maintain these 400 LOC starting from the day when it
    gets merged, while it's far from even being tested.  Then I don't
    see how to maintain...

With above, I would suggest you put this patch into the future
patchset where you would like to have the first emulated device for
pasid and then you can even test this patch with those ones.  What do
you think?

Thanks,

-- 
Peter Xu