Received-SPF: pass (google.com: domain of linux-kernel+bounces-177272-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: Mon, 13 May 2024 15:43:45 +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: <ZkHEsfaGAXuOFMkq@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>
 <Zj3UuHQe4XgdDmDs@yzhao56-desk.sh.intel.com>
 <20240510132928.GS4650@nvidia.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240510132928.GS4650@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?D1U/JqkziZ0BQDN7ls/LDVMk7db/+GoatxdZgstWAJcFfJZlcRNAXiXtQ3og?=
 =?us-ascii?Q?t+alJVyQyWzOOD7GRLZ1fRezyI7OnTCO2MnJ4dCQlrmGRGdKDqyruq7uW18Q?=
 =?us-ascii?Q?TfkXWcZwVIsrGhp+5TFKn9+PFpPkeM6qXdzaB2h8nndvECLc7P8H2UY134c+?=
 =?us-ascii?Q?j3NXbCP6yaxPg/IG99l/yikxmkGAjxmYk+1husYJgyhOceejERd//ZEvEUfM?=
 =?us-ascii?Q?OsFatoCnzIXm/63SSxhObWBUYXaruv56wYoPWw+KvuuLLRppTND8Bfnsc1aa?=
 =?us-ascii?Q?M0i3UrR0qsQ428KRcAaR91obfYA5kmciEnd6xiz4C+BR6zsun1oi4M4+L2Nd?=
 =?us-ascii?Q?IEy6ytZSmookTQuguJ8R3RW+rJ/mhrdqRtwAsedwdj3yVsGojYXS/vSknZqs?=
 =?us-ascii?Q?niL8EmNre7W9TYiNwVCRyhlnWHk0ztq3xc/xBHCm+DfwAqDwKCrJOm8ONt6g?=
 =?us-ascii?Q?QOfLKdWZK/kCRJ6V/hxSWt1aPW1Mocp7ENky/xQq/IvAQ3q/Dl5TjZZXCral?=
 =?us-ascii?Q?f+EEcIlDfGsfDaJNtTRZA854Ebk3OIT9wOZLuFRQVyt9/pwwSTUiKY4y3tUW?=
 =?us-ascii?Q?cJxQ7WoqAIdJhjbeQYv7JhxzYkAJ8kXcV9NQywadBMjXuzrL2F1jD1suFPI7?=
 =?us-ascii?Q?nNBp2pDlixnCWOhD/Uuuff2r+1LSIDlzkPTBhR/i5aFlvgXM8bI/ib0+rfVm?=
 =?us-ascii?Q?T5f29jC916HP4mxz8jfCgA4KKO1U2yyCJu0FP50vZbeO30dHIJNXWGzuvw63?=
 =?us-ascii?Q?CcoBQJ44bAcntmGzm051E/7nUWMQcUj5tyETLvx0U7BichpjxZ6kz+O8vqG8?=
 =?us-ascii?Q?4oi3Fj3p+byYV+s1noDc5wUDYbCKTMvBrJ/kh0I6XgEp1NGSixrdeaTLBFGR?=
 =?us-ascii?Q?bFIn6WYPagkXlaF3JwvQ8gvta7kB/qAgB6gyXCW4ce0nOq6FVUXGGH8zAMWp?=
 =?us-ascii?Q?mM8E+ccwaYTcuCdViNoOqTNsFFYW5c7HwO+6GZlpL9ZAKlMYXw5NYqO6vmOh?=
 =?us-ascii?Q?FN6NeXfg5u0N9N5lTmLM4+G5Ts53wXGWw2bDP4SwXBudkQrq+/7ILOM54YH4?=
 =?us-ascii?Q?9b5OnMdsGymaqJQW1FXYO4Ez5MhcHSAP6hDsgx6kVtS+Vk0gZ4K+NqDpbacD?=
 =?us-ascii?Q?bnZGkihk1akEErFOuYKOCoQFMulSSeIdFXS1Mz00nwXKs8IggzvWwOGkYuWB?=
 =?us-ascii?Q?DYd/7jcddKrMzCrEtiiVBGX0G/edik8mnEXx9xlLWO5DQKs215SXZlYDqbgs?=
 =?us-ascii?Q?UhdMn6D9OOrsDIHjrcXf5XJYSS6tC5YtWPXnuIChfzrM0412MadlHABJAeBe?=
 =?us-ascii?Q?w0JFl/Od407yXjr68JcJFGpzZ/RR83QpBzieVv1yZWPZuBuZHDT+WAPpBDb/?=
 =?us-ascii?Q?azEK4aYcxV3qsUi0AI2zetF3lyq9ncQkkyIdbpEv+Rqo31Lbh1S+ccM756OI?=
 =?us-ascii?Q?eLvCTkDRUKoyKDSHjfvEXk3KYsRrTqAQm57/ZyBwKEZI0Z7248W3zZ1ErZfm?=
 =?us-ascii?Q?1lMnm9pDajiLxmMOxcymWiSjjQxasWWv9rgHTVcEFOPoEzzzBPefWFhy7uHA?=
 =?us-ascii?Q?6ZObc3vLqFyIhsHs6qOPRKeJWKNzQyIWHeVf6TWZ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5e29fb37-2937-4860-499d-08dc732085d7
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 May 2024 07:44:30.3541
 (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: V6Pmk0tQ07tWOoTeH1XrNhxBLh4lu51OMaZzbaW66zVFY2r+NuAsNxebB5qivNNid4k2BZAbGV/b9NIh6cp+zg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7911
X-OriginatorOrg: intel.com

On Fri, May 10, 2024 at 10:29:28AM -0300, Jason Gunthorpe wrote:
> On Fri, May 10, 2024 at 04:03:04PM +0800, Yan Zhao wrote:
> > > > @@ -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?
> 
> Do we need that? The hwpt already keeps track of that? the enforced could be
> copied into the area along side storage_domain
> 
> Then I guess you could avoid flushing in the case the page came from
> the storage_domain...
> 
> You'd want the storage_domain to preferentially point to any
> non-enforced domain.
> 
> Is it worth it? How slow is this stuff?
Sorry, I might have misunderstood your intentions in my previous mail.
In iopt_area_fill_domain(), flushing CPU caches is only performed when
(1) noncoherent_domain_cnt is non-zero and
(2) area->pages->cache_flush_required is false.
area->pages->cache_flush_required is also set to true after the two are met, so
that the next flush to the same "area->pages" in filling phase will be skipped.

In my last mail, I thought you wanted to flush for every domain even if
area->pages->cache_flush_required is true, because I thought that you were
worried about that checking area->pages->cache_flush_required might results in
some pages, which ought be flushed, not being flushed.
So, I was wondering if we could do the flush for every non-coherent domain by
checking whether domain enforces cache coherency.

However, as you said, we can check hwpt instead if it's passed in
iopt_area_fill_domain().

On the other side, after a second thought, looks it's still good to check
area->pages->cache_flush_required?
- "area" and "pages" are 1:1. In other words, there's no such a condition that
  several "area"s are pointing to the same "pages".
  Is this assumption right?
- Once area->pages->cache_flush_required is set to true, it means all pages
  indicated by "area->pages" has been mapped into a non-coherent domain
  (though the domain is not necessarily the storage domain).
  Is this assumption correct as well?
  If so, we can safely skip the flush in iopt_area_fill_domain() if
  area->pages->cache_flush_required is true.