Received-SPF: pass (google.com: domain of linux-kernel+bounces-175420-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 10 May 2024 16:03:04 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>
CC: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
	<alex.williamson@redhat.com>, <kevin.tian@intel.com>,
	<iommu@lists.linux.dev>, <pbonzini@redhat.com>, <seanjc@google.com>,
	<dave.hansen@linux.intel.com>, <luto@kernel.org>, <peterz@infradead.org>,
	<tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>, <hpa@zytor.com>,
	<corbet@lwn.net>, <joro@8bytes.org>, <will@kernel.org>,
	<robin.murphy@arm.com>, <baolu.lu@linux.intel.com>, <yi.l.liu@intel.com>
Subject: Re: [PATCH 5/5] iommufd: Flush CPU caches on DMA pages in
 non-coherent domains
Message-ID: <Zj3UuHQe4XgdDmDs@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20240507061802.20184-1-yan.y.zhao@intel.com>
 <20240507062212.20535-1-yan.y.zhao@intel.com>
 <20240509141332.GP4650@nvidia.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240509141332.GP4650@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?AygMMnpbRUcfHvNuoWbGJak5FhQu95NcX75bqurEKq4e6qgb/lLgrCTyarDS?=
 =?us-ascii?Q?NwD9QfpDxKs3rhwuH1uJxY98HunMmLStzLc4NTAPjW4w602RGu5p14EQa4VF?=
 =?us-ascii?Q?qTZD2ZeeE4mW985HyBTx/Hv0SLVtVpiXRnWRM5gwsigo/RZen5dgQlgTabWS?=
 =?us-ascii?Q?4x/DCNYFGsz8ZD56twXNDpqJFmo0jmiqLMIscDvfou/CNslBr4KdXRg87wHi?=
 =?us-ascii?Q?TiSN0dH/OcEs4WsPW09d19HfwpdR29Xki6rbqmdDozLIQmQFb9lSA0qDyYeu?=
 =?us-ascii?Q?ia0OnqSygMpVgMvdVBTP4NCqJK+CLuGXCoCNwbXMXhc8PwEtNRiTjTRcMyXs?=
 =?us-ascii?Q?2ypUbUuOSIvzQmec7TaHEKSU9JXJTgDOU71husXg7qPKL9ylOeKRb31RGjDq?=
 =?us-ascii?Q?vG0ntyNuERS3fvpQeEPd0AKGzBA2HIvZ0ZXxvC9bOoAjQBJs8av+3MOhKp7K?=
 =?us-ascii?Q?Ef8CXuF4Jw8FGVpiTYMjMBAse1tJ+dET1d5/LpivLPcsvSroi78DPeMK5lqm?=
 =?us-ascii?Q?ZcMbCppUgofRF1DC0w/ZaYD2XnefBWRXS+9b8Utk8nZByTVQd1ELo/aHcDy+?=
 =?us-ascii?Q?t/09KcYpMwhK1OcVXBBWIyZV5V3C1eiByM9nLclYkVHldMSsTT1g47WDzKM1?=
 =?us-ascii?Q?aDRjFSEeJG7Lif8fWCYAye/C929LX+LuLB+FFn+AZwGF6CHT0gQb53siQ3Gp?=
 =?us-ascii?Q?YlaKI7CiLJHFEBkM2cvYaSgYWjBAyTZ87v16EipWnbTWY/UFph/lVlRjPNi2?=
 =?us-ascii?Q?Fg84VVo0dUNY0yqDEzAxxCUf+OsvjlbpKMO+YD+ffZgO0UBR4QJLHwv5Tk//?=
 =?us-ascii?Q?JWr973YPrRTlKZ7ldUtf6yiXqCHlAYlIEl8gkQi2GTpfKTlKf24jyVQmev7J?=
 =?us-ascii?Q?pruhU0KsevewiYzQGMyXa0Qy0+Coxdec9xpdTQLEWDuxRhLmMSR6YUkT5XtP?=
 =?us-ascii?Q?NlJQJjgkPBVgQuf1cg4C4L5K9c5rgLjgjOiwM3dwX5i/nqeDbITtdfQbxt7x?=
 =?us-ascii?Q?B+pZxgCDBRp0KDSq4t79ce3Ncy/fjVuQgto0Zc4RnuKjfnEl2zVrw6mWBurU?=
 =?us-ascii?Q?uSCRd5txmWzroKEl2odjNSp0GXpvfaKJWRNBs/im+/fRivmaDJao76l+l554?=
 =?us-ascii?Q?NQzUuoSWOvrHHIUEeI1Da/6ra8u5neir1X3PoHJWPBARbT8cUqkDbEKyFCVm?=
 =?us-ascii?Q?O5FKlUIQiEHWZ2vOY4PTf7hnp7Qe5kDa2XyS9z5tnV+6s4MDGIztaRVlW31z?=
 =?us-ascii?Q?v3buw7mooi4XmuzWyRijaFH2K6UOxre5w2/ya4RH5YUdYM9qk4L7i5OtiUJz?=
 =?us-ascii?Q?z9s1TE/1uL4+WZn2O7ZRGmxePRMKTcwn294+onDEgYDqX2BzR2Oza5ZUAE2f?=
 =?us-ascii?Q?Wg2sHUhXRKvShQ5jioLBwr6oxMjkjZ2vw+hSbFfyTv4mmu7Y30KSH40SRsos?=
 =?us-ascii?Q?4gbCF4yPzgUHoSitnVpJn7CMYl8G1bOuOiDzjxbKegO9z7WA5tT3m67lHZEn?=
 =?us-ascii?Q?u7qc5pqT5KkICBs2Y548aHdtV/PZT+HuSM6RG+uwlOn3Syf36rUB27NobAr7?=
 =?us-ascii?Q?J6/Zs6X7dlJ56kwmTDf9uyHj1E62CP7TgWhaEs70?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4ecbc227-62c4-47fe-522a-08dc70c7bac5
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2024 08:03:51.5455
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1N7o0u+zhaTZxP+oNUfxBDs+0ARwkzoI9SF3H7wyhR/tHc0SFTozeY176LDfOpjEhL0oVyPN0elFbU5RByxJaQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB8167
X-OriginatorOrg: intel.com

On Thu, May 09, 2024 at 11:13:32AM -0300, Jason Gunthorpe wrote:
> On Tue, May 07, 2024 at 02:22:12PM +0800, Yan Zhao wrote:
> > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> > index 33d142f8057d..e3099d732c5c 100644
> > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > @@ -14,12 +14,18 @@ void iommufd_hwpt_paging_destroy(struct iommufd_object *obj)
> >  		container_of(obj, struct iommufd_hwpt_paging, common.obj);
> >  
> >  	if (!list_empty(&hwpt_paging->hwpt_item)) {
> > +		struct io_pagetable *iopt = &hwpt_paging->ioas->iopt;
> >  		mutex_lock(&hwpt_paging->ioas->mutex);
> >  		list_del(&hwpt_paging->hwpt_item);
> >  		mutex_unlock(&hwpt_paging->ioas->mutex);
> >  
> > -		iopt_table_remove_domain(&hwpt_paging->ioas->iopt,
> > -					 hwpt_paging->common.domain);
> > +		iopt_table_remove_domain(iopt, hwpt_paging->common.domain);
> > +
> > +		if (!hwpt_paging->enforce_cache_coherency) {
> > +			down_write(&iopt->domains_rwsem);
> > +			iopt->noncoherent_domain_cnt--;
> > +			up_write(&iopt->domains_rwsem);
> 
> I think it would be nicer to put this in iopt_table_remove_domain()
> since we already have the lock there anyhow. It would be OK to pass
> int he hwpt. Same remark for the incr side
Ok. Passed hwpt to the two funcions.

int iopt_table_add_domain(struct io_pagetable *iopt,
                          struct iommufd_hw_pagetable *hwpt);

void iopt_table_remove_domain(struct io_pagetable *iopt,
                              struct iommufd_hw_pagetable *hwpt);

> 
> > @@ -176,6 +182,12 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
> >  			goto out_abort;
> >  	}
> >  
> > +	if (!hwpt_paging->enforce_cache_coherency) {
> > +		down_write(&ioas->iopt.domains_rwsem);
> > +		ioas->iopt.noncoherent_domain_cnt++;
> > +		up_write(&ioas->iopt.domains_rwsem);
> > +	}
> > +
> >  	rc = iopt_table_add_domain(&ioas->iopt, hwpt->domain);
> 
> iopt_table_add_domain also already gets the required locks too
Right.

> 
> >  	if (rc)
> >  		goto out_detach;
> > @@ -183,6 +195,9 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
> >  	return hwpt_paging;
> >  
> >  out_detach:
> > +	down_write(&ioas->iopt.domains_rwsem);
> > +	ioas->iopt.noncoherent_domain_cnt--;
> > +	up_write(&ioas->iopt.domains_rwsem);
> 
> And then you don't need this error unwind
Yes :)

> > diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
> > index 0ec3509b7e33..557da8fb83d9 100644
> > --- a/drivers/iommu/iommufd/io_pagetable.h
> > +++ b/drivers/iommu/iommufd/io_pagetable.h
> > @@ -198,6 +198,11 @@ struct iopt_pages {
> >  	void __user *uptr;
> >  	bool writable:1;
> >  	u8 account_mode;
> > +	/*
> > +	 * CPU cache flush is required before mapping the pages to or after
> > +	 * unmapping it from a noncoherent domain
> > +	 */
> > +	bool cache_flush_required:1;
> 
> Move this up a line so it packs with the other bool bitfield.
Yes, thanks!

> >  static void batch_clear(struct pfn_batch *batch)
> >  {
> >  	batch->total_pfns = 0;
> > @@ -637,10 +648,18 @@ static void batch_unpin(struct pfn_batch *batch, struct iopt_pages *pages,
> >  	while (npages) {
> >  		size_t to_unpin = min_t(size_t, npages,
> >  					batch->npfns[cur] - first_page_off);
> > +		unsigned long pfn = batch->pfns[cur] + first_page_off;
> > +
> > +		/*
> > +		 * Lazily flushing CPU caches when a page is about to be
> > +		 * unpinned if the page was mapped into a noncoherent domain
> > +		 */
> > +		if (pages->cache_flush_required)
> > +			arch_clean_nonsnoop_dma(pfn << PAGE_SHIFT,
> > +						to_unpin << PAGE_SHIFT);
> >  
> >  		unpin_user_page_range_dirty_lock(
> > -			pfn_to_page(batch->pfns[cur] + first_page_off),
> > -			to_unpin, pages->writable);
> > +			pfn_to_page(pfn), to_unpin, pages->writable);
> >  		iopt_pages_sub_npinned(pages, to_unpin);
> >  		cur++;
> >  		first_page_off = 0;
> 
> Make sense
> 
> > @@ -1358,10 +1377,17 @@ int iopt_area_fill_domain(struct iopt_area *area, struct iommu_domain *domain)
> >  {
> >  	unsigned long done_end_index;
> >  	struct pfn_reader pfns;
> > +	bool cache_flush_required;
> >  	int rc;
> >  
> >  	lockdep_assert_held(&area->pages->mutex);
> >  
> > +	cache_flush_required = area->iopt->noncoherent_domain_cnt &&
> > +			       !area->pages->cache_flush_required;
> > +
> > +	if (cache_flush_required)
> > +		area->pages->cache_flush_required = true;
> > +
> >  	rc = pfn_reader_first(&pfns, area->pages, iopt_area_index(area),
> >  			      iopt_area_last_index(area));
> >  	if (rc)
> > @@ -1369,6 +1395,9 @@ int iopt_area_fill_domain(struct iopt_area *area, struct iommu_domain *domain)
> >  
> >  	while (!pfn_reader_done(&pfns)) {
> >  		done_end_index = pfns.batch_start_index;
> > +		if (cache_flush_required)
> > +			iopt_cache_flush_pfn_batch(&pfns.batch);
> > +
> 
> This is a bit unfortunate, it means we are going to flush for every
> domain, even though it is not required. I don't see any easy way out
> of that :(
Yes. Do you think it's possible to add an op get_cache_coherency_enforced
to iommu_domain_ops?