Received-SPF: pass (google.com: domain of linux-kernel+bounces-199899-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <7440cd36-41e0-4705-a597-f2ce3f841a70@linux.intel.com>
Date: Tue, 4 Jun 2024 09:14:42 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, "iommu@lists.linux.dev"
 <iommu@lists.linux.dev>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] iommu/vt-d: Support batching IOTLB/dev-IOTLB
 invalidation commands
To: "Zhang, Tina" <tina.zhang@intel.com>, "Tian, Kevin" <kevin.tian@intel.com>
References: <20240517003728.251115-1-tina.zhang@intel.com>
 <20240517003728.251115-2-tina.zhang@intel.com>
 <2b390228-190c-4508-b98f-1811c54c9e5c@linux.intel.com>
 <MW5PR11MB588165376224FEA74A0426D989FF2@MW5PR11MB5881.namprd11.prod.outlook.com>
Content-Language: en-US
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <MW5PR11MB588165376224FEA74A0426D989FF2@MW5PR11MB5881.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 6/3/24 3:37 PM, Zhang, Tina wrote:
>> -----Original Message-----
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Sunday, May 19, 2024 5:43 PM
>> To: Zhang, Tina<tina.zhang@intel.com>; Tian, Kevin<kevin.tian@intel.com>
>> Cc:baolu.lu@linux.intel.com;iommu@lists.linux.dev; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH 1/2] iommu/vt-d: Support batching IOTLB/dev-IOTLB
>> invalidation commands
>>
>> On 5/17/24 8:37 AM, Tina Zhang wrote:
>>> Introduce a new parameter batch_desc to the QI based IOTLB/dev-IOTLB
>>> invalidation operations to support batching invalidation descriptors.
>>> This batch_desc is a pointer to the descriptor entry in a batch cmds
>>> buffer. If the batch_desc is NULL, it indicates that batch submission
>>> is not being used, and descriptors will be submitted individually.
>>>
>>> Also fix an issue reported by checkpatch about "unsigned mask":
>>>           "Prefer 'unsigned int' to bare use of 'unsigned'"
>>>
>>> Signed-off-by: Tina Zhang<tina.zhang@intel.com>
>>> ---
>>>    drivers/iommu/intel/cache.c | 33 +++++++++++-------
>>>    drivers/iommu/intel/dmar.c  | 67 ++++++++++++++++++++-----------------
>>>    drivers/iommu/intel/iommu.c | 27 +++++++++------
>>>    drivers/iommu/intel/iommu.h | 21 ++++++++----
>>>    drivers/iommu/intel/pasid.c | 20 ++++++-----
>>>    5 files changed, 100 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
>>> index e8418cdd8331..dcf5e0e6af17 100644
>>> --- a/drivers/iommu/intel/cache.c
>>> +++ b/drivers/iommu/intel/cache.c
>>> @@ -278,7 +278,7 @@ void cache_tag_flush_range(struct dmar_domain
>> *domain, unsigned long start,
>>>    		case CACHE_TAG_NESTING_IOTLB:
>>>    			if (domain->use_first_level) {
>>>    				qi_flush_piotlb(iommu, tag->domain_id,
>>> -						tag->pasid, addr, pages, ih);
>>> +						tag->pasid, addr, pages, ih,
>> NULL);
>>>    			} else {
>> I'd like to have all batched descriptors code inside this file to make it easier for
>> maintenance. Perhaps we can add the below infrastructure in the
>> dmar_domain structure together with the cache tag.
> Does it suggest we need to add a batch version of qi_flush_iotlb/qi_flush_dev_iotlb/qi_flush_piotlb/qi_flush_dev_iotlb_pasid() in the cache.c file? It doesn't sound like an easy to maintain those functions, does it?

Yes. I don't think it's that difficult as the helpers just compose a qi
descriptor and insert it in a local queue. This local queue will be
flushed after finishing iterating all cache tags, or there's no room for
more descriptors, or switches to a different iommu. Have I missed
anything?

> 
> In this patch, we reuse the current qi_flush_xxx() for both batching and non-batching processing, so that we don't need to duplicate the logic of qi_flush_xxx() in two places with one for batching processing and the other one for non-batching processing. What do you think?

I don't like to put different things in a single helper. They share some
code, for example, composing the descriptor's dw words. Others are just
different. You can move that code into a static inline if you would
like, but the batching and non-batching processing should be in
different helpers.

Best regards,
baolu