Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] iommu: Flawed partial unmap logic causes memory leak and over-unmap in hv_iommu_map_pages - [High] iommu: Incorrect assignment of IOVA aperture_end restricts DMA address space --
commit c33bf0d2ad36d04023ab67f38b30621e50f3b924 Author: Mukesh R <[email protected]> x86/hyperv: Implement root VM IOMMU kernel only driver Adds a new kernel-only virtual IOMMU driver that works with the Microsoft Hyper-V hypervisor on privileged root VMs. The driver communicates with the hypervisor for managing paging domains and device mappings. > diff --git a/drivers/iommu/hyperv/hv-iommu-root.c > b/drivers/iommu/hyperv/hv-iommu-root.c > new file mode 100644 > index 0000000000000..a5268e0e52cc2 > --- /dev/null > +++ b/drivers/iommu/hyperv/hv-iommu-root.c > @@ -0,0 +1,645 @@ [ ... ] > +static struct iommu_domain *hv_iommu_domain_alloc_paging(struct device *dev) > +{ [ ... ] > + hvdom->domid_num = unique_id; > + > + hvdom->iommu_dom.pgsize_bitmap = HV_IOMMU_PGSIZES; > + hvdom->iommu_dom.geometry.aperture_start = 0; > + hvdom->iommu_dom.geometry.aperture_end = hv_max_iova_width; [Severity: High] Does this directly assign the bit-width value to the maximum address boundary instead of creating an address mask? The hypervisor capability max_iova_width returns the bit-width of the supported IOVA space (e.g., 39 or 48 bits), not the maximum memory address mask. Since the IOMMU core uses aperture_end as the absolute maximum address for IOVA allocations, this seems to restrict the DMA address space to a scalar bit-width value, which could cause any DMA mapping requested above address 39 or 48 to fail. > + hvdom->iommu_dom.geometry.force_aperture = true; > + hvdom->iommu_dom.ops = &hv_paging_domain_ops; [ ... ] > +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) > +{ [ ... ] > + if (!hv_result_success(status)) { > + size_t done_size = done << HV_HYP_PAGE_SHIFT; > + > + hv_status_err(status, "pgs:%lx/%lx iova:%lx\n", > + done, npages, iova); > + /* > + * lookup tree has all mappings [0 - size-1]. Below unmap will > + * only remove from [0 - done], we need to remove second chunk > + * [done+1 - size-1]. > + */ > + hv_iommu_del_tree_mappings(hvdom, iova, size - done_size); > + hv_iommu_unmap_pages(immdom, iova - done_size, HV_HYP_PAGE_SIZE, > + done, NULL); [Severity: High] If a batched mapping operation fails midway, does this error path inadvertently leak mappings and over-unmap pages? When splitting the software tree mapping, the error path calls hv_iommu_del_tree_mappings(), which blocks removing the unmapped tail because it does not support splitting: drivers/iommu/hyperv/hv-iommu-root.c:hv_iommu_del_tree_mappings() { ... /* Splitting of a mapping is not supported at present */ if (mapping->iova.start < iova) break; ... } Subsequently, does the head unmap via hv_iommu_unmap_pages() delete the entire software node and use its full original size to unmap pages from the hypervisor? This seems like it would send out-of-bounds unmap requests for pages never successfully mapped, and abandon the correctly mapped pages since the software node is destroyed. > + if (mapped) > + *mapped = 0; > + } else [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
