Re: [PATCH v2 11/26] iommu/amd: Allocate and map vIOMMU private regions

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, May 28, 2026 at 05:17:23AM +0000, Suravee Suthikulpanit wrote:

> --- a/drivers/iommu/amd/viommu.c
> +++ b/drivers/iommu/amd/viommu.c
> @@ -131,8 +131,66 @@ static int __init viommu_vf_vfcntl_init(struct amd_iommu *iommu)
>  	return -ENOMEM;
>  }
>  
> +static void *alloc_private_subregion(struct amd_iommu *iommu, u64 base, size_t size)
> +{
> +	int ret;
> +	void *region;
> +	int nid = iommu && iommu->dev ? dev_to_node(&iommu->dev->dev) : NUMA_NO_NODE;
> +
> +	region = (void *)iommu_alloc_pages_node_sz(nid, GFP_KERNEL | __GFP_ZERO, size);
> +	if (!region)
> +		return NULL;
> +
> +	ret = set_memory_uc((unsigned long)region, size >> PAGE_SHIFT);
> +	if (ret)
> +		goto err_out;

Why?

> +	ret = iommu_map(&iommu->viommu_pdom->domain, base,
> +			iommu_virt_to_phys(region), size,
> +			IOMMU_PROT_IR | IOMMU_PROT_IW, GFP_KERNEL);
> +
> +	if (ret)
> +		goto cleanup_mem_attr;
> +
> +	pr_debug("%s: base=%#llx, size=%#lx, subregion=%#llx(%#llx)\n",
> +		 __func__, base, size, (unsigned long long)region, iommu_virt_to_phys(region));
> +
> +	amd_iommu_flush_private_vm_region(iommu, iommu->viommu_pdom, base, size);

Why? Is there suddenly negative caching for this mode?

> +	return region;
> +cleanup_mem_attr:
> +	set_memory_wb((unsigned long)region, size >> PAGE_SHIFT);
> +err_out:
> +	iommu_free_pages(region);
> +	return NULL;
> +}
> +
> +static void viommu_private_space_uninit(struct amd_iommu *iommu)
> +{
> +	int i;
> +	struct iommu_domain *dom;
> +
> +	if (!iommu->viommu_pdom)
> +		return;
> +
> +	for (i = 0; i < VIOMMU_PRIV_SUBREGION_CNT; i++) {
> +		if (!iommu->viommu_priv_region[i])
> +			continue;
> +		set_memory_wb((unsigned long)iommu->viommu_priv_region[i],
> +			      VIOMMU_PRIV_SUBREGION_SIZE >> PAGE_SHIFT);
> +		iommu_free_pages(iommu->viommu_priv_region[i]);
> +		iommu->viommu_priv_region[i] = NULL;
> +	}
> +
> +	dom = &iommu->viommu_pdom->domain;
> +	amd_iommu_domain_free(dom);
> +	iommu->viommu_pdom = NULL;
> +}

Shouldn't something flush the DID before freeing the domain?

>  static int viommu_private_space_init(struct amd_iommu *iommu)
>  {
> +	int i;
> +	u64 base;
>  	struct iommu_domain *dom;
>  	struct protection_domain *pdom;
>  	struct pt_iommu_amdv1_hw_info pt_info;
> @@ -144,22 +202,33 @@ static int viommu_private_space_init(struct amd_iommu *iommu)
>  	dom = amd_iommu_domain_alloc_paging_v1(&iommu->dev->dev, 0);
>  	if (!dom) {
>  		pr_err("%s: Failed to initialize private space\n", __func__);
> -		goto err_out;
> +		return -ENOMEM;
>  	}
>  
>  	pdom = to_pdomain(dom);
>  	iommu->viommu_pdom = pdom;
>  
> +	/*
> +	 * Each private region requires to 8MB of memory to be allocated
> +	 * and mapped. Split the region into 4 x 2MB-subregion.
> +	 */
> +	for (i = 0; i < VIOMMU_PRIV_SUBREGION_CNT; i++) {
> +		base = VIOMMU_PRIV_REGION_BASE + (i * VIOMMU_PRIV_SUBREGION_SIZE);
> +		iommu->viommu_priv_region[i] = alloc_private_subregion(iommu, base,
> +								       VIOMMU_PRIV_SUBREGION_SIZE);
> +		if (!iommu->viommu_priv_region[i]) {
> +			pr_err("%s: Failed to allocate vIOMMU private subregion %d\n", __func__, i);
> +			viommu_private_space_uninit(iommu);
> +			return -ENOMEM;
> +		}
> +	}
> +
>  	pt_iommu_amdv1_hw_info(&pdom->amdv1, &pt_info);
>  	pr_debug("%s: devid=%#x, pte_root=%#llx\n",
>  		 __func__, iommu->devid,
>  		 (unsigned long long)pt_info.host_pt_root);
>  
>  	return 0;
> -err_out:
> -	if (dom)
> -		amd_iommu_domain_free(dom);
> -	return -ENOMEM;

Why is the error handling being deleted now? You should organize your
patches to avoid churn like this.

Jason