Re: [RFC v1 5/8] mshv: add paravirtualized IOMMU support

On Wed, Aug 04, 2021 at 12:13:42PM +0530, Praveen Kumar wrote:

On 04-08-2021 03:17, Wei Liu wrote:

quoted

quoted

quoted

+static size_t hv_iommu_unmap(struct iommu_domain *d, unsigned long iova,
+			   size_t size, struct iommu_iotlb_gather *gather)
+{
+	size_t unmapped;
+	struct hv_iommu_domain *domain = to_hv_iommu_domain(d);
+	unsigned long flags, npages;
+	struct hv_input_unmap_device_gpa_pages *input;
+	u64 status;
+
+	unmapped = hv_iommu_del_mappings(domain, iova, size);
+	if (unmapped < size)
+		return 0;

Is there a case where unmapped > 0 && unmapped < size ?

There could be such a case -- hv_iommu_del_mappings' return value is >= 0.
Is there a problem with this predicate?

What I understand, if we are unmapping and return 0, means nothing was
unmapped, and will that not cause any corruption or illegal access of
unmapped memory later?  From __iommu_unmap

Those pages are not really unmapped. The hypercall is skipped.

...
    13         while (unmapped < size) {
    12                 size_t pgsize = iommu_pgsize(domain, iova, size - unmapped);
    11
    10                 unmapped_page = ops->unmap(domain, iova, pgsize, iotlb_gather);
     9                 if (!unmapped_page)
     8                         break;		<<< we just break here, thinking there is nothing unmapped, but actually hv_iommu_del_mappings has removed some pages.
     7
     6                 pr_debug("unmapped: iova 0x%lx size 0x%zx\n",
     5                         ¦iova, unmapped_page);
     4
     3                 iova += unmapped_page;
     2                 unmapped += unmapped_page;
     1         }
...

Am I missing something ?

Regards,

~Praveen.