Re: [PATCH v2 12/26] iommu/amd: Add per-VM private IPA alloc/map helpers

[Suthikulpanit, Suravee]

On 6/1/2026 8:08 PM, Jason Gunthorpe wrote:
> On Thu, May 28, 2026 at 05:17:24AM +0000, Suravee Suthikulpanit wrote:
> > +static int __maybe_unused alloc_private_vm_region(struct amd_iommu *iommu, u64 **entry,
> > +						 u64 base, size_t size, u16 gid)
> > +{
> > +	int ret;
> > +	u64 addr = base + (gid * size);
> > +	int nid = iommu && iommu->dev ? dev_to_node(&iommu->dev->dev) : NUMA_NO_NODE;
> > +
> > +	*entry = (void *)iommu_alloc_pages_node_sz(nid, GFP_KERNEL | __GFP_ZERO, size);
> > +	if (!*entry)
> > +		return -ENOMEM;
>
> No cast
>
> > +	ret = set_memory_uc((unsigned long)*entry, size >> PAGE_SHIFT);
> > +	if (ret)
> > +		goto err_out;
> > +
> > +	pr_debug("%s: entry=%#llx(%#llx), addr=%#llx, size=%#lx\n", __func__,
> > +		 (unsigned long  long)*entry, iommu_virt_to_phys(*entry), addr, size);
> > +
> > +	ret = iommu_map(&iommu->viommu_pdom->domain, addr,
> > +			iommu_virt_to_phys(*entry), size,
> > +			IOMMU_PROT_IR | IOMMU_PROT_IW, GFP_KERNEL);
> > +	if (ret)
> > +		goto cleanup_mem_attr;
> > +
> > +	return amd_iommu_flush_private_vm_region(iommu, iommu->viommu_pdom, addr, size);
> > +cleanup_mem_attr:
> > +	set_memory_wb((unsigned long)*entry, size >> PAGE_SHIFT);
> > +err_out:
> > +	iommu_free_pages(*entry);
> > +	*entry = NULL;
> > +	return ret;
> > +}
>
> This all seems duplicated from the prior patch, you should try to make
> one helper that does this alloc and map sequence.
>
> > +static void __maybe_unused free_private_vm_region(struct amd_iommu *iommu, u64 **entry,
> > +						  u64 base, size_t size, u16 gid)
> > +{
> > +	size_t unmapped;
> > +	u64 addr = base + (gid * size);
> > +
> > +	pr_debug("%s: entry=%#llx(%#llx), base=%#llx, addr=%#llx, size=%#lx\n",
> > +		 __func__, (unsigned long  long)*entry,
> > +		 iommu_virt_to_phys(*entry), base, addr, size);
> > +
> > +	if (!iommu || !iommu->viommu_pdom)
> > +		return;
> > +
> > +	unmapped = iommu_unmap(&iommu->viommu_pdom->domain, addr, size);
> > +	if (unmapped != size)
> > +		pr_warn("%s: unmapped %#zx of %#lx at %#llx\n", __func__, unmapped, size, addr);
>
> No flush? Or does the gather work for the special domain?
>
> Jason

I'll take care of this in V3.

Suravee