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

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

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