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 <[email protected]> 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/[email protected]?part=4
