From: Keqian Zhu <zhukeqian1@huawei.com> To: Alex Williamson <alex.williamson@redhat.com> Cc: <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <iommu@lists.linux-foundation.org>, <kvm@vger.kernel.org>, <kvmarm@lists.cs.columbia.edu>, Cornelia Huck <cohuck@redhat.com>, "Will Deacon" <will@kernel.org>, Marc Zyngier <maz@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, Mark Rutland <mark.rutland@arm.com>, James Morse <james.morse@arm.com>, Robin Murphy <robin.murphy@arm.com>, Joerg Roedel <joro@8bytes.org>, Daniel Lezcano <daniel.lezcano@linaro.org>, "Thomas Gleixner" <tglx@linutronix.de>, Suzuki K Poulose <suzuki.poulose@arm.com>, Julien Thierry <julien.thierry.kdev@gmail.com>, Andrew Morton <akpm@linux-foundation.org>, Alexios Zavras <alexios.zavras@intel.com>, <wanghaibin.wang@huawei.com>, <jiangkunkun@huawei.com> Subject: Re: [PATCH 1/6] vfio/iommu_type1: Make an explicit "promote" semantic Date: Mon, 18 Jan 2021 21:33:39 +0800 [thread overview] Message-ID: <0cf47d65-cb91-199e-af7d-048134634298@huawei.com> (raw) In-Reply-To: <20210115154240.0d3ee455@omen.home.shazbot.org> On 2021/1/16 6:42, Alex Williamson wrote: > On Thu, 7 Jan 2021 12:43:56 +0800 > Keqian Zhu <zhukeqian1@huawei.com> wrote: > >> When we want to promote the pinned_page_dirty_scope of vfio_iommu, >> we call the "update" function to visit all vfio_group, but when we >> want to downgrade this, we can set the flag as false directly. > > I agree that the transition can only go in one direction, but it's > still conditional on the scope of all groups involved. We are > "updating" the iommu state based on the change of a group. Renaming > this to "promote" seems like a matter of personal preference. > >> So we'd better make an explicit "promote" semantic to the "update" >> function. BTW, if vfio_iommu already has been promoted, then return >> early. > > Currently it's the caller that avoids using this function when the > iommu scope is already correct. In fact the changes induces a > redundant test in the pin_pages code path, we're changing a group from > non-pinned-page-scope to pinned-page-scope, therefore the iommu scope > cannot initially be scope limited. In the attach_group call path, > we're moving that test from the caller, so at best we've introduced an > additional function call. > > The function as it exists today is also more versatile whereas the > "promote" version here forces it to a single task with no appreciable > difference in complexity or code. This seems like a frivolous change. > Thanks, OK, I will adapt your idea that maintenance a counter of non-pinned groups. Then we keep the "update" semantic, and the target is the counter ;-). Thanks, Keqian > > Alex > >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> drivers/vfio/vfio_iommu_type1.c | 30 ++++++++++++++---------------- >> 1 file changed, 14 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index 0b4dedaa9128..334a8240e1da 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -148,7 +148,7 @@ static int put_pfn(unsigned long pfn, int prot); >> static struct vfio_group *vfio_iommu_find_iommu_group(struct vfio_iommu *iommu, >> struct iommu_group *iommu_group); >> >> -static void update_pinned_page_dirty_scope(struct vfio_iommu *iommu); >> +static void promote_pinned_page_dirty_scope(struct vfio_iommu *iommu); >> /* >> * This code handles mapping and unmapping of user data buffers >> * into DMA'ble space using the IOMMU >> @@ -714,7 +714,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data, >> group = vfio_iommu_find_iommu_group(iommu, iommu_group); >> if (!group->pinned_page_dirty_scope) { >> group->pinned_page_dirty_scope = true; >> - update_pinned_page_dirty_scope(iommu); >> + promote_pinned_page_dirty_scope(iommu); >> } >> >> goto pin_done; >> @@ -1622,27 +1622,26 @@ static struct vfio_group *vfio_iommu_find_iommu_group(struct vfio_iommu *iommu, >> return group; >> } >> >> -static void update_pinned_page_dirty_scope(struct vfio_iommu *iommu) >> +static void promote_pinned_page_dirty_scope(struct vfio_iommu *iommu) >> { >> struct vfio_domain *domain; >> struct vfio_group *group; >> >> + if (iommu->pinned_page_dirty_scope) >> + return; >> + >> list_for_each_entry(domain, &iommu->domain_list, next) { >> list_for_each_entry(group, &domain->group_list, next) { >> - if (!group->pinned_page_dirty_scope) { >> - iommu->pinned_page_dirty_scope = false; >> + if (!group->pinned_page_dirty_scope) >> return; >> - } >> } >> } >> >> if (iommu->external_domain) { >> domain = iommu->external_domain; >> list_for_each_entry(group, &domain->group_list, next) { >> - if (!group->pinned_page_dirty_scope) { >> - iommu->pinned_page_dirty_scope = false; >> + if (!group->pinned_page_dirty_scope) >> return; >> - } >> } >> } >> >> @@ -2057,8 +2056,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, >> * addition of a dirty tracking group. >> */ >> group->pinned_page_dirty_scope = true; >> - if (!iommu->pinned_page_dirty_scope) >> - update_pinned_page_dirty_scope(iommu); >> + promote_pinned_page_dirty_scope(iommu); >> mutex_unlock(&iommu->lock); >> >> return 0; >> @@ -2341,7 +2339,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> struct vfio_iommu *iommu = iommu_data; >> struct vfio_domain *domain; >> struct vfio_group *group; >> - bool update_dirty_scope = false; >> + bool promote_dirty_scope = false; >> LIST_HEAD(iova_copy); >> >> mutex_lock(&iommu->lock); >> @@ -2349,7 +2347,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> if (iommu->external_domain) { >> group = find_iommu_group(iommu->external_domain, iommu_group); >> if (group) { >> - update_dirty_scope = !group->pinned_page_dirty_scope; >> + promote_dirty_scope = !group->pinned_page_dirty_scope; >> list_del(&group->next); >> kfree(group); >> >> @@ -2379,7 +2377,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> continue; >> >> vfio_iommu_detach_group(domain, group); >> - update_dirty_scope = !group->pinned_page_dirty_scope; >> + promote_dirty_scope = !group->pinned_page_dirty_scope; >> list_del(&group->next); >> kfree(group); >> /* >> @@ -2415,8 +2413,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> * Removal of a group without dirty tracking may allow the iommu scope >> * to be promoted. >> */ >> - if (update_dirty_scope) >> - update_pinned_page_dirty_scope(iommu); >> + if (promote_dirty_scope) >> + promote_pinned_page_dirty_scope(iommu); >> mutex_unlock(&iommu->lock); >> } >> > > . >
next prev parent reply other threads:[~2021-01-18 13:34 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-07 4:43 [PATCH 0/6] vfio/iommu_type1: Some optimizations about dirty tracking Keqian Zhu 2021-01-07 4:43 ` [PATCH 1/6] vfio/iommu_type1: Make an explicit "promote" semantic Keqian Zhu 2021-01-15 22:42 ` Alex Williamson 2021-01-18 13:33 ` Keqian Zhu [this message] 2021-01-07 4:43 ` [PATCH 2/6] vfio/iommu_type1: Ignore external domain when promote pinned_scope Keqian Zhu 2021-01-15 23:23 ` Alex Williamson 2021-01-07 4:43 ` [PATCH 3/6] vfio/iommu_type1: Initially set the pinned_page_dirty_scope Keqian Zhu 2021-01-15 23:30 ` Alex Williamson 2021-01-18 13:34 ` Keqian Zhu 2021-01-07 4:43 ` [PATCH 4/6] vfio/iommu_type1: Drop parameter "pgsize" of vfio_dma_bitmap_alloc_all Keqian Zhu 2021-01-15 23:37 ` Alex Williamson 2021-01-07 4:44 ` [PATCH 5/6] vfio/iommu_type1: Drop parameter "pgsize" of vfio_iova_dirty_bitmap Keqian Zhu 2021-01-15 23:39 ` Alex Williamson 2021-01-07 4:44 ` [PATCH 6/6] vfio/iommu_type1: Drop parameter "pgsize" of update_user_bitmap Keqian Zhu 2021-01-15 23:44 ` Alex Williamson 2021-01-18 13:48 ` Keqian Zhu
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=0cf47d65-cb91-199e-af7d-048134634298@huawei.com \ --to=zhukeqian1@huawei.com \ --cc=akpm@linux-foundation.org \ --cc=alex.williamson@redhat.com \ --cc=alexios.zavras@intel.com \ --cc=catalin.marinas@arm.com \ --cc=cohuck@redhat.com \ --cc=daniel.lezcano@linaro.org \ --cc=iommu@lists.linux-foundation.org \ --cc=james.morse@arm.com \ --cc=jiangkunkun@huawei.com \ --cc=joro@8bytes.org \ --cc=julien.thierry.kdev@gmail.com \ --cc=kvm@vger.kernel.org \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=maz@kernel.org \ --cc=robin.murphy@arm.com \ --cc=suzuki.poulose@arm.com \ --cc=tglx@linutronix.de \ --cc=wanghaibin.wang@huawei.com \ --cc=will@kernel.org \ --subject='Re: [PATCH 1/6] vfio/iommu_type1: Make an explicit "promote" semantic' \ /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
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 NNTP newsgroup(s).