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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aN69rkBJS2hIcZjk@Asurada-Nvidia>
Date: Thu, 2 Oct 2025 11:00:14 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
CC: <jgg@...dia.com>, <linux-kernel@...r.kernel.org>, <robin.murphy@....com>,
	<will@...nel.org>, <joro@...tes.org>, <kevin.tian@...el.com>,
	<jsnitsel@...hat.com>, <vasant.hegde@....com>, <iommu@...ts.linux.dev>,
	<santosh.shukla@....com>, <sairaj.arunkodilkar@....com>, <jon.grimm@....com>,
	<prashanthpra@...gle.com>, <wvw@...gle.com>, <wnliu@...gle.com>,
	<gptran@...gle.com>, <kpsingh@...gle.com>, <joao.m.martins@...cle.com>,
	<alejandro.j.jimenez@...cle.com>
Subject: Re: [PATCH v2 09/12] iommu/amd: Add support for nest parent domain
 allocation

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@....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@...dia.com>