Re: [PATCH v2 09/12] iommu/amd: Add support for nest parent domain allocation

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Oct 01, 2025 at 06:09:51AM +0000, Suravee Suthikulpanit wrote:
> To support nested translation, the nest parent domain is allocated with
> IOMMU_HWPT_ALLOC_NEST_PARENT flag, and stores information of the v1 page
> table for stage 2 (i.e. GPA->SPA).
> 
> Also, only support nest parent domain on AMD system, which can support
> the Guest CR3 Table (GCR3TRPMode) feature. This feature is required in
> order to program DTE[GCR3 Table Root Pointer] with the GPA.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

> +static inline bool is_nest_parent_supported(u32 flags)
> +{
> +	/* Only allow nest parent when these features are supported */
> +	if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) &&
> +	    (!check_feature(FEATURE_GT) ||
> +	     !check_feature(FEATURE_GIOSUP) ||
> +	     !check_feature2(FEATURE_GCR3TRPMODE)))
> +		return false;
> +
> +	return true;

If the "flags" doesn't have IOMMU_HWPT_ALLOC_NEST_PARENT while one
of the features is missing, this would return true, indicating the
HW supports nesting parent, which will be logically wrong although
it does validate the nest parent flag.

Following the existing coding style, I think this could be just:

static inline bool is_nest_parent_supported(void)
{
	/* Only allow nest parent when these features are supported */
	return check_feature(FEATURE_GT) && check_feature(FEATURE_GIOSUP) &&
	       check_feature2(FEATURE_GCR3TRPMODE);
}

> @@ -2591,15 +2603,22 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
>  {
>  	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
>  	const u32 supported_flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
> -						IOMMU_HWPT_ALLOC_PASID;
> +						IOMMU_HWPT_ALLOC_PASID |
> +						IOMMU_HWPT_ALLOC_NEST_PARENT;
>  
> -	if ((flags & ~supported_flags) || user_data)
> +	if ((flags & ~supported_flags) || user_data || !is_nest_parent_supported(flags))
>  		return ERR_PTR(-EOPNOTSUPP);

Un-change this, given the code below is already validating flags.

>  	switch (flags & supported_flags) {
>  	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING:
> -		/* Allocate domain with v1 page table for dirty tracking */
> -		if (!amd_iommu_hd_support(iommu))
> +	case IOMMU_HWPT_ALLOC_NEST_PARENT:
> +	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING | IOMMU_HWPT_ALLOC_NEST_PARENT:
> +		/*
> +		 * Allocate domain with v1 page table for dirty tracking
> +		 * and/or Nest parent.
> +		 */
> +		if ((flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) &&
> +		    !amd_iommu_hd_support(iommu))
>  			break;

And add here:
		if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT)) &&
		    !is_nest_parent_supported())
			    break;

Otherwise, this looks good to me:

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>