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

[Mukesh R <mrathor@linux.microsoft.com>]

On 9/24/26 06:35, J?rg R?del wrote:
> 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.

yes, indeed, i missed it.  will address it...


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

well, it was before in V0, but sashiko/copilot/claude/.. you name it
all bots jumped on me that it would cause tree to go out of sync if map
fails to map everything, since removal from tree then will remove the
entire node in the cleanup path, thus leaving mappings in the hyp that
are not in the tree. In this case above, we only add to the tree whatever
succeeded. Problem at high level is, any of the two can fail, and undoing
could also fail, sorta chicken and egg issue.. We discussed internally
and thought adding WARN() would allow one to catch and maybe panic in
those cases. I also noticed that other drivers also have similar issues
and viommu_map_pages() just ignores if viommu_del_mappings() fails, and
fails the callback.  We fail the callback here also. My V0 did same as
viommu_map_pages(), difference being, in our case, a hypercall can
fail with partially done list.

So, I'm at a bit of loss. I could go back to V0 which did what what you
suggested above, but it can also leave stale mappings when map hypercall
succeeds partially and we delete the entire tree node in cleanup (but
we could add to the tree back whatever succeeded, but that could fail
also, but then we could unmap whatever was mapped, but that could fail
also.. see :)...).

Open to any other suggestions you might have also.


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

In general this caps hypercall is not available to root partitions,
our only agreement is for hyp to provide max_iova_width. But, we check
HV_DEVICE_DOMAIN_AVAILABLE in hv_iommu_detect() which sorta supersedes
HV_IOMMU_CAP_PRESENT. As for page size, hyp owns the iommu and can chose
the page size, meaning it can automatically use larger page size when
possible. But the hypercall HVCALL_MAP_DEVICE_GPA_PAGES will only take
4k pfns as input, hence:

#define HV_IOMMU_PGSIZES SZ_4K        /* for now, to be enhanced */

>> +	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().

Ok, I can change it back to DMA_BIT_MASK. (I removed after comment
on V0 that this is not dma address).

Thanks a lot for the review.

-Mukesh