Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
On Mon, Jun 20, 2022 at 11:11:01AM +0100, Robin Murphy wrote:
> External email: Use caution opening links or attachments
>
>
> On 2022-06-17 03:53, Tian, Kevin wrote:
> > > From: Nicolin Chen
> > > Sent: Friday, June 17, 2022 6:41 AM
> > >
> > > > ...
> > > > > - if (resv_msi) {
> > > > > + if (resv_msi && !domain->msi_cookie) {
> > > > >ret = iommu_get_msi_cookie(domain->domain,
> > > > > resv_msi_base);
> > > > >if (ret && ret != -ENODEV)
> > > > >goto out_detach;
> > > > > + domain->msi_cookie = true;
> > > > >}
> > > >
> > > > why not moving to alloc_attach_domain() then no need for the new
> > > > domain field? It's required only when a new domain is allocated.
> > >
> > > When reusing an existing domain that doesn't have an msi_cookie,
> > > we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> > > not limited to a new domain.
> >
> > Looks msi_cookie requirement is per platform (currently only
> > for smmu. see arm_smmu_get_resv_regions()). If there is
> > no mixed case then above check is not required.
> >
> > But let's hear whether Robin has a different thought here.
>
> Yes, the cookie should logically be tied to the lifetime of the domain
> itself. In the relevant context, "an existing domain that doesn't have
> an msi_cookie" should never exist.
Thanks for the explanation. I will move the iommu_get_msi_cookie()
into alloc_attach_domain(), as Kevin suggested.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
On Mon, Jun 20, 2022 at 01:03:17AM -0300, Jason Gunthorpe wrote: > On Fri, Jun 17, 2022 at 04:07:20PM -0700, Nicolin Chen wrote: > > > > > > > + vfio_iommu_aper_expand(iommu, _copy); > > > > > > > > > > but now it's done for every group detach. The aperture is decided > > > > > by domain geometry which is not affected by attached groups. > > > > > > > > Yea, I've noticed this part. Actually Jason did this change for > > > > simplicity, and I think it'd be safe to do so? > > > > > > Perhaps detach_destroy() can return a Boolean to indicate whether > > > a domain is destroyed. > > > > It could be a solution but doesn't feel that common for a clean > > function to have a return value indicating a special case. Maybe > > passing in "" so that we can check if it's NULL after? > > It is harmless to do every time, it just burns a few CPU cycles on a > slow path. We don't need complexity to optmize it. OK. I will keep it simple then. If Kevin or other has a further objection, please let us know. Thanks Nic ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
On 2022-06-17 03:53, Tian, Kevin wrote:
From: Nicolin Chen
Sent: Friday, June 17, 2022 6:41 AM
...
- if (resv_msi) {
+ if (resv_msi && !domain->msi_cookie) {
ret = iommu_get_msi_cookie(domain->domain,
resv_msi_base);
if (ret && ret != -ENODEV)
goto out_detach;
+ domain->msi_cookie = true;
}
why not moving to alloc_attach_domain() then no need for the new
domain field? It's required only when a new domain is allocated.
When reusing an existing domain that doesn't have an msi_cookie,
we can do iommu_get_msi_cookie() if resv_msi is found. So it is
not limited to a new domain.
Looks msi_cookie requirement is per platform (currently only
for smmu. see arm_smmu_get_resv_regions()). If there is
no mixed case then above check is not required.
But let's hear whether Robin has a different thought here.
Yes, the cookie should logically be tied to the lifetime of the domain
itself. In the relevant context, "an existing domain that doesn't have
an msi_cookie" should never exist.
Thanks,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
On Fri, Jun 17, 2022 at 04:07:20PM -0700, Nicolin Chen wrote: > > > > > + vfio_iommu_aper_expand(iommu, _copy); > > > > > > > > but now it's done for every group detach. The aperture is decided > > > > by domain geometry which is not affected by attached groups. > > > > > > Yea, I've noticed this part. Actually Jason did this change for > > > simplicity, and I think it'd be safe to do so? > > > > Perhaps detach_destroy() can return a Boolean to indicate whether > > a domain is destroyed. > > It could be a solution but doesn't feel that common for a clean > function to have a return value indicating a special case. Maybe > passing in "" so that we can check if it's NULL after? It is harmless to do every time, it just burns a few CPU cycles on a slow path. We don't need complexity to optmize it. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
On Fri, Jun 17, 2022 at 02:53:13AM +, Tian, Kevin wrote:
> > > ...
> > > > - if (resv_msi) {
> > > > + if (resv_msi && !domain->msi_cookie) {
> > > > ret = iommu_get_msi_cookie(domain->domain,
> > > > resv_msi_base);
> > > > if (ret && ret != -ENODEV)
> > > > goto out_detach;
> > > > + domain->msi_cookie = true;
> > > > }
> > >
> > > why not moving to alloc_attach_domain() then no need for the new
> > > domain field? It's required only when a new domain is allocated.
> >
> > When reusing an existing domain that doesn't have an msi_cookie,
> > we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> > not limited to a new domain.
>
> Looks msi_cookie requirement is per platform (currently only
> for smmu. see arm_smmu_get_resv_regions()). If there is
> no mixed case then above check is not required.
Do you mean "reusing existing domain" for the "mixed case"?
> But let's hear whether Robin has a different thought here.
Yea, sure.
> > > > - iommu_domain_free(domain->domain);
> > > > - list_del(>next);
> > > > - kfree(domain);
> > > > - vfio_iommu_aper_expand(iommu, _copy);
> > >
> > > Previously the aperture is adjusted when a domain is freed...
> > >
> > > > - vfio_update_pgsize_bitmap(iommu);
> > > > - }
> > > > - /*
> > > > - * Removal of a group without dirty tracking may allow
> > > > - * the iommu scope to be promoted.
> > > > - */
> > > > - if (!group->pinned_page_dirty_scope) {
> > > > - iommu->num_non_pinned_groups--;
> > > > - if (iommu->dirty_page_tracking)
> > > > - vfio_iommu_populate_bitmap_full(iommu);
> > > > - }
> > > > + vfio_iommu_detach_destroy_domain(domain, iommu,
> > > > group);
> > > > kfree(group);
> > > > break;
> > > > }
> > > >
> > > > + vfio_iommu_aper_expand(iommu, _copy);
> > >
> > > but now it's done for every group detach. The aperture is decided
> > > by domain geometry which is not affected by attached groups.
> >
> > Yea, I've noticed this part. Actually Jason did this change for
> > simplicity, and I think it'd be safe to do so?
>
> Perhaps detach_destroy() can return a Boolean to indicate whether
> a domain is destroyed.
It could be a solution but doesn't feel that common for a clean
function to have a return value indicating a special case. Maybe
passing in "" so that we can check if it's NULL after?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
> From: Nicolin Chen
> Sent: Friday, June 17, 2022 6:41 AM
>
> > ...
> > > - if (resv_msi) {
> > > + if (resv_msi && !domain->msi_cookie) {
> > > ret = iommu_get_msi_cookie(domain->domain,
> > > resv_msi_base);
> > > if (ret && ret != -ENODEV)
> > > goto out_detach;
> > > + domain->msi_cookie = true;
> > > }
> >
> > why not moving to alloc_attach_domain() then no need for the new
> > domain field? It's required only when a new domain is allocated.
>
> When reusing an existing domain that doesn't have an msi_cookie,
> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> not limited to a new domain.
Looks msi_cookie requirement is per platform (currently only
for smmu. see arm_smmu_get_resv_regions()). If there is
no mixed case then above check is not required.
But let's hear whether Robin has a different thought here.
>
> > ...
> > > - if (list_empty(>group_list)) {
> > > - if (list_is_singular(>domain_list)) {
> > > - if (list_empty(
> > > >emulated_iommu_groups)) {
> > > - WARN_ON(iommu->notifier.head);
> > > -
> > > vfio_iommu_unmap_unpin_all(iommu);
> > > - } else {
> > > -
> > > vfio_iommu_unmap_unpin_reaccount(iommu);
> > > - }
> > > - }
> > > - iommu_domain_free(domain->domain);
> > > - list_del(>next);
> > > - kfree(domain);
> > > - vfio_iommu_aper_expand(iommu, _copy);
> >
> > Previously the aperture is adjusted when a domain is freed...
> >
> > > - vfio_update_pgsize_bitmap(iommu);
> > > - }
> > > - /*
> > > - * Removal of a group without dirty tracking may allow
> > > - * the iommu scope to be promoted.
> > > - */
> > > - if (!group->pinned_page_dirty_scope) {
> > > - iommu->num_non_pinned_groups--;
> > > - if (iommu->dirty_page_tracking)
> > > - vfio_iommu_populate_bitmap_full(iommu);
> > > - }
> > > + vfio_iommu_detach_destroy_domain(domain, iommu,
> > > group);
> > > kfree(group);
> > > break;
> > > }
> > >
> > > + vfio_iommu_aper_expand(iommu, _copy);
> >
> > but now it's done for every group detach. The aperture is decided
> > by domain geometry which is not affected by attached groups.
>
> Yea, I've noticed this part. Actually Jason did this change for
> simplicity, and I think it'd be safe to do so?
Perhaps detach_destroy() can return a Boolean to indicate whether
a domain is destroyed.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
On Thu, Jun 16, 2022 at 07:08:10AM +, Tian, Kevin wrote:
> ...
> > +static struct vfio_domain *
> > +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> > *iommu,
> > +struct vfio_iommu_group *group)
> > +{
> > + struct iommu_domain *new_domain;
> > + struct vfio_domain *domain;
> > + int ret = 0;
> > +
> > + /* Try to match an existing compatible domain */
> > + list_for_each_entry (domain, >domain_list, next) {
> > + ret = iommu_attach_group(domain->domain, group-
> > >iommu_group);
> > + if (ret == -EMEDIUMTYPE)
> > + continue;
>
> Probably good to add one line comment here for what EMEDIUMTYPE
> represents. It's not a widely-used retry type like EAGAIN. A comment
> can save the time of digging out the fact by jumping to iommu file.
Sure. I can add that.
> ...
> > - if (resv_msi) {
> > + if (resv_msi && !domain->msi_cookie) {
> > ret = iommu_get_msi_cookie(domain->domain,
> > resv_msi_base);
> > if (ret && ret != -ENODEV)
> > goto out_detach;
> > + domain->msi_cookie = true;
> > }
>
> why not moving to alloc_attach_domain() then no need for the new
> domain field? It's required only when a new domain is allocated.
When reusing an existing domain that doesn't have an msi_cookie,
we can do iommu_get_msi_cookie() if resv_msi is found. So it is
not limited to a new domain.
> ...
> > - if (list_empty(>group_list)) {
> > - if (list_is_singular(>domain_list)) {
> > - if (list_empty(
> > >emulated_iommu_groups)) {
> > - WARN_ON(iommu->notifier.head);
> > -
> > vfio_iommu_unmap_unpin_all(iommu);
> > - } else {
> > -
> > vfio_iommu_unmap_unpin_reaccount(iommu);
> > - }
> > - }
> > - iommu_domain_free(domain->domain);
> > - list_del(>next);
> > - kfree(domain);
> > - vfio_iommu_aper_expand(iommu, _copy);
>
> Previously the aperture is adjusted when a domain is freed...
>
> > - vfio_update_pgsize_bitmap(iommu);
> > - }
> > - /*
> > - * Removal of a group without dirty tracking may allow
> > - * the iommu scope to be promoted.
> > - */
> > - if (!group->pinned_page_dirty_scope) {
> > - iommu->num_non_pinned_groups--;
> > - if (iommu->dirty_page_tracking)
> > - vfio_iommu_populate_bitmap_full(iommu);
> > - }
> > + vfio_iommu_detach_destroy_domain(domain, iommu,
> > group);
> > kfree(group);
> > break;
> > }
> >
> > + vfio_iommu_aper_expand(iommu, _copy);
>
> but now it's done for every group detach. The aperture is decided
> by domain geometry which is not affected by attached groups.
Yea, I've noticed this part. Actually Jason did this change for
simplicity, and I think it'd be safe to do so?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
>
> Un-inline the domain specific logic from the attach/detach_group ops into
> two paired functions vfio_iommu_alloc_attach_domain() and
> vfio_iommu_detach_destroy_domain() that strictly deal with creating and
> destroying struct vfio_domains.
>
> Add the logic to check for EMEDIUMTYPE return code of
> iommu_attach_group()
> and avoid the extra domain allocations and attach/detach sequences of the
> old code. This allows properly detecting an actual attach error, like
> -ENOMEM, vs treating all attach errors as an incompatible domain.
>
> Co-developed-by: Jason Gunthorpe
> Signed-off-by: Jason Gunthorpe
> Signed-off-by: Nicolin Chen
> ---
> drivers/vfio/vfio_iommu_type1.c | 298 +---
> 1 file changed, 163 insertions(+), 135 deletions(-)
>
...
> +static struct vfio_domain *
> +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> *iommu,
> +struct vfio_iommu_group *group)
> +{
> + struct iommu_domain *new_domain;
> + struct vfio_domain *domain;
> + int ret = 0;
> +
> + /* Try to match an existing compatible domain */
> + list_for_each_entry (domain, >domain_list, next) {
> + ret = iommu_attach_group(domain->domain, group-
> >iommu_group);
> + if (ret == -EMEDIUMTYPE)
> + continue;
Probably good to add one line comment here for what EMEDIUMTYPE
represents. It's not a widely-used retry type like EAGAIN. A comment
can save the time of digging out the fact by jumping to iommu file.
...
> - if (resv_msi) {
> + if (resv_msi && !domain->msi_cookie) {
> ret = iommu_get_msi_cookie(domain->domain,
> resv_msi_base);
> if (ret && ret != -ENODEV)
> goto out_detach;
> + domain->msi_cookie = true;
> }
why not moving to alloc_attach_domain() then no need for the new
domain field? It's required only when a new domain is allocated.
...
> - if (list_empty(>group_list)) {
> - if (list_is_singular(>domain_list)) {
> - if (list_empty(
> >emulated_iommu_groups)) {
> - WARN_ON(iommu->notifier.head);
> -
> vfio_iommu_unmap_unpin_all(iommu);
> - } else {
> -
> vfio_iommu_unmap_unpin_reaccount(iommu);
> - }
> - }
> - iommu_domain_free(domain->domain);
> - list_del(>next);
> - kfree(domain);
> - vfio_iommu_aper_expand(iommu, _copy);
Previously the aperture is adjusted when a domain is freed...
> - vfio_update_pgsize_bitmap(iommu);
> - }
> - /*
> - * Removal of a group without dirty tracking may allow
> - * the iommu scope to be promoted.
> - */
> - if (!group->pinned_page_dirty_scope) {
> - iommu->num_non_pinned_groups--;
> - if (iommu->dirty_page_tracking)
> - vfio_iommu_populate_bitmap_full(iommu);
> - }
> + vfio_iommu_detach_destroy_domain(domain, iommu,
> group);
> kfree(group);
> break;
> }
>
> + vfio_iommu_aper_expand(iommu, _copy);
but now it's done for every group detach. The aperture is decided
by domain geometry which is not affected by attached groups.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
Un-inline the domain specific logic from the attach/detach_group ops into
two paired functions vfio_iommu_alloc_attach_domain() and
vfio_iommu_detach_destroy_domain() that strictly deal with creating and
destroying struct vfio_domains.
Add the logic to check for EMEDIUMTYPE return code of iommu_attach_group()
and avoid the extra domain allocations and attach/detach sequences of the
old code. This allows properly detecting an actual attach error, like
-ENOMEM, vs treating all attach errors as an incompatible domain.
Co-developed-by: Jason Gunthorpe
Signed-off-by: Jason Gunthorpe
Signed-off-by: Nicolin Chen
---
drivers/vfio/vfio_iommu_type1.c | 298 +---
1 file changed, 163 insertions(+), 135 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 573caf320788..5986c68e59ee 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -86,6 +86,7 @@ struct vfio_domain {
struct list_headgroup_list;
boolfgsp : 1; /* Fine-grained super pages */
boolenforce_cache_coherency : 1;
+ boolmsi_cookie : 1;
};
struct vfio_dma {
@@ -2153,12 +2154,163 @@ static void vfio_iommu_iova_insert_copy(struct
vfio_iommu *iommu,
list_splice_tail(iova_copy, iova);
}
+static struct vfio_domain *
+vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu *iommu,
+ struct vfio_iommu_group *group)
+{
+ struct iommu_domain *new_domain;
+ struct vfio_domain *domain;
+ int ret = 0;
+
+ /* Try to match an existing compatible domain */
+ list_for_each_entry (domain, >domain_list, next) {
+ ret = iommu_attach_group(domain->domain, group->iommu_group);
+ if (ret == -EMEDIUMTYPE)
+ continue;
+ if (ret)
+ return ERR_PTR(ret);
+ goto done;
+ }
+
+ new_domain = iommu_domain_alloc(bus);
+ if (!new_domain)
+ return ERR_PTR(-EIO);
+
+ if (iommu->nesting) {
+ ret = iommu_enable_nesting(new_domain);
+ if (ret)
+ goto out_free_iommu_domain;
+ }
+
+ ret = iommu_attach_group(new_domain, group->iommu_group);
+ if (ret)
+ goto out_free_iommu_domain;
+
+ domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto out_detach;
+ }
+
+ domain->domain = new_domain;
+ vfio_test_domain_fgsp(domain);
+
+ /*
+* If the IOMMU can block non-coherent operations (ie PCIe TLPs with
+* no-snoop set) then VFIO always turns this feature on because on Intel
+* platforms it optimizes KVM to disable wbinvd emulation.
+*/
+ if (new_domain->ops->enforce_cache_coherency)
+ domain->enforce_cache_coherency =
+ new_domain->ops->enforce_cache_coherency(new_domain);
+
+ /* replay mappings on new domains */
+ ret = vfio_iommu_replay(iommu, domain);
+ if (ret)
+ goto out_free_domain;
+
+ INIT_LIST_HEAD(>group_list);
+ list_add(>next, >domain_list);
+ vfio_update_pgsize_bitmap(iommu);
+
+done:
+ list_add(>next, >group_list);
+
+ /*
+* An iommu backed group can dirty memory directly and therefore
+* demotes the iommu scope until it declares itself dirty tracking
+* capable via the page pinning interface.
+*/
+ iommu->num_non_pinned_groups++;
+
+ return domain;
+
+out_free_domain:
+ kfree(domain);
+out_detach:
+ iommu_detach_group(new_domain, group->iommu_group);
+out_free_iommu_domain:
+ iommu_domain_free(new_domain);
+ return ERR_PTR(ret);
+}
+
+static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
+{
+ struct rb_node *node;
+
+ while ((node = rb_first(>dma_list)))
+ vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
+}
+
+static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
+{
+ struct rb_node *n, *p;
+
+ n = rb_first(>dma_list);
+ for (; n; n = rb_next(n)) {
+ struct vfio_dma *dma;
+ long locked = 0, unlocked = 0;
+
+ dma = rb_entry(n, struct vfio_dma, node);
+ unlocked += vfio_unmap_unpin(iommu, dma, false);
+ p = rb_first(>pfn_list);
+ for (; p; p = rb_next(p)) {
+ struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn,
+node);
+
+ if (!is_invalid_reserved_pfn(vpfn->pfn))
+ locked++;
+ }
+ vfio_lock_acct(dma, locked - unlocked, true);
+ }
+}
+
+static void
