Re: [PATCH V2 4/4] x86/hyperv: Implement root partition IOMMU kernel only driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iommu/hyperv: hardware and software state desynchronization on partial unmaps
- [Medium] iommu/hyperv: caller-provided GFP flags are ignored in hv_iommu_map_pages
- [Low] iommu/hyperv: missing select INTERVAL_TREE in Kconfig causes build failure
--

commit 3a4e409fbf35c471ff4e423714deeda11c2192b2
Author: Mukesh R <mrathor@linux.microsoft.com>

x86/hyperv: Implement root partition IOMMU kernel only driver

This commit adds a kernel-only virtual IOMMU driver for Microsoft Hyper-V
running on root partitions. It registers the IOMMU device, creates identity
and blocked domains, and handles device attachment and page mappings.

> diff --git a/drivers/iommu/hyperv/Kconfig b/drivers/iommu/hyperv/Kconfig
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iommu/hyperv/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +# Hyper-V IOMMU support
> +
> +config HYPERV_ROOT_IOMMU
> +	bool "Hyper-V IOMMU Device in root partition"
> +	depends on HYPERV && X86 && PCI_MSI && MSHV_ROOT
> +	select IOMMU_API

[Severity: Low]
Does this config need to select INTERVAL_TREE? 

The driver relies on interval_tree_insert and interval_tree_remove, which
can cause a build failure if no other selected module provides the interval
tree API.

[ ... ]

> diff --git a/drivers/iommu/hyperv/hv-iommu-root.c b/drivers/iommu/hyperv/hv-iommu-root.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iommu/hyperv/hv-iommu-root.c
[ ... ]
> +static int hv_iommu_add_tree_mapping(struct hv_iommu_domain *hvdom, ulong iova,
> +				     phys_addr_t paddr, size_t size, u32 flags)
> +{
> +	ulong irqflags;
> +	struct hv_iommu_mapping *mapping;
> +
> +	mapping = kzalloc_obj(struct hv_iommu_mapping, GFP_ATOMIC);

[Severity: Medium]
Is it intentional to hardcode GFP_ATOMIC for this allocation?

Looking at hv_iommu_map_pages, the caller provides a gfp parameter:

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)

This provided gfp parameter is discarded when hv_iommu_map_pages calls
hv_iommu_add_tree_mapping. Using GFP_ATOMIC instead of the caller-provided
flags can cause allocations to fail under memory pressure.

[ ... ]

> +static size_t hv_iommu_unmap_pages(struct iommu_domain *immdom, ulong iova,
> +				   size_t pgsize, size_t pgcount,
> +				   struct iommu_iotlb_gather *gather)
> +{
> +	struct hv_iommu_domain *hvdom = to_hv_domain(immdom);
> +	size_t unmap_sz, size = pgsize * pgcount;
> +
> +	/* For valid input, the hypervisor guarantees unmap will succeed */
> +	unmap_sz = hv_iommu_hyp_unmap_pages(immdom, iova, size);
> +	if (unmap_sz != size)
> +		WARN(1, "Failed to unmap device gpa(%lx/%lx)\n", unmap_sz,
> +		     size);
> +
> +	size = hv_iommu_del_tree_mappings(hvdom, iova, unmap_sz);
> +	if (size != unmap_sz)
> +		pr_err("%s: could not delete tree mappings (%lx:%lx/%lx)\n",
> +		       __func__, iova, unmap_sz, size);
> +
> +	return size;
> +}

[Severity: High]
Can this sequence cause a permanent desynchronization between the hardware
and software tracking states?

If a partial unmap is requested, hv_iommu_hyp_unmap_pages irreversibly modifies
the hypervisor hardware state first. Then, hv_iommu_del_tree_mappings is called
to update the software tracking tree.

If the unmap forces a split, hv_iommu_del_tree_mappings bails out and returns 0:

        /* Splitting of a mapping is not supported at present */
        if (mapping->iova.start < iova)
                break;

Returning the 0 unmapped size from the software tree instead of the actual
hardware unmapped size causes the IOMMU core unmap loop to terminate
prematurely. This leaks IOVA space while the hardware has already unmapped
the pages.

Could the software tree boundary be checked prior to executing the hypercall,
or should the core receive the actual hardware unmapped size instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929223622.778617-1-mrathor@linux.microsoft.com?part=4