Re: [PATCH V3 5/5] x86/hyperv: Implement root partition IOMMU kernel only driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] iommu/hyperv: guest VM boot crash due to partition check in hv_root_iommu_detect()
- [High] x86/pci-dma: hv_root_iommu_init() overwritten by Intel/AMD IOMMU detect
- [High] iommu/hyperv: hv_iommu_att_dev2dom() fails on HV_STATUS_DEVICE_ALREADY_IN_DOMAIN
- [High] iommu/hyperv: tree corruption on zero or partial unmap in hv_iommu_unmap_pages()
- [Medium] iommu/hyperv: spurious hv_status_err() on memory deposit in hv_iommu_map_pgs()
- [Medium] iommu/hyperv: caller gfp_t flags ignored in hv_iommu_add_tree_mapping()
--

commit b814c086ae33fc3b6c22d26fe0a8b5dabc2d2f48
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
root partitions to manage paging domains, device attachment, and page
mappings via hypercalls. It hooks into pci_iommu_alloc() to detect and
initialize the Hyper-V root IOMMU during boot.

> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index 75cf8f6ae8cd0..b383c4549615a 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -8,6 +8,7 @@
>  #include <linux/gfp.h>
>  #include <linux/pci.h>
>  #include <linux/amd-iommu.h>
> +#include <linux/hyperv.h>
>  
>  #include <asm/proto.h>
>  #include <asm/dma.h>
> @@ -101,6 +102,7 @@ void __init pci_iommu_alloc(void)
>  	}
>  	pci_swiotlb_detect();
>  	gart_iommu_hole_init();
> +	hv_root_iommu_detect();
>  	amd_iommu_detect();
>  	detect_intel_iommu();
>  	swiotlb_init(x86_swiotlb_enable, x86_swiotlb_flags);

[Severity: High]
When hv_root_iommu_detect() succeeds, it sets iommu_detected = 1 and
assigns x86_init.iommu.iommu_init = hv_root_iommu_init. However,
amd_iommu_detect() and detect_intel_iommu() are called unconditionally
right after it in pci_iommu_alloc().

In drivers/iommu/intel/dmar.c:detect_intel_iommu(), whenever an ACPI DMAR
table with valid DRHD units is present, the assignment to
x86_init.iommu.iommu_init happens outside the !iommu_detected check:

	if (!iommu_detected && dmar_required()) {
		iommu_detected = 1;
		/* Make sure ACS will be enabled */
		pci_request_acs();
	}

	x86_init.iommu.iommu_init = intel_iommu_init;

Even if the user boots with intel_iommu=off (as suggested in the Kconfig
help text) or firmware sets DMAR_REMAP_OPT_OUT, detect_intel_iommu() still
overwrites x86_init.iommu.iommu_init with intel_iommu_init, which then
sees dmar_policy_off() and exits without initializing any IOMMU.

Similarly, in drivers/iommu/amd/init.c:amd_iommu_detect(), if an ACPI IVRS
table is present and amd_iommu=off was not passed, iommu_go_to_state()
succeeds without checking iommu_detected:

	ret = iommu_go_to_state(IOMMU_IVRS_DETECTED);
	if (ret)
		goto disable_snp;

	iommu_detected = 1;
	x86_init.iommu.iommu_init = amd_iommu_init;

This clobbers hv_root_iommu_init and later runs disable_iommus() against
the physical IOMMU registers owned by Hyper-V.

Won't amd_iommu_detect() and detect_intel_iommu() overwrite
x86_init.iommu.iommu_init on baremetal root partitions where ACPI DMAR or
IVRS tables are exposed?

[ ... ]

> diff --git a/drivers/iommu/hyperv/hv-iommu-root.c b/drivers/iommu/hyperv/hv-iommu-root.c
> new file mode 100644
> index 0000000000000..23212c9e44e49
> --- /dev/null
> +++ b/drivers/iommu/hyperv/hv-iommu-root.c
> @@ -0,0 +1,602 @@

[ ... ]

> +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);
> +	if (!mapping)
> +		return -ENOMEM;

[Severity: Medium]
As reported by Sashiko on v2 [1], hv_iommu_map_pages() receives a gfp_t
gfp parameter from the IOMMU core and passes it to
hv_deposit_memory_gfp(), but drops it when calling
hv_iommu_add_tree_mapping().

Since mapping is allocated before acquiring hvdom->mappings_lock, should
hv_iommu_add_tree_mapping() accept and use the caller's gfp flags instead
of hardcoding GFP_ATOMIC, which prevents memory reclaim and can fail with
-ENOMEM under memory pressure when called from sleepable GFP_KERNEL
contexts?

> +
> +	mapping->paddr = paddr;
> +	mapping->iova.start = iova;
> +	mapping->iova.last = iova + size - 1;
> +	mapping->flags = flags;
> +
> +	spin_lock_irqsave(&hvdom->mappings_lock, irqflags);
> +	interval_tree_insert(&mapping->iova, &hvdom->mappings_tree);
> +	spin_unlock_irqrestore(&hvdom->mappings_lock, irqflags);
> +
> +	return 0;
> +}
> +
> +/* If size == 0, then last = ULONG_MAX. With iova 0, will remove everything */
> +static size_t hv_iommu_del_tree_mappings(struct hv_iommu_domain *hvdom,
> +					 ulong iova, size_t size)
> +{
> +	ulong flags;
> +	size_t unmapped = 0;
> +	ulong last = iova + size - 1;
> +	struct hv_iommu_mapping *mapping = NULL;
> +	struct interval_tree_node *node, *next;
> +
> +	spin_lock_irqsave(&hvdom->mappings_lock, flags);
> +	next = interval_tree_iter_first(&hvdom->mappings_tree, iova, last);
> +	while (next) {
> +		node = next;
> +		mapping = container_of(node, struct hv_iommu_mapping, iova);
> +		next = interval_tree_iter_next(node, iova, last);
> +
> +		/* Splitting of a mapping is not supported at present */
> +		if (mapping->iova.start < iova)
> +			break;
> +
> +		unmapped += mapping->iova.last - mapping->iova.start + 1;
> +
> +		interval_tree_remove(node, &hvdom->mappings_tree);
> +		kfree(mapping);
> +	}
> +	spin_unlock_irqrestore(&hvdom->mappings_lock, flags);
> +
> +	return unmapped;
> +}

[ ... ]

> +static int hv_iommu_att_dev2dom(struct hv_iommu_domain *hvdom,
> +				struct pci_dev *pdev)
> +{
> +	ulong flags;
> +	u64 status;
> +	struct hv_input_attach_device_domain *input;
> +
> +	local_irq_save(flags);
> +	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	memset(input, 0, sizeof(*input));
> +
> +	/* For null domain, hvdom->domid_num == HV_DEVICE_DOMAIN_ID_NULL */
> +	input->device_domain.partition_id = HV_PARTITION_ID_SELF;
> +	input->device_domain.domain_id.type = HV_DEVICE_DOMAIN_TYPE_S2;
> +	input->device_domain.domain_id.id = hvdom->domid_num;
> +
> +	input->device_id.as_uint64 = hv_build_devid_type_pci(pdev);
> +
> +	status = hv_do_hypercall(HVCALL_ATTACH_DEVICE_DOMAIN, input, NULL);
> +	local_irq_restore(flags);
> +
> +	if (!hv_result_success(status))
> +		hv_status_err(status, "\n");
> +
> +	return hv_result_to_errno(status);
> +}
> +
> +static int hv_iommu_attach_dev(struct iommu_domain *immdom, struct device *dev,
> +			       struct iommu_domain *old)
> +{
> +	struct pci_dev *pdev;
> +	int rc;
> +	struct hv_iommu_domain *hvdom = to_hv_domain(immdom);
> +
> +	pdev = to_pci_dev(dev);
> +
> +	rc = hv_iommu_att_dev2dom(hvdom, pdev);
> +	if (rc)
> +		WARN(1, "Failed to attach pdev:%s\n", pci_name(pdev));
> +
> +	return rc;
> +}

[Severity: High]
In hv_iommu_att_dev2dom(), any hypercall status other than
HV_STATUS_SUCCESS is treated as an error, converted to -EEXIST on
HV_STATUS_DEVICE_ALREADY_IN_DOMAIN, and triggers the WARN(1) in
hv_iommu_attach_dev().

There are two normal paths where HVCALL_ATTACH_DEVICE_DOMAIN returns
HV_STATUS_DEVICE_ALREADY_IN_DOMAIN:

1. At boot, Hyper-V places root partition PCI devices in the Stage 2
   default domain (HV_DEVICE_DOMAIN_ID_DEFAULT), which corresponds to
   hv_def_identity_dom (IOMMU_DOMAIN_IDENTITY). When attaching a device to
   hv_def_identity_dom (for example, when booting with
   iommu.passthrough=1 or CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y), the device
   is already in HV_DEVICE_DOMAIN_ID_DEFAULT.

2. When an iommu_group created by pci_device_group() contains multiple PCI
   devices sharing a DMA requester ID alias (such as devices behind a
   PCIe-to-PCI/PCI-X bridge or multifunction devices with DMA alias
   quirks), hv_build_devid_type_pci() resolves each device in the group to
   the same hv_device_id via pci_for_each_dma_alias(). When
   __iommu_group_set_domain_internal() iterates over group->devices
   calling hv_iommu_attach_dev(), the second aliased device attempts to
   attach the same hv_device_id to the same domain.

By comparison, drivers/iommu/hyperv/hv-iommu-guest.c:hv_iommu_attach_dev()
explicitly ignores HV_STATUS_DEVICE_ALREADY_IN_DOMAIN:

	if (!hv_result_success(status) &&
	    hv_result(status) != HV_STATUS_DEVICE_ALREADY_IN_DOMAIN) {
		hv_status_err(status, "HVCALL_ATTACH_DEVICE_DOMAIN failed\n");
		return hv_result_to_errno(status);
	}

Should hv_iommu_att_dev2dom() also treat
HV_STATUS_DEVICE_ALREADY_IN_DOMAIN as success?

[ ... ]

> +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]
As reported by Sashiko on v2 [1], hv_iommu_unmap_pages() and
hv_iommu_del_tree_mappings() can corrupt hvdom->mappings_tree when an
unmap fails, partially completes, or covers a sub-range of a multi-page
mapping:

1. In hv_iommu_del_tree_mappings(), last is computed as:

	ulong last = iova + size - 1;

   hv_iommu_domain_free() relies on iova == 0 and size == 0 wrapping last
   to ULONG_MAX to delete all mappings in the tree. However,
   hv_iommu_unmap_pages() passes unmap_sz directly to
   hv_iommu_del_tree_mappings(hvdom, iova, unmap_sz) without checking if
   unmap_sz == 0. If hv_iommu_hyp_unmap_pages() fails on the first batch
   and returns 0 at iova == 0 (which is valid since aperture_start is 0),
   last underflows to ULONG_MAX and hv_iommu_del_tree_mappings() deletes
   every mapping in the domain even though nothing was unmapped in the
   hypervisor. If iova > 0 and unmap_sz == 0, last wraps to iova - 1 and
   interval_tree_iter_first(&hvdom->mappings_tree, iova, iova - 1) will
   still match and delete any mapping spanning across iova - 1 and iova.

2. hv_iommu_map_pages() inserts a single struct hv_iommu_mapping node
   covering [iova, iova + size - 1] for multi-page requests. In
   hv_iommu_del_tree_mappings(), the loop checks:

	if (mapping->iova.start < iova)
		break;

   but never checks if mapping->iova.last > last. If unmap_sz < size (due
   to a partial hypercall unmap or a partial unmap of a multi-page node's
   prefix), hv_iommu_del_tree_mappings() removes and frees the entire
   hv_iommu_mapping node and returns its full size while
   [last + 1, mapping->iova.last] remains mapped in the hypervisor.

3. Because hv_iommu_hyp_unmap_pages() runs before
   hv_iommu_del_tree_mappings() validates the interval tree boundaries, an
   unmap starting in the middle of an existing mapping
   (mapping->iova.start < iova) will unmap the pages in the hypervisor
   first, and then hv_iommu_del_tree_mappings() will break out of the loop
   and return 0, leaving the stale mapping in hvdom->mappings_tree.

Could hv_iommu_unmap_pages() validate the range against
hvdom->mappings_tree before unmapping in the hypervisor, and guard against
unmap_sz == 0 and mapping->iova.last > last in
hv_iommu_del_tree_mappings()?

> +
> +/* Return: must return exact status from the hypercall without changes */
> +static u64 hv_iommu_map_pgs(struct hv_iommu_domain *hvdom, ulong iova,
> +			    phys_addr_t paddr, ulong npages, u32 map_flags)
> +{
> +	u64 status;
> +	int i;
> +	struct hv_input_map_device_gpa_pages *input;
> +	unsigned long flags, pfn;
> +
> +	local_irq_save(flags);
> +	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	memset(input, 0, sizeof(*input));
> +
> +	input->device_domain.partition_id = HV_PARTITION_ID_SELF;
> +	input->device_domain.domain_id.type = HV_DEVICE_DOMAIN_TYPE_S2;
> +	input->device_domain.domain_id.id = hvdom->domid_num;
> +	input->map_flags = map_flags;
> +	input->target_device_va_base = iova;
> +
> +	pfn = paddr >> HV_HYP_PAGE_SHIFT;
> +	for (i = 0; i < npages; i++, pfn++)
> +		input->gpa_page_list[i] = pfn;
> +
> +	status = hv_do_rep_hypercall(HVCALL_MAP_DEVICE_GPA_PAGES, npages, 0,
> +				     input, NULL);
> +	local_irq_restore(flags);
> +
> +	if (!hv_result_success(status))
> +		hv_status_err(status, "npgs:%lx paddr:%llx iova:%lx mf:0x%x\n",
> +			      npages, paddr, iova, map_flags);
> +	return status;
> +}
> +
> +#define HV_MAP_DEVICE_GPA_BATCH_SIZE   \
> +	((HV_HYP_PAGE_SIZE - sizeof(struct hv_input_map_device_gpa_pages)) \
> +			/ sizeof(u64))
> +
> +/* NB: cond_resched() is in vfio_iommu_map() */
> +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_iommu_domain *hvdom = to_hv_domain(immdom);
> +	size_t size = pgsize * pgcount;
> +
> +	map_flags = HV_MAP_GPA_READABLE;	/* required */
> +	map_flags |= prot & IOMMU_WRITE ? HV_MAP_GPA_WRITABLE : 0;
> +
> +	ret = hv_iommu_add_tree_mapping(hvdom, iova, paddr, size, map_flags);
> +	if (ret)
> +		return ret;
> +
> +	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_deposit_memory_gfp(hv_current_partition_id,
> +						    status, gfp);
> +			if (ret)
> +				break;
> +			continue;
> +		}
> +		if (!hv_result_success(status))
> +			break;
> +	}
> +
> +	if (hv_result_success(status)) {
> +		if (mapped)
> +			*mapped = size;
> +	} else {
> +		size_t done_size = done << HV_HYP_PAGE_SHIFT;
> +
> +		hv_status_err(status, "pgs:%lx/%lx iova:%lx\n", done, npages,
> +			      sav_iova);
> +
> +		hv_iommu_del_tree_mappings(hvdom, sav_iova, size);
> +
> +		/* Hypervisor guarantees unmap will succeed here */
> +		hv_iommu_hyp_unmap_pages(immdom, sav_iova, done_size);
> +
> +		if (mapped)
> +			*mapped = 0;
> +	}
> +
> +	return hv_result_to_errno(status);
> +}

[Severity: Medium]
When HVCALL_MAP_DEVICE_GPA_PAGES returns a status where
hv_result_needs_memory(status) is true so that hv_iommu_map_pages() can
deposit memory via hv_deposit_memory_gfp() and retry, hv_iommu_map_pgs()
checks:

	if (!hv_result_success(status))
		hv_status_err(status, "npgs:%lx paddr:%llx iova:%lx mf:0x%x\n",
			      npages, paddr, iova, map_flags);

Won't this log a false KERN_ERR message every time the hypervisor requests
a normal memory deposit during DMA mapping, and log hv_status_err() twice
when a genuine hypercall failure occurs (once in hv_iommu_map_pgs() and
again in the error path of hv_iommu_map_pages())? Also, if
hv_deposit_memory_gfp() fails with a non-zero ret, its error code is
discarded at the end of hv_iommu_map_pages() in favor of
hv_result_to_errno(status).

[ ... ]

> +static int hv_iommu_get_caps(struct hv_output_get_iommu_capabilities *caps)
> +{
> +	u64 status;
> +	unsigned long flags;
> +	struct hv_input_get_iommu_capabilities *input;
> +	struct hv_output_get_iommu_capabilities *output;
> +
> +	local_irq_save(flags);
> +
> +	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> +	memset(input, 0, sizeof(*input));
> +	input->partition_id = HV_PARTITION_ID_SELF;
> +	status = hv_do_hypercall(HVCALL_GET_IOMMU_CAPABILITIES, input, output);
> +	*caps = *output;
> +
> +	local_irq_restore(flags);
> +
> +	if (!hv_result_success(status))
> +		hv_status_err(status, "\n");
> +
> +	return hv_result_to_errno(status);
> +}

[ ... ]

> +void __init hv_root_iommu_detect(void)
> +{
> +	if (no_iommu || iommu_detected || hv_l1vh_partition())
> +		return;
> +
> +	if (!(ms_hyperv.misc_features & HV_DEVICE_DOMAIN_AVAILABLE))
> +		return;
> +
> +	iommu_detected = 1;
> +	x86_init.iommu.iommu_init = hv_root_iommu_init;
> +
> +	pci_request_acs();
> +}

[Severity: High]
CONFIG_HYPERV_ROOT_IOMMU defaults to CONFIG_HYPERV when CONFIG_MSHV_ROOT
is enabled, so a single kernel image can boot in both root partitions
(HV_PARTITION_TYPE_ROOT) and regular guest VMs (HV_PARTITION_TYPE_GUEST).

In hv_root_iommu_detect(), checking hv_l1vh_partition() only excludes
HV_PARTITION_TYPE_L1VH, not HV_PARTITION_TYPE_GUEST. When a regular guest
VM boots with HV_DEVICE_DOMAIN_AVAILABLE advertised in
ms_hyperv.misc_features (for example, when guest pvIOMMU is enabled),
hv_root_iommu_detect() will set iommu_detected = 1 (causing
hv_pviommu_init() to abort with -ENODEV) and register hv_root_iommu_init.

When pci_iommu_init() then runs hv_root_iommu_init() ->
hv_iommu_get_caps(), it dereferences hyperv_pcpu_output_arg:

	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
	...
	*caps = *output;

However, drivers/hv/hv_common.c:hv_common_init() only allocates
hyperv_pcpu_output_arg when hv_output_page_exists() is true:

	/* Allocate the per-CPU state for output arg for root */
	if (hv_output_page_exists()) {
		hyperv_pcpu_output_arg = alloc_percpu(void *);
		BUG_ON(!hyperv_pcpu_output_arg);
	}

In a regular guest VM, hyperv_pcpu_output_arg is NULL, causing a NULL
pointer dereference during boot.

Should hv_root_iommu_detect() check !hv_root_partition() instead of
hv_l1vh_partition()?

[1] https://lore.kernel.org/r/20260929223622.778617-5-mrathor@linux.microsoft.com

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