Re: [PATCH v3 09/22] iommu/amd: Introduce and map vIOMMU private IPA region

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Jun 29, 2026 at 03:35:22PM +0000, Suravee Suthikulpanit wrote:
> @@ -1808,6 +1808,22 @@ static int domain_flush_pages_v1(struct protection_domain *pdom,
>  	return ret;
>  }
>  
> +int amd_iommu_flush_private_vm_region(struct amd_iommu *iommu, struct protection_domain *pdom,
> +				      u64 address, size_t size)
> +{
> +	int ret;
> +	struct iommu_cmd cmd;

Why do we need this function?  iommu_unmap() should generate this
flush automatically, shouldn't it?

> +/*
> + * Allocate backing pages, mark UC, and map at @iova in viommu_pdom.
> + * *@out_va is NULL on any failure.
> + */
> +static int viommu_priv_alloc_map_flush(struct amd_iommu *iommu, u64 iova, size_t size,
> +				       gfp_t gfp, void **out_va)
> +{

I think the flush is a bit unnecessary in the name

> +	int ret;
> +	void *va;
> +	int nid = iommu && iommu->dev ? dev_to_node(&iommu->dev->dev) : NUMA_NO_NODE;
> +
> +	*out_va = NULL;
> +
> +	if (!iommu || !iommu->viommu_pdom)
> +		return -EINVAL;
> +
> +	va = iommu_alloc_pages_node_sz(nid, gfp, size);
> +	if (!va)
> +		return -ENOMEM;
> +
> +	/*
> +	 * IOMMU spec mentions that the vIOMMU backing storage memory
> +	 * should be marked as UC.
> +	 */
> +	ret = set_memory_uc((unsigned long)va, size >> PAGE_SHIFT);
> +	if (ret)
> +		goto err_free_pages;
> +
> +	ret = iommu_map(&iommu->viommu_pdom->domain, iova, iommu_virt_to_phys(va), size,
> +			IOMMU_PROT_IR | IOMMU_PROT_IW, GFP_KERNEL);
                        ^^^^^^^^^^^^^^^^^^^^^^^

These are the wrong constants to pass to iommu_prot

> +/*
> + * Unmap @iova, flush the unmapped span on this IOMMU, WB, and free @cpu_va.
> + * Returns 0, or the flush error if amd_iommu_flush_private_vm_region() fails.
> + */
> +static int viommu_priv_unmap_flush_free(struct amd_iommu *iommu, u64 iova, size_t size,
> +					void *cpu_va)
> +{
> +	size_t unmapped;
> +	int ret = 0;
> +
> +	if (!cpu_va)
> +		return 0;
> +	if (!iommu || !iommu->viommu_pdom)
> +		return -EINVAL;
> +
> +	unmapped = iommu_unmap(&iommu->viommu_pdom->domain, iova, size);
> +	if (unmapped != size)
> +		pr_warn("%s: unmapped %#zx of %#lx at %#llx\n", __func__, unmapped, size, iova);
> +
> +	if (unmapped) {
> +		ret = amd_iommu_flush_private_vm_region(iommu, iommu->viommu_pdom, iova,
> +							unmapped);

unmap calls flush through the domain, why do we need another flush?

Jason