Received-SPF: pass (google.com: domain of linux-kernel+bounces-174578-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 9 May 2024 11:13:32 -0300
From: Jason Gunthorpe <jgg@nvidia.com>
To: Yan Zhao <yan.y.zhao@intel.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: <20240509141332.GP4650@nvidia.com>
References: <20240507061802.20184-1-yan.y.zhao@intel.com>
 <20240507062212.20535-1-yan.y.zhao@intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240507062212.20535-1-yan.y.zhao@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?GRRUUOhRLRW06riDsCfoZOt+O2mEVgvs2n0lT11bDbF/zzMBV4KjtTucua5w?=
 =?us-ascii?Q?oPnKNaSIDsWz5XG7j7Ahfi6LrgNxHF6Epp9v+G3BmFvj6B93xdh9cqttsCrC?=
 =?us-ascii?Q?y8huDRnRf15+ICDbJPSel8DPO2k+T3PucHNRq/BHP/P/A+qpWDzB5i5tlL8o?=
 =?us-ascii?Q?h41d0hKp9ls0Zfuc2oaMao1GaI+JrC6qgAtHnLm9KsRjmW/zBnNqD8c4tg45?=
 =?us-ascii?Q?4S0amTC+75543CYdl0Wsbm3H0ozEC6+73mnvI5sIznZOlf2z3myXilEVcISQ?=
 =?us-ascii?Q?t+8QYisy+4kEaFd06m+S55Wf/Sv2CSYLRzKICjs0SXOxrBHRqXX9zGziI5xG?=
 =?us-ascii?Q?tpy5iLh+a/A7QMmCEUCm2L9xc2zQsJ55cpXzI5hXAjwk87fOQ1apOJ/nxyBx?=
 =?us-ascii?Q?vnPbLhjHnLEPYACX4xiivF8E/HDcMmFHwGuU9JNWz90E8FCg+z0dZWcNN6qX?=
 =?us-ascii?Q?Qv+CzNL4pIjiMWytbfTJ/hH0RIoK/Nj/Z6pHHKACWg3jmI109KxUQNXgzn3B?=
 =?us-ascii?Q?ctmVLNOp/kS5c1ert6bZda4Y7whmv0RQZCcXE/2jxV86jeLJ/NIScoXBa1Wt?=
 =?us-ascii?Q?Md8RvOaEz8qrrFla/CuRSwAhnO6gWTCFf0o8Nb1rD2bxcpKfcM8wUV8LT5tn?=
 =?us-ascii?Q?eyRhmTmIBimFdPgOSTkRHyAjrW92eQnh8RCK2e/EN+IOfi3xIpkm11uDxsSD?=
 =?us-ascii?Q?2dDHxKdf1YtN3PGP8vfGL1CZ+DDQWqTTYreRcl4XLShd2OW74Mh5e5STsYNm?=
 =?us-ascii?Q?iByVHtgWREiLdpe2WXuzkjVnlm6J3NuAsqf6apzLUfySVqFFJczzaOVICJTZ?=
 =?us-ascii?Q?fgcug7LMT5am2iTlQPzAfun+giZjfsdqW+ULaLAj9g9LL/MzEnwyeiMKwBkn?=
 =?us-ascii?Q?uoqzqcBMd41AzAHkunWowpnjt9u9xqK5nzR66NbeVY9lugVe+d0OGSHif56h?=
 =?us-ascii?Q?4oHYii9DSAlU3U3Ah9XwlawzQkU95np4IuuXGhCE0Z63TG+RaWhb0oSXnpPq?=
 =?us-ascii?Q?vYJVM17Lw898XbQ1mY8rn1K+lTr1qWbC1fvPr+hnCmKmtsabFEQyxfeVgUoI?=
 =?us-ascii?Q?6sk6fOAMOKHkbl0TsJGH6DNBfL2ZZsY0dWD60MXqgVuqgq3cLXGkP7Fed3Q9?=
 =?us-ascii?Q?0opDiSXrLIxTxODrGAAP5auWgg8sw+XCP/MlbSmQ7phyWexqwWorygsike35?=
 =?us-ascii?Q?HnnR9m+tyf1K8Eo35HOShiHC4/A1lwLGFvWiNTpbV0yxskabXDMqGbJFQcXx?=
 =?us-ascii?Q?ovxjd3hehv/SDMGRYGwcpZOxcidx+3l8Zk2SOSR2zwMghuqPiSIafU1tZaap?=
 =?us-ascii?Q?HpECEPisYlJHdaJTSyaWdTLWLbU4lMoVgY7cbBgabXIwAzeTqTgbOGBvF3m1?=
 =?us-ascii?Q?3G7DBtzEucDRmAnX8/T+D4HrLL64685KpGtr1O3HidOGlUZtN0w0FMBb4NB3?=
 =?us-ascii?Q?ITpS8VuEEjvlPfYrs1xhHYf/5LaKuFyJSfVJ8WL3fC1ozjtBfbKLxWwQGOe5?=
 =?us-ascii?Q?4+3i8mmEQEtWTn1gyJyUWQQyYF1RIkDplbE0n44uPYvXcXLPyu/GvXmq4oyn?=
 =?us-ascii?Q?9hw3D5dKHKNZWXR4sS9ni6o6LdamMZ4UahdJTLnc?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 16387807-a4cd-47ce-1f1f-08dc70323640
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3849.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2024 14:13:34.6416
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mGa17LiyJwrs9/A3gGXdXpO4MLf6pxSal49mZ+5NZ8k8oG6voBtvyXDh6UXIgpuf
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB8034

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

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

>  	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

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

>  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 :(

Jason