Re: [RFC PATCH 20/45] KVM: arm64: iommu: Add map() and unmap() operations

[Mostafa Saleh <smostafa@google.com>]

Hi Jean,

On Tue, Apr 04, 2023 at 05:00:46PM +0100, Jean-Philippe Brucker wrote:
> Hi Mostafa,
> 
> On Thu, Mar 30, 2023 at 06:14:04PM +0000, Mostafa Saleh wrote:
> > > +err_unmap:
> > > +	__kvm_iommu_unmap_pages(&iopt, iova_orig, pgsize, pgcount_orig - pgcount);
> > On error here, this unmaps (and unshares) only pages that has been
> > mapped.
> > But all pages where shared with IOMMU before (via
> > __pkvm_host_share_dma) and this corrupts the other pages state as
> > they are marked as shared while they are not.
> 
> Right, I'll fix this
> 
> > I see we can add a "bool unshare" arg to __kvm_iommu_unmap_pages which
> > will be called with false on error from here after calling
> > __pkvm_host_unshare_dma for the whole range.
> 
> I think it's simpler to call iopt_unmap_pages() directly here, followed by
> __pkvm_host_unshare_dma(). It even saves us a few lines
> 

I have been doing some testing based on the latest updates in
https://jpbrucker.net/git/linux/log/?h=pkvm/smmu

I think the unmap here is not enough as it assumes the whole range
can be unmapped in one call, so we would need something like this
instead (patch might no directly apply though):
diff --git a/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c b/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
index 7ebda87a1c61..32e145b9240f 100644
--- a/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
+++ b/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
@@ -432,6 +432,7 @@ int kvm_iommu_map_pages(pkvm_handle_t domain_id,
 	unsigned long iova_orig = iova;
 	struct kvm_hyp_iommu_domain *domain;
 	struct pkvm_hyp_vcpu *ctxt = pkvm_get_loaded_hyp_vcpu();
+	size_t unmapped;
 
 	if (!kvm_iommu_ops)
 		return -ENODEV;
@@ -489,8 +490,13 @@ int kvm_iommu_map_pages(pkvm_handle_t domain_id,
 
 err_unmap:
 	pgcount = pgcount_orig - pgcount;
-	if (pgcount)
-		iopt_unmap_pages(&iopt, iova_orig, pgsize, pgcount, NULL);
+	while (pgcount) {
+		unmapped = iopt_unmap_pages(&iopt, iova_orig, pgsize, pgcount, NULL);
+		iova_orig += unmapped;
+		pgcount -= unmapped/pgsize;
+		if (!unmapped)
+			break;
+	}
 	__pkvm_unshare_dma(paddr_orig, size);
 err_domain_put:
 	domain_put(domain);
-- 


Thanks,
Mostafa