Received-SPF: pass (google.com: domain of linux-kernel+bounces-181714-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 17 May 2024 11:11:48 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Alex Williamson <alex.williamson@redhat.com>
CC: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
	<jgg@nvidia.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 4/5] vfio/type1: Flush CPU caches on DMA pages in
 non-coherent domains
Message-ID: <ZkbK9CzmcxgqhSuR@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20240507061802.20184-1-yan.y.zhao@intel.com>
 <20240507062138.20465-1-yan.y.zhao@intel.com>
 <20240509121049.58238a6f.alex.williamson@redhat.com>
 <Zj33cUe7HYOIfj5N@yzhao56-desk.sh.intel.com>
 <20240510105728.76d97bbb.alex.williamson@redhat.com>
 <ZkG9IEQwi7HG3YBk@yzhao56-desk.sh.intel.com>
 <20240516145009.3bcd3d0c.alex.williamson@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240516145009.3bcd3d0c.alex.williamson@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?CSBBb6vE3jFb5NYDBDlVUbewG5Zjem7z5M1Ibel6SVAOdvkYY5J8d6ySMjcC?=
 =?us-ascii?Q?M+M9PAbvMxGWhqWQe0o+vSOqBnYJLo4irCaJVSovC1JDJsy7yPHLRtdAO051?=
 =?us-ascii?Q?+nXCM52XwZougq6BFtrZnQK9UScZRijVWz7xSJvfRi1un5EZzfpMxPKGkyUh?=
 =?us-ascii?Q?LuOfIxNhdpW5IDC6Pb7tQyyaVnIM0cQ7ttgNz+gubv/GzkM0WXL+Qt0OO1GK?=
 =?us-ascii?Q?T4sdNjjGR2D187CCTvks2//5xXoDYjM/3YcHYWsWqOQ/1SYzPu/1JFHyrwvc?=
 =?us-ascii?Q?b0n3EZpOD5Eqs7Pq0/VykFs2Og2Q5kYIwia+1VKLVz240yY9Heg0VGT/DzZ9?=
 =?us-ascii?Q?rzEYG/4g3XGrPxfuN0EkX8ZvJMnzVFdkEGCNeYU17Ls2WI3EiZM03DBqsUw/?=
 =?us-ascii?Q?IKza2jd7tvNdcXRHSkWneY2fF/sOLwpU4F8VWJlJXMFtdGqwhAhhwF/Z66qK?=
 =?us-ascii?Q?SO/4hahEdvFHsaYGggH1eIexrzNR1+flov9e80kxncp1WmzVfbgAeEHVNk6X?=
 =?us-ascii?Q?wYVsc87K2XGeFIJkDpelxbMGIBzun9D/WVQ2u6U9TBDEq1vXA5MWnXhl9uKy?=
 =?us-ascii?Q?m20TgWPsl4V8SbcTEmOmJ4+tkeEnrDpboQYIP/eW+mgUxRb7+zEO6NYb7tbS?=
 =?us-ascii?Q?s2SJBV3lDIaC44220gunLnnP8lor0wDkT97CXBHODrPzeCw6AoyeczL3tEWA?=
 =?us-ascii?Q?vU9oI6zapOo17sWX+xmjcrPPAuKIk7r6OykrcRQcuZQBG+ztRNKDGpOfGZrs?=
 =?us-ascii?Q?bC58LK0MR0XJ7JjCrrZNyf9B+dV/W1mF316ZUrYPktcW6X+oYFA+KDTyyjFy?=
 =?us-ascii?Q?fY/9jCEvOdKq4ocfu+Weex/vfPrOwzLGXqQ4PVXiDeFfMY8e4PvBgmJ2QV6E?=
 =?us-ascii?Q?ejENaFrcF2ifGauC8jZ6Kug5EEUEzDnva9hyCD1l1qBU9kzqNX90fT4k6NhK?=
 =?us-ascii?Q?sR60whGz0dB7a+g7do/1Cii9yEwIbu30lFTZhG8eJyE2/UOvKLvIpleMOTLk?=
 =?us-ascii?Q?2rx1iujhUuuvc28Nu/QKZ8UyVnwclgiFc1iFfX7sIYpCrFjdndAkIzmUdN+C?=
 =?us-ascii?Q?kVcUALmtRTOXR5cvwHM7pg4b12kNaQFfJh8tQRI7jdXsQzNgUmccdI+XyPnR?=
 =?us-ascii?Q?8gTuLRBv4qNvySIh1n6O8Cs7xZ3S/SCXABsumLVeZRZM2R8BJi1tCR2AjwmK?=
 =?us-ascii?Q?7KCt6GhnCprmiaM5Hc/CpgLmqcq1DQEDka/nKSDHVfyGJf1dB/aowgFD0YzE?=
 =?us-ascii?Q?e2B3O+OcoMPQ0+3MLZIoxvOt5SwVVi65rLUn3Qw0xBhYFRm/xsdTUNVlrOmn?=
 =?us-ascii?Q?rUaNslkEM+MOrokBavYZQF2KIUtlsbmip9vtwvRhysfJ6nt/cjtIso47K217?=
 =?us-ascii?Q?Ziid5Nfaz/yKByPgc2l8dFPYXyprEEL70XNTF65+LFKrpOFA9yJbsvfhLwnL?=
 =?us-ascii?Q?QzR0V4CwG+t9BjKrl9f2kwgKxe25h52jxy14gEm3p3gnv6qxI8abKyMPLfbZ?=
 =?us-ascii?Q?6W787KMH5LsnebK3Odc21fYV+anRpwIUvQrn5sq3foUbYVjfLQsSnTyk6CYf?=
 =?us-ascii?Q?J0STlmQZCqQCqPDgm6//OyoNQOTrFSRkjdAzPxwP?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b8456bdb-f310-41ac-95bf-08dc761f34dd
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2024 03:12:38.4121
 (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: /v3FTOPghRFDYequsXZO+iAjupjiZHhRTjZeD+M8opzrFn5byAt09fPI/4uZlLbynTRrlOvMKv00uGnQOmMnCQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6893
X-OriginatorOrg: intel.com

On Thu, May 16, 2024 at 02:50:09PM -0600, Alex Williamson wrote:
> On Mon, 13 May 2024 15:11:28 +0800
> Yan Zhao <yan.y.zhao@intel.com> wrote:
> 
> > On Fri, May 10, 2024 at 10:57:28AM -0600, Alex Williamson wrote:
> > > On Fri, 10 May 2024 18:31:13 +0800
> > > Yan Zhao <yan.y.zhao@intel.com> wrote:
> > >   
> > > > On Thu, May 09, 2024 at 12:10:49PM -0600, Alex Williamson wrote:  
> > > > > On Tue,  7 May 2024 14:21:38 +0800
> > > > > Yan Zhao <yan.y.zhao@intel.com> wrote:    
..   
> > > > > > @@ -1683,9 +1715,14 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
> > > > > >  	for (; n; n = rb_next(n)) {
> > > > > >  		struct vfio_dma *dma;
> > > > > >  		dma_addr_t iova;
> > > > > > +		bool cache_flush_required;
> > > > > >  
> > > > > >  		dma = rb_entry(n, struct vfio_dma, node);
> > > > > >  		iova = dma->iova;
> > > > > > +		cache_flush_required = !domain->enforce_cache_coherency &&
> > > > > > +				       !dma->cache_flush_required;
> > > > > > +		if (cache_flush_required)
> > > > > > +			dma->cache_flush_required = true;    
> > > > > 
> > > > > The variable name here isn't accurate and the logic is confusing.  If
> > > > > the domain does not enforce coherency and the mapping is not tagged as
> > > > > requiring a cache flush, then we need to mark the mapping as requiring
> > > > > a cache flush.  So the variable state is something more akin to
> > > > > set_cache_flush_required.  But all we're saving with this is a
> > > > > redundant set if the mapping is already tagged as requiring a cache
> > > > > flush, so it could really be simplified to:
> > > > > 
> > > > > 		dma->cache_flush_required = !domain->enforce_cache_coherency;    
> > > > Sorry about the confusion.
> > > > 
> > > > If dma->cache_flush_required is set to true by a domain not enforcing cache
> > > > coherency, we hope it will not be reset to false by a later attaching to domain 
> > > > enforcing cache coherency due to the lazily flushing design.  
> > > 
> > > Right, ok, the vfio_dma objects are shared between domains so we never
> > > want to set 'dma->cache_flush_required = false' due to the addition of a
> > > 'domain->enforce_cache_coherent == true'.  So this could be:
> > > 
> > > 	if (!dma->cache_flush_required)
> > > 		dma->cache_flush_required = !domain->enforce_cache_coherency;  
> > 
> > Though this code is easier for understanding, it leads to unnecessary setting of
> > dma->cache_flush_required to false, given domain->enforce_cache_coherency is
> > true at the most time.
> 
> I don't really see that as an issue, but the variable name originally
> chosen above, cache_flush_required, also doesn't convey that it's only
> attempting to set the value if it wasn't previously set and is now
> required by a noncoherent domain.
Agreed, the old name is too vague.
What about update_to_noncoherent_required?
Then in vfio_iommu_replay(), it's like

update_to_noncoherent_required = !domain->enforce_cache_coherency && !dma->is_noncoherent;
if (update_to_noncoherent_required)
         dma->is_noncoherent = true;

..
if (update_to_noncoherent_required)
	arch_flush_cache_phys((phys, size);
> 
> > > > > It might add more clarity to just name the mapping flag
> > > > > dma->mapped_noncoherent.    
> > > > 
> > > > The dma->cache_flush_required is to mark whether pages in a vfio_dma requires
> > > > cache flush in the subsequence mapping into the first non-coherent domain
> > > > and page unpinning.  
> > > 
> > > How do we arrive at a sequence where we have dma->cache_flush_required
> > > that isn't the result of being mapped into a domain with
> > > !domain->enforce_cache_coherency?  
> > Hmm, dma->cache_flush_required IS the result of being mapped into a domain with
> > !domain->enforce_cache_coherency.
> > My concern only arrives from the actual code sequence, i.e.
> > dma->cache_flush_required is set to true before the actual mapping.
> > 
> > If we rename it to dma->mapped_noncoherent and only set it to true after the
> > actual successful mapping, it would lead to more code to handle flushing for the
> > unwind case.
> > Currently, flush for unwind is handled centrally in vfio_unpin_pages_remote()
> > by checking dma->cache_flush_required, which is true even before a full
> > successful mapping, so we won't miss flush on any pages that are mapped into a
> > non-coherent domain in a short window.
> 
> I don't think we need to be so literal that "mapped_noncoherent" can
> only be set after the vfio_dma is fully mapped to a noncoherent domain,
> but also we can come up with other names for the flag.  Perhaps
> "is_noncoherent".  My suggestion was more from the perspective of what
> does the flag represent rather than what we intend to do as a result of
> the flag being set.  Thanks, 
Makes sense!
I like the name "is_noncoherent" :)