Re: [PATCH 01/12] iommu/vt-d: Add cache tag assignment interface

[Jason Gunthorpe]

On Mon, Mar 25, 2024 at 10:16:54AM +0800, Lu Baolu wrote:
> Caching tag is a combination of tags used by the hardware to cache various
> translations. Whenever a mapping in a domain is changed, the IOMMU driver
> should invalidate the caches with the caching tags. The VT-d specification
> describes caching tags in section 6.2.1, Tagging of Cached Translations.
> 
> Add interface to assign caching tags to an IOMMU domain when attached to a
> RID or PASID, and unassign caching tags when a domain is detached from a
> RID or PASID. All caching tags are listed in the per-domain tag list and
> are protected by a dedicated lock.
> 
> In addition to the basic IOTLB and devTLB caching tag types, PARENT_IOTLB
> and PARENT_DEVTLB tag types are also introduced. These tags are used for
> caches that store translations for DMA accesses through a nested user
> domain. They are affected by changes to mappings in the parent domain.
> 
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
>  drivers/iommu/intel/iommu.h  |  25 +++++
>  drivers/iommu/intel/cache.c  | 192 +++++++++++++++++++++++++++++++++++
>  drivers/iommu/intel/iommu.c  |  31 +++++-
>  drivers/iommu/intel/nested.c |  21 +++-
>  drivers/iommu/intel/svm.c    |  12 ++-
>  drivers/iommu/intel/Makefile |   2 +-
>  6 files changed, 274 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/iommu/intel/cache.c
> 
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 404d2476a877..e3723b7a0b31 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -607,6 +607,9 @@ struct dmar_domain {
>  	struct list_head devices;	/* all devices' list */
>  	struct list_head dev_pasids;	/* all attached pasids */
>  
> +	spinlock_t cache_lock;		/* Protect the cache tag list */
> +	struct list_head cache_tags;	/* Cache tag list */

That is quite a neat trick - though building a dedicated invalidation
list duplicates data stored in the attached devices list?

You didn't try to make it RCU safe for invalidation?

> +struct cache_tag {
> +	struct list_head node;
> +	enum cache_tag_type type;
> +	struct intel_iommu *iommu;
> +	struct device *dev;

iommu and dev probably don't both need to be stored together. We have
iommu_get_iommu_dev() now.. I suppose this is probably a union of the
two pointers depending on tag. DEVTLB needs the dev and IOTLB needs
the iommu.

> +	u16 domain_id;
> +	ioasid_t pasid;
> +	int users;

unsigned int

> +static int __cache_tag_assign_parent_domain(struct dmar_domain *domain, u16 did,
> +					    struct device *dev, ioasid_t pasid)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	int ret;
> +
> +	ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_TYPE_PARENT_IOTLB);
> +	if (ret || !info->ats_enabled)
> +		return ret;

I'm not sure I understood the point of PARENT_IOTLB? I didn't see any
different implementation?

Isn't this backwards though? Each domain should have a list of things
to invalidate if the domain itself changes.

So the nesting parent should have a list of CHILD_DEVTLB's that need
cleaning. That list is changed when the nesting domains are attached
to something.

And a list of CHILD_IOTLBs, but the HW doesn't seem to need that?

Jason