Received-SPF: pass (google.com: domain of linux-kernel+bounces-140409-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <4449fe72-78c9-4c39-aa4c-29829de1a545@linux.intel.com>
Date: Thu, 11 Apr 2024 20:10:52 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, Joerg Roedel <joro@8bytes.org>,
 Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>,
 Kevin Tian <kevin.tian@intel.com>, Tina Zhang <tina.zhang@intel.com>,
 Yi Liu <yi.l.liu@intel.com>, iommu@lists.linux.dev,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/12] iommu/vt-d: Add cache tag assignment interface
To: Jason Gunthorpe <jgg@ziepe.ca>
References: <20240325021705.249769-1-baolu.lu@linux.intel.com>
 <20240325021705.249769-2-baolu.lu@linux.intel.com>
 <20240410154134.GG223006@ziepe.ca>
Content-Language: en-US
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <20240410154134.GG223006@ziepe.ca>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2024/4/10 23:41, Jason Gunthorpe wrote:
> 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@linux.intel.com>
>> ---
>>   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?

Yes. The device and dev_pasid lists appear to be duplicate. I am about
to remove these two lists later.

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

The queued invalidation interface is a bit complicated, especially when
it comes to device TLB invalidation. Device TLB invalidation might
result in a timeout, which requires special treatment.

>> +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

Sure.

> 
>> +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?

This is a partial replacement of below series.

https://lore.kernel.org/all/20240208082307.15759-1-yi.l.liu@intel.com/

Best regards,
baolu