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

[Suthikulpanit, Suravee]

On 7/7/2026 9:07 PM, Jason Gunthorpe wrote:
> 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?


The domain_flush_pages_v1() only queues invalidation commands to IOMMUs 
recorded in pdom->iommu_array. iommu_array is populated only when a 
device is attached via attach_device() → pdom_attach_iommu(). The 
viommu_pdom is not attached that way; it is wired through the IOMMU’s 
own DTE in set_dte_ipa().

However, I am making change to the code so that we would not need this.

> > +/*
> > + * 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

Sure. I'll fix this.

> > +	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

I'll fix this.

> > +/*
> > + * 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

I am updating and sending out V4.

Thanks,
Suravee