drivers/iommu/iommufd/device.c | 52 ++++++++++++------------- drivers/iommu/iommufd/iommufd_private.h | 19 +++++++++ 2 files changed, 45 insertions(+), 26 deletions(-)
Currently, device reserved regions are only enforced when the device is
attached to an hwpt_paging. In other words, if the device gets attached
to an hwpt_nested directly, the parent hwpt_paging of the hwpt_nested's
would not enforce those reserved IOVAs. This works for most of reserved
region types, but not for IOMMU_RESV_SW_MSI, which is a unique software
defined window, required by a nesting case too to setup an MSI doorbell
on the parent stage-2 hwpt/domain.
Kevin pointed out that:
1) there is no usage using up closely the entire IOVA space yet,
2) guest may change the viommu mode to switch between nested
and paging then VMM has to take all devices' reserved regions
into consideration anyway, when composing the GPA space.
Link: https://lore.kernel.org/all/BN9PR11MB5276497781C96415272E6FED8CB12@BN9PR11MB5276.namprd11.prod.outlook.com/
So it would be actually convenient for us to also enforce reserved IOVA
onto the parent hwpt_paging, when attaching a device to an hwpt_nested.
Repurpose the existing attach/replace_paging helpers to attach device's
reserved IOVAs exclusively.
Add a new find_hwpt_paging helper, which is only used by these reserved
IOVA functions, to allow an IOMMUFD_OBJ_HWPT_NESTED hwpt to redirect to
its parent hwpt_paging. Return a NULL in these two helpers for any new
HWPT type in the future.
Suggested-by: Tian, Kevin <kevin.tian@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
Changelog
v3:
* Moved to_hwpt_paging logic back to the callers
* Added a new find_hwpt_paging for reserved iova callers
v2:
https://lore.kernel.org/all/20240806050201.3717816-1-nicolinc@nvidia.com/
* Corrected the ioas comparisons for future hwpt type that returns
NULL by the to_hwpt_paging helper.
v1:
https://lore.kernel.org/all/20240802053458.2754673-1-nicolinc@nvidia.com/
drivers/iommu/iommufd/device.c | 52 ++++++++++++-------------
drivers/iommu/iommufd/iommufd_private.h | 19 +++++++++
2 files changed, 45 insertions(+), 26 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 3214a4c17c6b3..bcbcd6d940624 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -327,8 +327,9 @@ static int iommufd_group_setup_msi(struct iommufd_group *igroup,
return 0;
}
-static int iommufd_hwpt_paging_attach(struct iommufd_hwpt_paging *hwpt_paging,
- struct iommufd_device *idev)
+static int
+iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
+ struct iommufd_hwpt_paging *hwpt_paging)
{
int rc;
@@ -354,6 +355,7 @@ static int iommufd_hwpt_paging_attach(struct iommufd_hwpt_paging *hwpt_paging,
int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
struct iommufd_device *idev)
{
+ struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
int rc;
mutex_lock(&idev->igroup->lock);
@@ -363,8 +365,8 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
goto err_unlock;
}
- if (hwpt_is_paging(hwpt)) {
- rc = iommufd_hwpt_paging_attach(to_hwpt_paging(hwpt), idev);
+ if (hwpt_paging) {
+ rc = iommufd_device_attach_reserved_iova(idev, hwpt_paging);
if (rc)
goto err_unlock;
}
@@ -387,9 +389,8 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
mutex_unlock(&idev->igroup->lock);
return 0;
err_unresv:
- if (hwpt_is_paging(hwpt))
- iopt_remove_reserved_iova(&to_hwpt_paging(hwpt)->ioas->iopt,
- idev->dev);
+ if (hwpt_paging)
+ iopt_remove_reserved_iova(&hwpt_paging->ioas->iopt, idev->dev);
err_unlock:
mutex_unlock(&idev->igroup->lock);
return rc;
@@ -399,6 +400,7 @@ struct iommufd_hw_pagetable *
iommufd_hw_pagetable_detach(struct iommufd_device *idev)
{
struct iommufd_hw_pagetable *hwpt = idev->igroup->hwpt;
+ struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
mutex_lock(&idev->igroup->lock);
list_del(&idev->group_item);
@@ -406,9 +408,8 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev)
iommufd_hwpt_detach_device(hwpt, idev);
idev->igroup->hwpt = NULL;
}
- if (hwpt_is_paging(hwpt))
- iopt_remove_reserved_iova(&to_hwpt_paging(hwpt)->ioas->iopt,
- idev->dev);
+ if (hwpt_paging)
+ iopt_remove_reserved_iova(&hwpt_paging->ioas->iopt, idev->dev);
mutex_unlock(&idev->igroup->lock);
/* Caller must destroy hwpt */
@@ -440,17 +441,17 @@ iommufd_group_remove_reserved_iova(struct iommufd_group *igroup,
}
static int
-iommufd_group_do_replace_paging(struct iommufd_group *igroup,
- struct iommufd_hwpt_paging *hwpt_paging)
+iommufd_group_do_replace_reserved_iova(struct iommufd_group *igroup,
+ struct iommufd_hwpt_paging *hwpt_paging)
{
- struct iommufd_hw_pagetable *old_hwpt = igroup->hwpt;
+ struct iommufd_hwpt_paging *old_hwpt_paging;
struct iommufd_device *cur;
int rc;
lockdep_assert_held(&igroup->lock);
- if (!hwpt_is_paging(old_hwpt) ||
- hwpt_paging->ioas != to_hwpt_paging(old_hwpt)->ioas) {
+ old_hwpt_paging = find_hwpt_paging(igroup->hwpt);
+ if (!old_hwpt_paging || hwpt_paging->ioas != old_hwpt_paging->ioas) {
list_for_each_entry(cur, &igroup->device_list, group_item) {
rc = iopt_table_enforce_dev_resv_regions(
&hwpt_paging->ioas->iopt, cur->dev, NULL);
@@ -473,6 +474,8 @@ static struct iommufd_hw_pagetable *
iommufd_device_do_replace(struct iommufd_device *idev,
struct iommufd_hw_pagetable *hwpt)
{
+ struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
+ struct iommufd_hwpt_paging *old_hwpt_paging;
struct iommufd_group *igroup = idev->igroup;
struct iommufd_hw_pagetable *old_hwpt;
unsigned int num_devices;
@@ -491,9 +494,8 @@ iommufd_device_do_replace(struct iommufd_device *idev,
}
old_hwpt = igroup->hwpt;
- if (hwpt_is_paging(hwpt)) {
- rc = iommufd_group_do_replace_paging(igroup,
- to_hwpt_paging(hwpt));
+ if (hwpt_paging) {
+ rc = iommufd_group_do_replace_reserved_iova(igroup, hwpt_paging);
if (rc)
goto err_unlock;
}
@@ -502,11 +504,10 @@ iommufd_device_do_replace(struct iommufd_device *idev,
if (rc)
goto err_unresv;
- if (hwpt_is_paging(old_hwpt) &&
- (!hwpt_is_paging(hwpt) ||
- to_hwpt_paging(hwpt)->ioas != to_hwpt_paging(old_hwpt)->ioas))
- iommufd_group_remove_reserved_iova(igroup,
- to_hwpt_paging(old_hwpt));
+ old_hwpt_paging = find_hwpt_paging(old_hwpt);
+ if (old_hwpt_paging &&
+ (!hwpt_paging || hwpt_paging->ioas != old_hwpt_paging->ioas))
+ iommufd_group_remove_reserved_iova(igroup, old_hwpt_paging);
igroup->hwpt = hwpt;
@@ -524,9 +525,8 @@ iommufd_device_do_replace(struct iommufd_device *idev,
/* Caller must destroy old_hwpt */
return old_hwpt;
err_unresv:
- if (hwpt_is_paging(hwpt))
- iommufd_group_remove_reserved_iova(igroup,
- to_hwpt_paging(hwpt));
+ if (hwpt_paging)
+ iommufd_group_remove_reserved_iova(igroup, hwpt_paging);
err_unlock:
mutex_unlock(&idev->igroup->lock);
return ERR_PTR(rc);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 92efe30a8f0d0..0bfacaf40c055 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -324,6 +324,25 @@ to_hwpt_paging(struct iommufd_hw_pagetable *hwpt)
return container_of(hwpt, struct iommufd_hwpt_paging, common);
}
+static inline struct iommufd_hwpt_nested *
+to_hwpt_nested(struct iommufd_hw_pagetable *hwpt)
+{
+ return container_of(hwpt, struct iommufd_hwpt_nested, common);
+}
+
+static inline struct iommufd_hwpt_paging *
+find_hwpt_paging(struct iommufd_hw_pagetable *hwpt)
+{
+ switch (hwpt->obj.type) {
+ case IOMMUFD_OBJ_HWPT_PAGING:
+ return to_hwpt_paging(hwpt);
+ case IOMMUFD_OBJ_HWPT_NESTED:
+ return to_hwpt_nested(hwpt)->parent;
+ default:
+ return NULL;
+ }
+}
+
static inline struct iommufd_hwpt_paging *
iommufd_get_hwpt_paging(struct iommufd_ucmd *ucmd, u32 id)
{
--
2.43.0On Tue, Aug 06, 2024 at 05:34:46PM -0700, Nicolin Chen wrote: > Currently, device reserved regions are only enforced when the device is > attached to an hwpt_paging. In other words, if the device gets attached > to an hwpt_nested directly, the parent hwpt_paging of the hwpt_nested's > would not enforce those reserved IOVAs. This works for most of reserved > region types, but not for IOMMU_RESV_SW_MSI, which is a unique software > defined window, required by a nesting case too to setup an MSI doorbell > on the parent stage-2 hwpt/domain. > > Kevin pointed out that: > 1) there is no usage using up closely the entire IOVA space yet, > 2) guest may change the viommu mode to switch between nested > and paging then VMM has to take all devices' reserved regions > into consideration anyway, when composing the GPA space. > Link: https://lore.kernel.org/all/BN9PR11MB5276497781C96415272E6FED8CB12@BN9PR11MB5276.namprd11.prod.outlook.com/ > > So it would be actually convenient for us to also enforce reserved IOVA > onto the parent hwpt_paging, when attaching a device to an hwpt_nested. > > Repurpose the existing attach/replace_paging helpers to attach device's > reserved IOVAs exclusively. > > Add a new find_hwpt_paging helper, which is only used by these reserved > IOVA functions, to allow an IOMMUFD_OBJ_HWPT_NESTED hwpt to redirect to > its parent hwpt_paging. Return a NULL in these two helpers for any new > HWPT type in the future. > > Suggested-by: Tian, Kevin <kevin.tian@intel.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > > Changelog > v3: > * Moved to_hwpt_paging logic back to the callers > * Added a new find_hwpt_paging for reserved iova callers > v2: > https://lore.kernel.org/all/20240806050201.3717816-1-nicolinc@nvidia.com/ > * Corrected the ioas comparisons for future hwpt type that returns > NULL by the to_hwpt_paging helper. > v1: > https://lore.kernel.org/all/20240802053458.2754673-1-nicolinc@nvidia.com/ > > drivers/iommu/iommufd/device.c | 52 ++++++++++++------------- > drivers/iommu/iommufd/iommufd_private.h | 19 +++++++++ > 2 files changed, 45 insertions(+), 26 deletions(-) Applied to iommufd for-next Thanks, Jason
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Wednesday, August 7, 2024 8:35 AM > > Currently, device reserved regions are only enforced when the device is > attached to an hwpt_paging. In other words, if the device gets attached > to an hwpt_nested directly, the parent hwpt_paging of the hwpt_nested's > would not enforce those reserved IOVAs. This works for most of reserved > region types, but not for IOMMU_RESV_SW_MSI, which is a unique software > defined window, required by a nesting case too to setup an MSI doorbell > on the parent stage-2 hwpt/domain. > > Kevin pointed out that: > 1) there is no usage using up closely the entire IOVA space yet, > 2) guest may change the viommu mode to switch between nested > and paging then VMM has to take all devices' reserved regions > into consideration anyway, when composing the GPA space. > Link: > https://lore.kernel.org/all/BN9PR11MB5276497781C96415272E6FED8CB12@ > BN9PR11MB5276.namprd11.prod.outlook.com/ > > So it would be actually convenient for us to also enforce reserved IOVA > onto the parent hwpt_paging, when attaching a device to an hwpt_nested. > > Repurpose the existing attach/replace_paging helpers to attach device's > reserved IOVAs exclusively. > > Add a new find_hwpt_paging helper, which is only used by these reserved > IOVA functions, to allow an IOMMUFD_OBJ_HWPT_NESTED hwpt to redirect > to > its parent hwpt_paging. Return a NULL in these two helpers for any new > HWPT type in the future. > > Suggested-by: Tian, Kevin <kevin.tian@intel.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> I'm not sure the name find_hwpt_paging() is good enough but can't find a better alternative. So, Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On Fri, Aug 09, 2024 at 02:00:10AM +0000, Tian, Kevin wrote: > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Wednesday, August 7, 2024 8:35 AM > > > > Currently, device reserved regions are only enforced when the device is > > attached to an hwpt_paging. In other words, if the device gets attached > > to an hwpt_nested directly, the parent hwpt_paging of the hwpt_nested's > > would not enforce those reserved IOVAs. This works for most of reserved > > region types, but not for IOMMU_RESV_SW_MSI, which is a unique software > > defined window, required by a nesting case too to setup an MSI doorbell > > on the parent stage-2 hwpt/domain. > > > > Kevin pointed out that: > > 1) there is no usage using up closely the entire IOVA space yet, > > 2) guest may change the viommu mode to switch between nested > > and paging then VMM has to take all devices' reserved regions > > into consideration anyway, when composing the GPA space. > > Link: > > https://lore.kernel.org/all/BN9PR11MB5276497781C96415272E6FED8CB12@ > > BN9PR11MB5276.namprd11.prod.outlook.com/ > > > > So it would be actually convenient for us to also enforce reserved IOVA > > onto the parent hwpt_paging, when attaching a device to an hwpt_nested. > > > > Repurpose the existing attach/replace_paging helpers to attach device's > > reserved IOVAs exclusively. > > > > Add a new find_hwpt_paging helper, which is only used by these reserved > > IOVA functions, to allow an IOMMUFD_OBJ_HWPT_NESTED hwpt to redirect > > to > > its parent hwpt_paging. Return a NULL in these two helpers for any new > > HWPT type in the future. > > > > Suggested-by: Tian, Kevin <kevin.tian@intel.com> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > I'm not sure the name find_hwpt_paging() is good enough but can't > find a better alternative. So, > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Thanks for the review. I couldn't figure out a better name but that :) With the ongoing discussion with Robin at the other thread, we we might be able to get rid of the msi_cookie. If so, this one will be unnecessary. So, we might want to put this on hold? That being said, a new solution will take some time. I'll need to go through all cases and untangle the msi_cookie cleanly.. Thanks Nicolin
On Fri, Aug 09, 2024 at 01:23:12PM -0700, Nicolin Chen wrote: > On Fri, Aug 09, 2024 at 02:00:10AM +0000, Tian, Kevin wrote: > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > Sent: Wednesday, August 7, 2024 8:35 AM > > > > > > Currently, device reserved regions are only enforced when the device is > > > attached to an hwpt_paging. In other words, if the device gets attached > > > to an hwpt_nested directly, the parent hwpt_paging of the hwpt_nested's > > > would not enforce those reserved IOVAs. This works for most of reserved > > > region types, but not for IOMMU_RESV_SW_MSI, which is a unique software > > > defined window, required by a nesting case too to setup an MSI doorbell > > > on the parent stage-2 hwpt/domain. > > > > > > Kevin pointed out that: > > > 1) there is no usage using up closely the entire IOVA space yet, > > > 2) guest may change the viommu mode to switch between nested > > > and paging then VMM has to take all devices' reserved regions > > > into consideration anyway, when composing the GPA space. > > > Link: > > > https://lore.kernel.org/all/BN9PR11MB5276497781C96415272E6FED8CB12@ > > > BN9PR11MB5276.namprd11.prod.outlook.com/ > > > > > > So it would be actually convenient for us to also enforce reserved IOVA > > > onto the parent hwpt_paging, when attaching a device to an hwpt_nested. > > > > > > Repurpose the existing attach/replace_paging helpers to attach device's > > > reserved IOVAs exclusively. > > > > > > Add a new find_hwpt_paging helper, which is only used by these reserved > > > IOVA functions, to allow an IOMMUFD_OBJ_HWPT_NESTED hwpt to redirect > > > to > > > its parent hwpt_paging. Return a NULL in these two helpers for any new > > > HWPT type in the future. > > > > > > Suggested-by: Tian, Kevin <kevin.tian@intel.com> > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > > > I'm not sure the name find_hwpt_paging() is good enough but can't > > find a better alternative. So, > > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > Thanks for the review. I couldn't figure out a better name but > that :) > > With the ongoing discussion with Robin at the other thread, we > we might be able to get rid of the msi_cookie. If so, this one > will be unnecessary. So, we might want to put this on hold? Regardless this seems like a bug fix to me that we may as well take. Unless we intend to retire the entire RESV_SW mechanism it should work correctly. When you say "enforce" you mean both that the reserved region list reported to userspace does not include the region and that the required mapping was never installed in the parent? Jason
On Wed, Aug 14, 2024 at 07:38:43PM -0300, Jason Gunthorpe wrote: > On Fri, Aug 09, 2024 at 01:23:12PM -0700, Nicolin Chen wrote: > > On Fri, Aug 09, 2024 at 02:00:10AM +0000, Tian, Kevin wrote: > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > Sent: Wednesday, August 7, 2024 8:35 AM > > > > > > > > Currently, device reserved regions are only enforced when the device is > > > > attached to an hwpt_paging. In other words, if the device gets attached > > > > to an hwpt_nested directly, the parent hwpt_paging of the hwpt_nested's > > > > would not enforce those reserved IOVAs. This works for most of reserved > > > > region types, but not for IOMMU_RESV_SW_MSI, which is a unique software > > > > defined window, required by a nesting case too to setup an MSI doorbell > > > > on the parent stage-2 hwpt/domain. > > > > > > > > Kevin pointed out that: > > > > 1) there is no usage using up closely the entire IOVA space yet, > > > > 2) guest may change the viommu mode to switch between nested > > > > and paging then VMM has to take all devices' reserved regions > > > > into consideration anyway, when composing the GPA space. > > > > Link: > > > > https://lore.kernel.org/all/BN9PR11MB5276497781C96415272E6FED8CB12@ > > > > BN9PR11MB5276.namprd11.prod.outlook.com/ > > > > > > > > So it would be actually convenient for us to also enforce reserved IOVA > > > > onto the parent hwpt_paging, when attaching a device to an hwpt_nested. > > > > > > > > Repurpose the existing attach/replace_paging helpers to attach device's > > > > reserved IOVAs exclusively. > > > > > > > > Add a new find_hwpt_paging helper, which is only used by these reserved > > > > IOVA functions, to allow an IOMMUFD_OBJ_HWPT_NESTED hwpt to redirect > > > > to > > > > its parent hwpt_paging. Return a NULL in these two helpers for any new > > > > HWPT type in the future. > > > > > > > > Suggested-by: Tian, Kevin <kevin.tian@intel.com> > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > > > > > I'm not sure the name find_hwpt_paging() is good enough but can't > > > find a better alternative. So, > > > > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > > > Thanks for the review. I couldn't figure out a better name but > > that :) > > > > With the ongoing discussion with Robin at the other thread, we > > we might be able to get rid of the msi_cookie. If so, this one > > will be unnecessary. So, we might want to put this on hold? > > Regardless this seems like a bug fix to me that we may as well > take. Unless we intend to retire the entire RESV_SW mechanism it > should work correctly. I am hoping for that, yet can't guarantee of accomplishing it at the end of the day. The SW-defined window can still be a default region, stored somewhere in VFIO/IOMMUFD instead of the RESV_SW. That being said, the alternative solution is gonna take a while to finalize. So, probably doesn't hurt for us to have this patch. > When you say "enforce" you mean both that the reserved region list > reported to userspace does not include the region and that the > required mapping was never installed in the parent? The "enforce" in the subject is a quote from this function: iopt_table_enforce_dev_resv_regions(). So it might be more accurate with: iommufd: Enforce device reserved regions as well when attached to hwpt_nested Thanks Nicolin
On Wed, Aug 14, 2024 at 04:31:17PM -0700, Nicolin Chen wrote: > > Regardless this seems like a bug fix to me that we may as well > > take. Unless we intend to retire the entire RESV_SW mechanism it > > should work correctly. > > I am hoping for that, yet can't guarantee of accomplishing it at > the end of the day. I think we can't retire it completely. It is necessary to allow DPDK to work as-is, for instance. We can only hope to provide an alternative for nesting.. Jason
© 2016 - 2025 Red Hat, Inc.