Re: [PATCH v2] iommufd/device: Enforce reserved IOVA also when attached to hwpt_nested

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Aug 05, 2024 at 10:02:01PM -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. Allow a common hwpt input to support both a
> hwpt_paging type and a hwpt_nested type.
> 
> Rework the to_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. And add another hwpt_to_ioas helper to get the
> IOAS pointer. Return a NULL in these two helpers for any potential new
> HWPT type, and make a NOP in those reserved IOVA functions accordingly.
> 
> Suggested-by: Tian, Kevin <kevin.tian@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> 
> Changelog
> v2:
>  * 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          | 77 ++++++++++++++-----------
>  drivers/iommu/iommufd/iommufd_private.h | 15 ++++-
>  2 files changed, 56 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 3214a4c17c6b3..949b69c9f3b2d 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -327,13 +327,18 @@ 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_hw_pagetable *hwpt)
>  {
> +	struct iommufd_hwpt_paging *hwpt_paging = to_hwpt_paging(hwpt);
>  	int rc;

This seems like the wrong place to put these types, a big point point of the
struct iommufd_hwpt_paging was to mark functions that should only be
operating on a paging hwpt.

The caller is expected to figure out what it is doing.

>  int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>  				struct iommufd_device *idev)
>  {
> @@ -363,11 +380,9 @@ 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 (rc)
> -			goto err_unlock;
> -	}

Like we had here, so it would be a bit nicer to write it as more like:

hwpt_paging = to_hwpt_paging(hwpt);
if (hwpt_paging) {
	rc = iommufd_hwpt_paging_attach(hwpt_paging, idev);
}

Then we can keep the clearer labeling of the function signatures.

> @@ -321,7 +321,20 @@ static inline bool hwpt_is_paging(struct iommufd_hw_pagetable *hwpt)
>  static inline struct iommufd_hwpt_paging *
>  to_hwpt_paging(struct iommufd_hw_pagetable *hwpt)
>  {
> -	return container_of(hwpt, struct iommufd_hwpt_paging, common);
> +	switch (hwpt->obj.type) {
> +	case IOMMUFD_OBJ_HWPT_PAGING:
> +		return container_of(hwpt, struct iommufd_hwpt_paging, common);
> +	case IOMMUFD_OBJ_HWPT_NESTED:
> +		return container_of(hwpt, struct iommufd_hwpt_nested, common)->parent;
> +	default:
> +		return NULL;
> +	}
> +}

There are alot of existing callers of this, I think it should get a
new function to do this behavior and it would only be used in a few
places.

It is not OK for a NESTING to get into most of the places that are
already calling this.

Jason