| Message ID | 9-v2-51b9896e7862+8a8c-iommufd_alloc_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add iommufd physical device operations for replace and alloc hwpt | expand |
On 2023/3/8 8:35, Jason Gunthorpe wrote: > Logically the HWPT should have the coherency set properly for the device > that it is being created for when it is created. > > This was happening implicitly if the immediate_attach was set because > iommufd_hw_pagetable_attach() does it as the first thing. > > Do it unconditionally so !immediate_attach works properly. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommufd/device.c | 20 +++++------------- > drivers/iommu/iommufd/hw_pagetable.c | 27 +++++++++++++++++++++++++ > drivers/iommu/iommufd/iommufd_private.h | 1 + > 3 files changed, 33 insertions(+), 15 deletions(-) > > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c > index d6d5c2fcc33889..ddde14d6d1352c 100644 > --- a/drivers/iommu/iommufd/device.c > +++ b/drivers/iommu/iommufd/device.c > @@ -293,21 +293,11 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, > if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt) > return -EINVAL; > > - /* > - * Try to upgrade the domain we have, it is an iommu driver bug to > - * report IOMMU_CAP_ENFORCE_CACHE_COHERENCY but fail > - * enforce_cache_coherency when there are no devices attached to the > - * domain. > - */ > - if (idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency) { > - if (hwpt->domain->ops->enforce_cache_coherency) > - hwpt->enforce_cache_coherency = > - hwpt->domain->ops->enforce_cache_coherency( > - hwpt->domain); > - if (!hwpt->enforce_cache_coherency) { > - WARN_ON(list_empty(&idev->igroup->device_list)); > - return -EINVAL; > - } > + /* Try to upgrade the domain we have */ > + if (idev->enforce_cache_coherency) { > + rc = iommufd_hw_pagetable_enforce_cc(hwpt); > + if (rc) > + return rc; As "HWPT should have the coherency set properly for the device that it is being created for when it is created", is it an incompatible case if idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency ? If so, why not, if (idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency) return -EINVAL; ? Best regards, baolu > } > > rc = iopt_table_enforce_dev_resv_regions(&hwpt->ioas->iopt, idev->dev, > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c > index 566eba0cd9b917..2584f9038b29a2 100644 > --- a/drivers/iommu/iommufd/hw_pagetable.c > +++ b/drivers/iommu/iommufd/hw_pagetable.c > @@ -25,6 +25,20 @@ void iommufd_hw_pagetable_destroy(struct iommufd_object *obj) > refcount_dec(&hwpt->ioas->obj.users); > } > > +int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt) > +{ > + if (hwpt->enforce_cache_coherency) > + return 0; > + > + if (hwpt->domain->ops->enforce_cache_coherency) > + hwpt->enforce_cache_coherency = > + hwpt->domain->ops->enforce_cache_coherency( > + hwpt->domain); > + if (!hwpt->enforce_cache_coherency) > + return -EINVAL; > + return 0; > +} > + > /** > * iommufd_hw_pagetable_alloc() - Get an iommu_domain for a device > * @ictx: iommufd context > @@ -60,6 +74,19 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, > goto out_abort; > } > > + /* > + * Set the coherency mode before we do iopt_table_add_domain() as some > + * iommus have a per-PTE bit that controls it and need to decide before > + * doing any maps. It is an iommu driver bug to report > + * IOMMU_CAP_ENFORCE_CACHE_COHERENCY but fail enforce_cache_coherency on > + * a new domain. > + */ > + if (idev->enforce_cache_coherency) { > + rc = iommufd_hw_pagetable_enforce_cc(hwpt); > + if (WARN_ON(rc)) > + goto out_abort; > + } > + > mutex_lock(&idev->igroup->lock); > > /* > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h > index 8bf053f4d4a9ce..471a3fdff1e0b6 100644 > --- a/drivers/iommu/iommufd/iommufd_private.h > +++ b/drivers/iommu/iommufd/iommufd_private.h > @@ -254,6 +254,7 @@ struct iommufd_hw_pagetable { > struct iommufd_hw_pagetable * > iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, > struct iommufd_device *idev, bool immediate_attach); > +int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt); > int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, > struct iommufd_device *idev); > struct iommufd_hw_pagetable *
On Wed, Mar 08, 2023 at 09:04:47PM +0800, Baolu Lu wrote: > > + /* Try to upgrade the domain we have */ > > + if (idev->enforce_cache_coherency) { > > + rc = iommufd_hw_pagetable_enforce_cc(hwpt); > > + if (rc) > > + return rc; > > As "HWPT should have the coherency set properly for the device that it > is being created for when it is created", is it an incompatible case if > > idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency > > ? > > If so, why not, > > if (idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency) > return -EINVAL; In principle you can create a HWPT for device A and then later attach it to device B If B requires coherency then we try to upgrade the existing domain. This is an effort to try an minimize the number of domains that we need to have for simpe IOAS based cases where we don't want to spawn two auto domains Jason
On 3/8/23 11:06 PM, Jason Gunthorpe wrote: > On Wed, Mar 08, 2023 at 09:04:47PM +0800, Baolu Lu wrote: >>> + /* Try to upgrade the domain we have */ >>> + if (idev->enforce_cache_coherency) { >>> + rc = iommufd_hw_pagetable_enforce_cc(hwpt); >>> + if (rc) >>> + return rc; >> As "HWPT should have the coherency set properly for the device that it >> is being created for when it is created", is it an incompatible case if >> >> idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency >> >> ? >> >> If so, why not, >> >> if (idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency) >> return -EINVAL; > In principle you can create a HWPT for device A and then later attach > it to device B > > If B requires coherency then we try to upgrade the existing domain. > > This is an effort to try an minimize the number of domains that we > need to have for simpe IOAS based cases where we don't want to spawn > two auto domains OK. Clear to me now. Actually hwpt->enforce_cache_coherency makes sense only after it is attached to an idev. Best regards, baolu
On 3/8/23 8:35 AM, Jason Gunthorpe wrote: > Logically the HWPT should have the coherency set properly for the device > that it is being created for when it is created. > > This was happening implicitly if the immediate_attach was set because > iommufd_hw_pagetable_attach() does it as the first thing. > > Do it unconditionally so !immediate_attach works properly. > > Reviewed-by: Kevin Tian<kevin.tian@intel.com> > Signed-off-by: Jason Gunthorpe<jgg@nvidia.com> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index d6d5c2fcc33889..ddde14d6d1352c 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -293,21 +293,11 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt) return -EINVAL; - /* - * Try to upgrade the domain we have, it is an iommu driver bug to - * report IOMMU_CAP_ENFORCE_CACHE_COHERENCY but fail - * enforce_cache_coherency when there are no devices attached to the - * domain. - */ - if (idev->enforce_cache_coherency && !hwpt->enforce_cache_coherency) { - if (hwpt->domain->ops->enforce_cache_coherency) - hwpt->enforce_cache_coherency = - hwpt->domain->ops->enforce_cache_coherency( - hwpt->domain); - if (!hwpt->enforce_cache_coherency) { - WARN_ON(list_empty(&idev->igroup->device_list)); - return -EINVAL; - } + /* Try to upgrade the domain we have */ + if (idev->enforce_cache_coherency) { + rc = iommufd_hw_pagetable_enforce_cc(hwpt); + if (rc) + return rc; } rc = iopt_table_enforce_dev_resv_regions(&hwpt->ioas->iopt, idev->dev, diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index 566eba0cd9b917..2584f9038b29a2 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -25,6 +25,20 @@ void iommufd_hw_pagetable_destroy(struct iommufd_object *obj) refcount_dec(&hwpt->ioas->obj.users); } +int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt) +{ + if (hwpt->enforce_cache_coherency) + return 0; + + if (hwpt->domain->ops->enforce_cache_coherency) + hwpt->enforce_cache_coherency = + hwpt->domain->ops->enforce_cache_coherency( + hwpt->domain); + if (!hwpt->enforce_cache_coherency) + return -EINVAL; + return 0; +} + /** * iommufd_hw_pagetable_alloc() - Get an iommu_domain for a device * @ictx: iommufd context @@ -60,6 +74,19 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, goto out_abort; } + /* + * Set the coherency mode before we do iopt_table_add_domain() as some + * iommus have a per-PTE bit that controls it and need to decide before + * doing any maps. It is an iommu driver bug to report + * IOMMU_CAP_ENFORCE_CACHE_COHERENCY but fail enforce_cache_coherency on + * a new domain. + */ + if (idev->enforce_cache_coherency) { + rc = iommufd_hw_pagetable_enforce_cc(hwpt); + if (WARN_ON(rc)) + goto out_abort; + } + mutex_lock(&idev->igroup->lock); /* diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 8bf053f4d4a9ce..471a3fdff1e0b6 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -254,6 +254,7 @@ struct iommufd_hw_pagetable { struct iommufd_hw_pagetable * iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, struct iommufd_device *idev, bool immediate_attach); +int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt); int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev); struct iommufd_hw_pagetable *