Re: [PATCH V1 3/3] x86/hyperv: Implement root VM IOMMU kernel only driver

["Jörg Rödel" <joro@8bytes.org>]

On Wed, Sep 23, 2026 at 07:02:21PM -0700, Mukesh R wrote:
> +static int hv_iommu_map_pages(struct iommu_domain *immdom, ulong iova,
> +			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
> +			      int prot, gfp_t gfp, size_t *mapped)
> +{
> +	u32 map_flags;
> +	int ret;
> +	u64 status = 0;
> +	ulong npages, sav_iova = iova, done = 0;
> +	struct hv_domain *hvdom = to_hv_domain(immdom);
> +	phys_addr_t sav_paddr = paddr;
> +	size_t done_size, size = pgsize * pgcount;
> +
> +	map_flags = HV_MAP_GPA_READABLE;	/* required */
> +	map_flags |= prot & IOMMU_WRITE ? HV_MAP_GPA_WRITABLE : 0;
> +
> +	npages = size >> HV_HYP_PAGE_SHIFT;
> +	while (done < npages) {
> +		ulong completed, remain = npages - done;
> +
> +		remain = min(remain, HV_MAP_DEVICE_GPA_BATCH_SIZE);
> +
> +		status = hv_iommu_map_pgs(hvdom, iova, paddr, remain,
> +					  map_flags);
> +
> +		completed = hv_repcomp(status);
> +		done = done + completed;
> +		iova = iova + (completed << HV_HYP_PAGE_SHIFT);
> +		paddr = paddr + (completed << HV_HYP_PAGE_SHIFT);
> +
> +		if (hv_result_needs_memory(status)) {
> +			ret = hv_call_deposit_pages(NUMA_NO_NODE,
> +						    hv_current_partition_id,
> +						    256);

hv_call_deposit_pages() can sleep and must not be called when gfp == GFP_ATOMIC.

> +			if (ret)
> +				break;
> +			continue;
> +		}
> +		if (!hv_result_success(status))
> +			break;
> +	}
> +
> +	done_size = done << HV_HYP_PAGE_SHIFT;
> +
> +	if (mapped)
> +		*mapped = done_size;
> +
> +	if (done_size == 0)
> +		return hv_result_to_errno(status);
> +
> +	ret = hv_iommu_add_tree_mapping(hvdom, sav_iova, sav_paddr, done_size,
> +					map_flags);
> +	if (ret) {
> +		size = hv_iommu_hyp_unmap_pages(immdom, sav_iova, done_size);
> +		if (size != done_size)
> +			WARN(1, "Failed to unmap exact sizes(%lx/%lx)\n",
> +			     done_size, size);
> +		if (mapped)
> +			*mapped = done_size - size;
> +
> +		return ret;
> +	}

A general question: Why is the tree mapping added after the hypervisor mappings
are established. It seems easier and more robust the other way around.  Same on
the unmap path which should use strictly the reverse order.

With this order, if adding a tree entry fails and the unmap only partially
succeeds, it ends up with a partial mapping which can not be unmapped anymore
because there is no tree entry.

> +static int __init hv_iommu_init(void)
> +{
> +	int rc;
> +	struct iommu_device *iommup = &hv_virt_iommu;
> +	struct hv_output_get_iommu_capabilities caps;
> +
> +	if (!hv_is_hyperv_initialized())
> +		return -ENODEV;
> +
> +	rc = hv_iommu_get_caps(&caps);
> +	if (rc)
> +		return rc;
> +

The capabilities returned need more checking. I think at least it needs a check
for HV_IOMMU_CAP_PRESENT and that PAGE_SIZE is set in the pgsize_bitmap.

> +	hv_iommu_max_iova = ((ulong)1 << caps.max_iova_width) - 1;

This is undefined behavior if caps.max_iova_width == 64. Better use DMA_BIT_MASK().