| Message ID | a98e622f41d76b64f5a7d0c758d8bda5e8043013.1675320212.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add IO page table replacement support | expand |
On 2023/2/2 15:05, Nicolin Chen wrote: > +/** > + * iommu_group_replace_domain - replace the domain that a group is attached to > + * @new_domain: new IOMMU domain to replace with > + * @group: IOMMU group that will be attached to the new domain > + * > + * This API allows the group to switch domains without being forced to go to > + * the blocking domain in-between. > + * > + * If the attached domain is a core domain (e.g. a default_domain), it will act > + * just like the iommu_attach_group(). I am not following above two lines. Why and how could iommufd set a core domain to an iommu_group? > + */ > +int iommu_group_replace_domain(struct iommu_group *group, > + struct iommu_domain *new_domain) > +{ > + int ret; > + > + if (!new_domain) > + return -EINVAL; > + > + mutex_lock(&group->mutex); > + ret = __iommu_group_set_domain(group, new_domain); > + if (ret) { > + if (__iommu_group_set_domain(group, group->domain)) > + __iommu_group_set_core_domain(group); > + } > + mutex_unlock(&group->mutex); > + return ret; > +} > +EXPORT_SYMBOL_NS_GPL(iommu_group_replace_domain, IOMMUFD_INTERNAL); Best regards, baolu
On Thu, Feb 02, 2023 at 06:21:20PM +0800, Baolu Lu wrote: > External email: Use caution opening links or attachments > > > On 2023/2/2 15:05, Nicolin Chen wrote: > > +/** > > + * iommu_group_replace_domain - replace the domain that a group is attached to > > + * @new_domain: new IOMMU domain to replace with > > + * @group: IOMMU group that will be attached to the new domain > > + * > > + * This API allows the group to switch domains without being forced to go to > > + * the blocking domain in-between. > > + * > > + * If the attached domain is a core domain (e.g. a default_domain), it will act > > + * just like the iommu_attach_group(). > > I am not following above two lines. Why and how could iommufd set a > core domain to an iommu_group? Perhaps this isn't the best narrative. What it's supposed to say is that this function acts as an iommu_attach_group() call if the device is "detached", yet we have changed the semantics about the word "detach". So, what should the correct way to write such a note? Thanks Nic
On 2023/2/3 3:14, Nicolin Chen wrote: > On Thu, Feb 02, 2023 at 06:21:20PM +0800, Baolu Lu wrote: >> External email: Use caution opening links or attachments >> >> >> On 2023/2/2 15:05, Nicolin Chen wrote: >>> +/** >>> + * iommu_group_replace_domain - replace the domain that a group is attached to >>> + * @new_domain: new IOMMU domain to replace with >>> + * @group: IOMMU group that will be attached to the new domain >>> + * >>> + * This API allows the group to switch domains without being forced to go to >>> + * the blocking domain in-between. >>> + * >>> + * If the attached domain is a core domain (e.g. a default_domain), it will act >>> + * just like the iommu_attach_group(). >> I am not following above two lines. Why and how could iommufd set a >> core domain to an iommu_group? > Perhaps this isn't the best narrative. What it's supposed to say > is that this function acts as an iommu_attach_group() call if the > device is "detached", yet we have changed the semantics about the > word "detach". So, what should the correct way to write such a > note? How could this interface be used as detaching a domain from a group? Even it could be used, doesn't it act as an iommu_detach_group()? Best regards, baolu
On Fri, Feb 03, 2023 at 09:33:44AM +0800, Baolu Lu wrote: > External email: Use caution opening links or attachments > > > On 2023/2/3 3:14, Nicolin Chen wrote: > > On Thu, Feb 02, 2023 at 06:21:20PM +0800, Baolu Lu wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > On 2023/2/2 15:05, Nicolin Chen wrote: > > > > +/** > > > > + * iommu_group_replace_domain - replace the domain that a group is attached to > > > > + * @new_domain: new IOMMU domain to replace with > > > > + * @group: IOMMU group that will be attached to the new domain > > > > + * > > > > + * This API allows the group to switch domains without being forced to go to > > > > + * the blocking domain in-between. > > > > + * > > > > + * If the attached domain is a core domain (e.g. a default_domain), it will act > > > > + * just like the iommu_attach_group(). > > > I am not following above two lines. Why and how could iommufd set a > > > core domain to an iommu_group? > > Perhaps this isn't the best narrative. What it's supposed to say > > is that this function acts as an iommu_attach_group() call if the > > device is "detached", yet we have changed the semantics about the > > word "detach". So, what should the correct way to write such a > > note? > > How could this interface be used as detaching a domain from a group? > Even it could be used, doesn't it act as an iommu_detach_group()? No. I didn't say that. It doesn't act as detach(), but attach() when a device is already "detached". The original statement is saying, "if the attached domain is a core domain", i.e. the device is detach()-ed, "it will act just like the iommu_attach_group()". Thanks Nic
On 2023/2/3 9:41, Nicolin Chen wrote: > On Fri, Feb 03, 2023 at 09:33:44AM +0800, Baolu Lu wrote: >> External email: Use caution opening links or attachments >> >> >> On 2023/2/3 3:14, Nicolin Chen wrote: >>> On Thu, Feb 02, 2023 at 06:21:20PM +0800, Baolu Lu wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> On 2023/2/2 15:05, Nicolin Chen wrote: >>>>> +/** >>>>> + * iommu_group_replace_domain - replace the domain that a group is attached to >>>>> + * @new_domain: new IOMMU domain to replace with >>>>> + * @group: IOMMU group that will be attached to the new domain >>>>> + * >>>>> + * This API allows the group to switch domains without being forced to go to >>>>> + * the blocking domain in-between. >>>>> + * >>>>> + * If the attached domain is a core domain (e.g. a default_domain), it will act >>>>> + * just like the iommu_attach_group(). >>>> I am not following above two lines. Why and how could iommufd set a >>>> core domain to an iommu_group? >>> Perhaps this isn't the best narrative. What it's supposed to say >>> is that this function acts as an iommu_attach_group() call if the >>> device is "detached", yet we have changed the semantics about the >>> word "detach". So, what should the correct way to write such a >>> note? >> How could this interface be used as detaching a domain from a group? >> Even it could be used, doesn't it act as an iommu_detach_group()? > No. I didn't say that. It doesn't act as detach(), but attach() > when a device is already "detached". > > The original statement is saying, "if the attached domain is a > core domain", i.e. the device is detach()-ed, "it will act just > like the iommu_attach_group()". Oh! My bad. I misunderstood it. Sorry for the noise. :-) Best regards, baolu
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Thursday, February 2, 2023 3:05 PM > > All drivers are already required to support changing between active > UNMANAGED domains when using their attach_dev ops. All drivers which don't have *broken* UNMANAGED domain? > > +/** > + * iommu_group_replace_domain - replace the domain that a group is > attached to > + * @new_domain: new IOMMU domain to replace with > + * @group: IOMMU group that will be attached to the new domain > + * > + * This API allows the group to switch domains without being forced to go to > + * the blocking domain in-between. > + * > + * If the attached domain is a core domain (e.g. a default_domain), it will act > + * just like the iommu_attach_group(). I think you meant "the currently-attached domain", which implies a 'detached' state as you replied to Baolu. > + */ > +int iommu_group_replace_domain(struct iommu_group *group, > + struct iommu_domain *new_domain) what actual value does 'replace' give us? It's just a wrapper of __iommu_group_set_domain() then calling it set_domain is probably clearer. You can clarify the 'replace' behavior in the comment. > +{ > + int ret; > + > + if (!new_domain) > + return -EINVAL; > + > + mutex_lock(&group->mutex); > + ret = __iommu_group_set_domain(group, new_domain); > + if (ret) { > + if (__iommu_group_set_domain(group, group->domain)) > + __iommu_group_set_core_domain(group); > + } Can you elaborate the error handling here? Ideally if __iommu_group_set_domain() fails then group->domain shouldn't be changed. Why do we need further housekeeping here?
On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Thursday, February 2, 2023 3:05 PM > > > > All drivers are already required to support changing between active > > UNMANAGED domains when using their attach_dev ops. > > All drivers which don't have *broken* UNMANAGED domain? No, all drivers.. It has always been used by VFIO. > > + */ > > +int iommu_group_replace_domain(struct iommu_group *group, > > + struct iommu_domain *new_domain) > > what actual value does 'replace' give us? It's just a wrapper of > __iommu_group_set_domain() then calling it set_domain is > probably clearer. You can clarify the 'replace' behavior in the > comment. As the APIs are setup: attach demands that no domain is currently set (eg the domain must be blocking) replace permits the domain to be currently set 'set' vs 'attach' is really unclear what the intended difference is. We could also address this by simply removing the protection from attach, but it is not so clear if that is safe for the few users. > > +{ > > + int ret; > > + > > + if (!new_domain) > > + return -EINVAL; > > + > > + mutex_lock(&group->mutex); > > + ret = __iommu_group_set_domain(group, new_domain); > > + if (ret) { > > + if (__iommu_group_set_domain(group, group->domain)) > > + __iommu_group_set_core_domain(group); > > + } > > Can you elaborate the error handling here? Ideally if > __iommu_group_set_domain() fails then group->domain shouldn't > be changed. That isn't what it implements though. The internal helper leaves things in a mess, it is for the caller to fix it, and it depends on the caller what that means. In this case the API cannot retain a hidden reference to the new domain, so it must be purged, one way or another. Jason
On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > +/** > > + * iommu_group_replace_domain - replace the domain that a group is > > attached to > > + * @new_domain: new IOMMU domain to replace with > > + * @group: IOMMU group that will be attached to the new domain > > + * > > + * This API allows the group to switch domains without being forced to go to > > + * the blocking domain in-between. > > + * > > + * If the attached domain is a core domain (e.g. a default_domain), it will act > > + * just like the iommu_attach_group(). > > I think you meant "the currently-attached domain", which implies a > 'detached' state as you replied to Baolu. Hmm, I don't see an implication, since we only have either "the attached domain" or "a new domain" in the context? With that being said, I can add "currently" in v2: * If the currently attached domain is a core domain (e.g. a default_domain), * it will act just like the iommu_attach_group(). Thanks Nic
On Fri, Feb 03, 2023 at 11:03:20AM -0400, Jason Gunthorpe wrote: > > > + */ > > > +int iommu_group_replace_domain(struct iommu_group *group, > > > + struct iommu_domain *new_domain) > > > > what actual value does 'replace' give us? It's just a wrapper of > > __iommu_group_set_domain() then calling it set_domain is > > probably clearer. You can clarify the 'replace' behavior in the > > comment. > > As the APIs are setup: > > attach demands that no domain is currently set (eg the domain must be blocking) > > replace permits the domain to be currently set > > 'set' vs 'attach' is really unclear what the intended difference is. > > We could also address this by simply removing the protection from > attach, but it is not so clear if that is safe for the few users. I can add a couple of lines to the commit message to make things clear. > > > +{ > > > + int ret; > > > + > > > + if (!new_domain) > > > + return -EINVAL; > > > + > > > + mutex_lock(&group->mutex); > > > + ret = __iommu_group_set_domain(group, new_domain); > > > + if (ret) { > > > + if (__iommu_group_set_domain(group, group->domain)) > > > + __iommu_group_set_core_domain(group); > > > + } > > > > Can you elaborate the error handling here? Ideally if > > __iommu_group_set_domain() fails then group->domain shouldn't > > be changed. > > That isn't what it implements though. The internal helper leaves > things in a mess, it is for the caller to fix it, and it depends on > the caller what that means. > > In this case the API cannot retain a hidden reference to the new > domain, so it must be purged, one way or another. Considering it is a bit different than the existing APIs like iommu_attach_group(), perhaps I should add a line of comments in the fallback routine. Thanks Nic
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Saturday, February 4, 2023 1:45 AM > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > > > +/** > > > + * iommu_group_replace_domain - replace the domain that a group is > > > attached to > > > + * @new_domain: new IOMMU domain to replace with > > > + * @group: IOMMU group that will be attached to the new domain > > > + * > > > + * This API allows the group to switch domains without being forced to > go to > > > + * the blocking domain in-between. > > > + * > > > + * If the attached domain is a core domain (e.g. a default_domain), it will > act > > > + * just like the iommu_attach_group(). > > > > I think you meant "the currently-attached domain", which implies a > > 'detached' state as you replied to Baolu. > > Hmm, I don't see an implication, since we only have either > "the attached domain" or "a new domain" in the context? probably just me reading it as "the newly attached domain".
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, February 3, 2023 11:03 PM > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > Sent: Thursday, February 2, 2023 3:05 PM > > > > > > All drivers are already required to support changing between active > > > UNMANAGED domains when using their attach_dev ops. > > > > All drivers which don't have *broken* UNMANAGED domain? > > No, all drivers.. It has always been used by VFIO. existing iommu_attach_group() doesn't support changing between two UNMANAGED domains. only from default->unmanaged or blocking->unmanaged. Above statement is true only if this series is based on your unmerged work removing DMA domain types. > > > > +{ > > > + int ret; > > > + > > > + if (!new_domain) > > > + return -EINVAL; > > > + > > > + mutex_lock(&group->mutex); > > > + ret = __iommu_group_set_domain(group, new_domain); > > > + if (ret) { > > > + if (__iommu_group_set_domain(group, group->domain)) > > > + __iommu_group_set_core_domain(group); > > > + } > > > > Can you elaborate the error handling here? Ideally if > > __iommu_group_set_domain() fails then group->domain shouldn't > > be changed. > > That isn't what it implements though. The internal helper leaves > things in a mess, it is for the caller to fix it, and it depends on > the caller what that means. I didn't see any warning of the mess and the caller's responsibility in __iommu_group_set_domain(). Can it be documented clearly so if someone wants to add a new caller on it he can clearly know what to do? and why doesn't iommu_attach_group() need to do anything when an attach attempt fails? In the end it calls the same iommu_group_do_attach_device() as __iommu_group_set_domain() does. btw looking at the code __iommu_group_set_domain(): * Note that this is called in error unwind paths, attaching to a * domain that has already been attached cannot fail. */ ret = __iommu_group_for_each_dev(group, new_domain, iommu_group_do_attach_device); with that we don't need fall back to core domain in above error unwinding per this comment. > > In this case the API cannot retain a hidden reference to the new > domain, so it must be purged, one way or another. > Could you elaborate where the hidden reference is retained?
On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Friday, February 3, 2023 11:03 PM > > > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > Sent: Thursday, February 2, 2023 3:05 PM > > > > > > > > All drivers are already required to support changing between active > > > > UNMANAGED domains when using their attach_dev ops. > > > > > > All drivers which don't have *broken* UNMANAGED domain? > > > > No, all drivers.. It has always been used by VFIO. > > existing iommu_attach_group() doesn't support changing between > two UNMANAGED domains. only from default->unmanaged or > blocking->unmanaged. Yes, but before we added the blocking domains VFIO was changing between unmanaged domains. Blocking domains are so new that no driver could have suddenly started to depend on this. > > > Can you elaborate the error handling here? Ideally if > > > __iommu_group_set_domain() fails then group->domain shouldn't > > > be changed. > > > > That isn't what it implements though. The internal helper leaves > > things in a mess, it is for the caller to fix it, and it depends on > > the caller what that means. > > I didn't see any warning of the mess and the caller's responsibility > in __iommu_group_set_domain(). Can it be documented clearly > so if someone wants to add a new caller on it he can clearly know > what to do? That would be nice.. > and why doesn't iommu_attach_group() need to do anything > when an attach attempt fails? In the end it calls the same > iommu_group_do_attach_device() as __iommu_group_set_domain() > does. That's a bug for sure. > btw looking at the code __iommu_group_set_domain(): > > * Note that this is called in error unwind paths, attaching to a > * domain that has already been attached cannot fail. > */ > ret = __iommu_group_for_each_dev(group, new_domain, > iommu_group_do_attach_device); > > with that we don't need fall back to core domain in above error > unwinding per this comment. That does make some sense. I tried to make a patch to consolidate all this error handling once, that would be the better way to approach this. > > In this case the API cannot retain a hidden reference to the new > > domain, so it must be purged, one way or another. > > Could you elaborate where the hidden reference is retained? Inside the driver, it can keep track of the domain pointer if attach_dev succeeds Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Monday, February 6, 2023 9:25 PM > > On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Friday, February 3, 2023 11:03 PM > > > > > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > > Sent: Thursday, February 2, 2023 3:05 PM > > > > > > > > > > All drivers are already required to support changing between active > > > > > UNMANAGED domains when using their attach_dev ops. > > > > > > > > All drivers which don't have *broken* UNMANAGED domain? > > > > > > No, all drivers.. It has always been used by VFIO. > > > > existing iommu_attach_group() doesn't support changing between > > two UNMANAGED domains. only from default->unmanaged or > > blocking->unmanaged. > > Yes, but before we added the blocking domains VFIO was changing > between unmanaged domains. Blocking domains are so new that no driver > could have suddenly started to depend on this. In legacy VFIO unmanaged domain was 1:1 associated with vfio container. I didn't say how a group can switch between two containers w/o going through transition to/from the default domain, i.e. detach from 1st container and then attach to the 2nd. > > btw looking at the code __iommu_group_set_domain(): > > > > * Note that this is called in error unwind paths, attaching to a > > * domain that has already been attached cannot fail. > > */ > > ret = __iommu_group_for_each_dev(group, new_domain, > > iommu_group_do_attach_device); > > > > with that we don't need fall back to core domain in above error > > unwinding per this comment. > > That does make some sense. > > I tried to make a patch to consolidate all this error handling once, > that would be the better way to approach this. that would be good. > > > > In this case the API cannot retain a hidden reference to the new > > > domain, so it must be purged, one way or another. > > > > Could you elaborate where the hidden reference is retained? > > Inside the driver, it can keep track of the domain pointer if > attach_dev succeeds > Are you referring to no error unwinding in __iommu_group_for_each_dev() so if it is failed some devices may have attach_dev succeeds then simply recovering group->domain in __iommu_attach_group() is insufficient?
On Tue, Feb 07, 2023 at 12:32:50AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Monday, February 6, 2023 9:25 PM > > > > On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote: > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > Sent: Friday, February 3, 2023 11:03 PM > > > > > > > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > > > Sent: Thursday, February 2, 2023 3:05 PM > > > > > > > > > > > > All drivers are already required to support changing between active > > > > > > UNMANAGED domains when using their attach_dev ops. > > > > > > > > > > All drivers which don't have *broken* UNMANAGED domain? > > > > > > > > No, all drivers.. It has always been used by VFIO. > > > > > > existing iommu_attach_group() doesn't support changing between > > > two UNMANAGED domains. only from default->unmanaged or > > > blocking->unmanaged. > > > > Yes, but before we added the blocking domains VFIO was changing > > between unmanaged domains. Blocking domains are so new that no driver > > could have suddenly started to depend on this. > > In legacy VFIO unmanaged domain was 1:1 associated with vfio > container. I didn't say how a group can switch between two > containers w/o going through transition to/from the default > domain, i.e. detach from 1st container and then attach to the 2nd. Yes, in the past we went through the default domain which is basically another unmanaged domain type. So unmanaged -> unmanaged is OK. > > Inside the driver, it can keep track of the domain pointer if > > attach_dev succeeds > > Are you referring to no error unwinding in __iommu_group_for_each_dev() > so if it is failed some devices may have attach_dev succeeds then simply > recovering group->domain in __iommu_attach_group() is insufficient? Yes Jason
On Mon, Feb 06, 2023 at 09:25:17AM -0400, Jason Gunthorpe wrote: > > > > Can you elaborate the error handling here? Ideally if > > > > __iommu_group_set_domain() fails then group->domain shouldn't > > > > be changed. > > > > > > That isn't what it implements though. The internal helper leaves > > > things in a mess, it is for the caller to fix it, and it depends on > > > the caller what that means. > > > > I didn't see any warning of the mess and the caller's responsibility > > in __iommu_group_set_domain(). Can it be documented clearly > > so if someone wants to add a new caller on it he can clearly know > > what to do? > > That would be nice.. I'd expect the doc to come with some other patch/series than this replace series, so I think we should be fine without adding a line of comments in this patch? > > btw looking at the code __iommu_group_set_domain(): > > > > * Note that this is called in error unwind paths, attaching to a > > * domain that has already been attached cannot fail. > > */ > > ret = __iommu_group_for_each_dev(group, new_domain, > > iommu_group_do_attach_device); > > > > with that we don't need fall back to core domain in above error > > unwinding per this comment. > > That does make some sense. > > I tried to make a patch to consolidate all this error handling once, > that would be the better way to approach this. Then, I'll drop the core-domain line. Combining my reply above: + mutex_lock(&group->mutex); + ret = __iommu_group_set_domain(group, new_domain); + if (ret) + __iommu_group_set_domain(group, group->domain); + mutex_unlock(&group->mutex); Will wrap things up and send v2 today. Thanks Nic
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, February 7, 2023 8:23 PM > > On Tue, Feb 07, 2023 at 12:32:50AM +0000, Tian, Kevin wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Monday, February 6, 2023 9:25 PM > > > > > > On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote: > > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > > Sent: Friday, February 3, 2023 11:03 PM > > > > > > > > > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > > > > Sent: Thursday, February 2, 2023 3:05 PM > > > > > > > > > > > > > > All drivers are already required to support changing between > active > > > > > > > UNMANAGED domains when using their attach_dev ops. > > > > > > > > > > > > All drivers which don't have *broken* UNMANAGED domain? > > > > > > > > > > No, all drivers.. It has always been used by VFIO. > > > > > > > > existing iommu_attach_group() doesn't support changing between > > > > two UNMANAGED domains. only from default->unmanaged or > > > > blocking->unmanaged. > > > > > > Yes, but before we added the blocking domains VFIO was changing > > > between unmanaged domains. Blocking domains are so new that no > driver > > > could have suddenly started to depend on this. > > > > In legacy VFIO unmanaged domain was 1:1 associated with vfio > > container. I didn't say how a group can switch between two > > containers w/o going through transition to/from the default > > domain, i.e. detach from 1st container and then attach to the 2nd. > > Yes, in the past we went through the default domain which is basically > another unmanaged domain type. So unmanaged -> unmanaged is OK. > it's right in concept but confusing in current context whether DMA domain still has its own type. That's why I replied earlier the statement makes more sense after your patch which cleans up the domain type is merged.
On Wed, Feb 08, 2023 at 04:25:58AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Tuesday, February 7, 2023 8:23 PM > > > > On Tue, Feb 07, 2023 at 12:32:50AM +0000, Tian, Kevin wrote: > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > Sent: Monday, February 6, 2023 9:25 PM > > > > > > > > On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote: > > > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > > > Sent: Friday, February 3, 2023 11:03 PM > > > > > > > > > > > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote: > > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > > > > > Sent: Thursday, February 2, 2023 3:05 PM > > > > > > > > > > > > > > > > All drivers are already required to support changing between > > active > > > > > > > > UNMANAGED domains when using their attach_dev ops. > > > > > > > > > > > > > > All drivers which don't have *broken* UNMANAGED domain? > > > > > > > > > > > > No, all drivers.. It has always been used by VFIO. > > > > > > > > > > existing iommu_attach_group() doesn't support changing between > > > > > two UNMANAGED domains. only from default->unmanaged or > > > > > blocking->unmanaged. > > > > > > > > Yes, but before we added the blocking domains VFIO was changing > > > > between unmanaged domains. Blocking domains are so new that no > > driver > > > > could have suddenly started to depend on this. > > > > > > In legacy VFIO unmanaged domain was 1:1 associated with vfio > > > container. I didn't say how a group can switch between two > > > containers w/o going through transition to/from the default > > > domain, i.e. detach from 1st container and then attach to the 2nd. > > > > Yes, in the past we went through the default domain which is basically > > another unmanaged domain type. So unmanaged -> unmanaged is OK. > > > > it's right in concept but confusing in current context whether DMA > domain still has its own type. That's why I replied earlier the statement > makes more sense after your patch which cleans up the domain type > is merged. We are just reasoning about why the existing drivers are safe with this - and my draft patch to remove DMA simply demonstrates that DMA == UNMANAGED, and the above reasoning about VFIO in the past demonstrates that this has historically be expected of drivers. So I'm not so worried about patch order as long as things do work Jason
diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h index 9e1497027cff..b546795a7e49 100644 --- a/drivers/iommu/iommu-priv.h +++ b/drivers/iommu/iommu-priv.h @@ -15,4 +15,8 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev) */ return dev->iommu->iommu_dev->ops; } + +extern int iommu_group_replace_domain(struct iommu_group *group, + struct iommu_domain *new_domain); + #endif /* __LINUX_IOMMU_PRIV_H */ diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index a18b7f1a4e6e..c35da7a5c0d4 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2151,6 +2151,36 @@ int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group) } EXPORT_SYMBOL_GPL(iommu_attach_group); +/** + * iommu_group_replace_domain - replace the domain that a group is attached to + * @new_domain: new IOMMU domain to replace with + * @group: IOMMU group that will be attached to the new domain + * + * This API allows the group to switch domains without being forced to go to + * the blocking domain in-between. + * + * If the attached domain is a core domain (e.g. a default_domain), it will act + * just like the iommu_attach_group(). + */ +int iommu_group_replace_domain(struct iommu_group *group, + struct iommu_domain *new_domain) +{ + int ret; + + if (!new_domain) + return -EINVAL; + + mutex_lock(&group->mutex); + ret = __iommu_group_set_domain(group, new_domain); + if (ret) { + if (__iommu_group_set_domain(group, group->domain)) + __iommu_group_set_core_domain(group); + } + mutex_unlock(&group->mutex); + return ret; +} +EXPORT_SYMBOL_NS_GPL(iommu_group_replace_domain, IOMMUFD_INTERNAL); + static int iommu_group_do_set_platform_dma(struct device *dev, void *data) { const struct iommu_ops *ops = dev_iommu_ops(dev);
qemu has a need to replace the translations associated with a domain when the guest does large-scale operations like switching between an IDENTITY domain and, say, dma-iommu.c. Currently, it does this by replacing all the mappings in a single domain, but this is very inefficient and means that domains have to be per-device rather than per-translation. Provide a high-level API to allow replacements of one domain with another. This is similar to a detach/attach cycle except it doesn't force the group to go to the blocking domain in-between. By removing this forced blocking domain the iommu driver has the opportunity to implement an atomic replacement of the domains to the greatest extent its hardware allows. Atomic replacement allows the qemu emulation of the viommu to be more complete, as real hardware has this ability. All drivers are already required to support changing between active UNMANAGED domains when using their attach_dev ops. This API is expected to be used by IOMMUFD, so add to the iommu-priv header and mark it as IOMMUFD_INTERNAL. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/iommu-priv.h | 4 ++++ drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)