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
Date: Fri, 10 May 2024 16:03:04 +0800 [thread overview]
Message-ID: <Zj3UuHQe4XgdDmDs@yzhao56-desk.sh.intel.com> (raw)
In-Reply-To: <20240509141332.GP4650@nvidia.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?
next prev parent reply other threads:[~2024-05-10 8:03 UTC|newest]
Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 6:18 [PATCH 0/5] Enforce CPU cache flush for non-coherent device assignment Yan Zhao
2024-05-07 6:19 ` [PATCH 1/5] x86/pat: Let pat_pfn_immune_to_uc_mtrr() check MTRR for untracked PAT range Yan Zhao
2024-05-07 8:26 ` Tian, Kevin
2024-05-07 9:12 ` Yan Zhao
2024-05-08 22:14 ` Alex Williamson
2024-05-09 3:36 ` Yan Zhao
2024-05-16 7:42 ` Tian, Kevin
2024-05-16 14:07 ` Sean Christopherson
2024-05-20 2:36 ` Tian, Kevin
2024-05-07 6:20 ` [PATCH 2/5] KVM: x86/mmu: Fine-grained check of whether a invalid & RAM PFN is MMIO Yan Zhao
2024-05-07 8:39 ` Tian, Kevin
2024-05-07 9:19 ` Yan Zhao
2024-05-07 6:20 ` [PATCH 3/5] x86/mm: Introduce and export interface arch_clean_nonsnoop_dma() Yan Zhao
2024-05-07 8:51 ` Tian, Kevin
2024-05-07 9:40 ` Yan Zhao
2024-05-20 14:07 ` Christoph Hellwig
2024-05-21 15:49 ` Jason Gunthorpe
2024-05-21 16:00 ` Jason Gunthorpe
2024-05-22 3:41 ` Yan Zhao
2024-05-28 6:37 ` Christoph Hellwig
2024-06-01 19:46 ` Jason Gunthorpe
2024-05-07 6:21 ` [PATCH 4/5] vfio/type1: Flush CPU caches on DMA pages in non-coherent domains Yan Zhao
2024-05-09 18:10 ` Alex Williamson
2024-05-10 10:31 ` Yan Zhao
2024-05-10 16:57 ` Alex Williamson
2024-05-13 7:11 ` Yan Zhao
2024-05-16 7:53 ` Tian, Kevin
2024-05-16 8:34 ` Tian, Kevin
2024-05-16 20:31 ` Alex Williamson
2024-05-17 17:11 ` Jason Gunthorpe
2024-05-20 2:52 ` Tian, Kevin
2024-05-21 16:07 ` Jason Gunthorpe
2024-05-21 16:21 ` Alex Williamson
2024-05-21 16:34 ` Jason Gunthorpe
2024-05-21 18:19 ` Alex Williamson
2024-05-21 18:37 ` Jason Gunthorpe
2024-05-22 6:24 ` Tian, Kevin
2024-05-22 12:29 ` Jason Gunthorpe
2024-05-22 14:43 ` Alex Williamson
2024-05-22 16:52 ` Jason Gunthorpe
2024-05-22 18:22 ` Alex Williamson
2024-05-22 23:26 ` Tian, Kevin
2024-05-22 23:32 ` Jason Gunthorpe
2024-05-22 23:40 ` Tian, Kevin
2024-05-23 14:58 ` Jason Gunthorpe
2024-05-23 22:47 ` Alex Williamson
2024-05-24 0:30 ` Tian, Kevin
2024-05-24 13:50 ` Jason Gunthorpe
2024-05-22 3:33 ` Yan Zhao
2024-05-22 3:24 ` Yan Zhao
2024-05-22 12:26 ` Jason Gunthorpe
2024-05-24 3:07 ` Yan Zhao
2024-05-16 20:50 ` Alex Williamson
2024-05-17 3:11 ` Yan Zhao
2024-05-17 4:44 ` Alex Williamson
2024-05-17 5:00 ` Yan Zhao
2024-05-07 6:22 ` [PATCH 5/5] iommufd: " Yan Zhao
2024-05-09 14:13 ` Jason Gunthorpe
2024-05-10 8:03 ` Yan Zhao [this message]
2024-05-10 13:29 ` Jason Gunthorpe
2024-05-13 7:43 ` Yan Zhao
2024-05-14 15:11 ` Jason Gunthorpe
2024-05-15 7:06 ` Yan Zhao
2024-05-15 20:43 ` Jason Gunthorpe
2024-05-16 2:32 ` Yan Zhao
2024-05-16 8:38 ` Tian, Kevin
2024-05-16 9:48 ` Yan Zhao
2024-05-17 17:04 ` Jason Gunthorpe
2024-05-20 2:45 ` Yan Zhao
2024-05-21 16:04 ` Jason Gunthorpe
2024-05-22 3:17 ` Yan Zhao
2024-05-22 6:29 ` Yan Zhao
2024-05-22 17:01 ` Jason Gunthorpe
2024-05-27 7:15 ` Yan Zhao
2024-06-01 16:48 ` Jason Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zj3UuHQe4XgdDmDs@yzhao56-desk.sh.intel.com \
--to=yan.y.zhao@intel.com \
--cc=alex.williamson@redhat.com \
--cc=baolu.lu@linux.intel.com \
--cc=bp@alien8.de \
--cc=corbet@lwn.net \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=iommu@lists.linux.dev \
--cc=jgg@nvidia.com \
--cc=joro@8bytes.org \
--cc=kevin.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peterz@infradead.org \
--cc=robin.murphy@arm.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
--cc=x86@kernel.org \
--cc=yi.l.liu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).