Re: [PATCH 1/2] iommu: Add ops->domain_alloc_nested()

[Baolu Lu <baolu.lu@linux.intel.com>]

On 11/15/24 03:55, Jason Gunthorpe wrote:
> It turns out all the drivers that are using this immediately call into
> another function, so just make that function directly into the op. This
> makes paging=NULL for domain_alloc_user and we can remove the argument in
> the next patch.
> 
> The function mirrors the similar op in the viommu that allocates a nested
> domain on top of the viommu's nesting parent. This version supports cases
> where a viommu is not being used.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> ---
>   drivers/iommu/intel/iommu.c          |  9 +++------
>   drivers/iommu/intel/iommu.h          |  6 ++++--
>   drivers/iommu/intel/nested.c         | 11 +++++++++--
>   drivers/iommu/iommufd/hw_pagetable.c |  8 ++++----
>   drivers/iommu/iommufd/selftest.c     |  7 ++++---
>   include/linux/iommu.h                |  9 +++++----
>   6 files changed, 29 insertions(+), 21 deletions(-)
>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

with below minor suggestion.

[...]

> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
> index 42c4533a6ea21d..aba92c00b42740 100644
> --- a/drivers/iommu/intel/nested.c
> +++ b/drivers/iommu/intel/nested.c
> @@ -186,14 +186,21 @@ static const struct iommu_domain_ops intel_nested_domain_ops = {
>   	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
>   };
>   
> -struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
> -					       const struct iommu_user_data *user_data)
> +struct iommu_domain *
> +intel_iommu_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
> +				u32 flags,
> +				const struct iommu_user_data *user_data)
>   {
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
>   	struct dmar_domain *s2_domain = to_dmar_domain(parent);
> +	struct intel_iommu *iommu = info->iommu;
>   	struct iommu_hwpt_vtd_s1 vtd;
>   	struct dmar_domain *domain;
>   	int ret;
>   
> +	if (!nested_supported(iommu) || flags)
> +		return ERR_PTR(-EOPNOTSUPP);

How about making it like

         if (!nested_supported(iommu) || (flags & 
~IOMMU_HWPT_FAULT_ID_VALID))
                 return ERR_PTR(-EOPNOTSUPP);

         if ((flags & IOMMU_HWPT_FAULT_ID_VALID) && !info->pri_supported)
                 return ERR_PTR(-EOPNOTSUPP);

?

--
baolu