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

[Mostafa Saleh <smostafa@google.com>]

Hi Jean,

On Wed, Feb 01, 2023 at 12:53:04PM +0000, Jean-Philippe Brucker wrote:
> Handle map() and unmap() hypercalls by calling the io-pgtable library.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  arch/arm64/kvm/hyp/nvhe/iommu/iommu.c | 144 ++++++++++++++++++++++++++
>  1 file changed, 144 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c b/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
> index 7404ea77ed9f..0550e7bdf179 100644
> --- a/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
> +++ b/arch/arm64/kvm/hyp/nvhe/iommu/iommu.c
> @@ -183,6 +183,150 @@ int kvm_iommu_detach_dev(pkvm_handle_t iommu_id, pkvm_handle_t domain_id,
>  	return ret;
>  }
>  
> +static int __kvm_iommu_unmap_pages(struct io_pgtable *iopt, unsigned long iova,
> +				   size_t pgsize, size_t pgcount)
> +{
> +	int ret;
> +	size_t unmapped;
> +	phys_addr_t paddr;
> +	size_t total_unmapped = 0;
> +	size_t size = pgsize * pgcount;
> +
> +	while (total_unmapped < size) {
> +		paddr = iopt_iova_to_phys(iopt, iova);
> +		if (paddr == 0)
> +			return -EINVAL;
> +
> +		/*
> +		 * One page/block at a time, because the range provided may not
> +		 * be physically contiguous, and we need to unshare all physical
> +		 * pages.
> +		 */
> +		unmapped = iopt_unmap_pages(iopt, iova, pgsize, 1, NULL);
> +		if (!unmapped)
> +			return -EINVAL;
> +
> +		ret = __pkvm_host_unshare_dma(paddr, pgsize);
> +		if (ret)
> +			return ret;
> +
> +		iova += unmapped;
> +		pgcount -= unmapped / pgsize;
> +		total_unmapped += unmapped;
> +	}
> +
> +	return 0;
> +}
> +
> +#define IOMMU_PROT_MASK (IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE |\
> +			 IOMMU_NOEXEC | IOMMU_MMIO)
> +
> +int kvm_iommu_map_pages(pkvm_handle_t iommu_id, pkvm_handle_t domain_id,
> +			unsigned long iova, phys_addr_t paddr, size_t pgsize,
> +			size_t pgcount, int prot)
> +{
> +	size_t size;
> +	size_t granule;
> +	int ret = -EINVAL;
> +	size_t mapped = 0;
> +	struct io_pgtable iopt;
> +	struct kvm_hyp_iommu *iommu;
> +	size_t pgcount_orig = pgcount;
> +	unsigned long iova_orig = iova;
> +	struct kvm_hyp_iommu_domain *domain;
> +
> +	if (prot & ~IOMMU_PROT_MASK)
> +		return -EINVAL;
> +
> +	if (__builtin_mul_overflow(pgsize, pgcount, &size) ||
> +	    iova + size < iova || paddr + size < paddr)
> +		return -EOVERFLOW;
> +
> +	hyp_spin_lock(&iommu_lock);
> +
> +	domain = handle_to_domain(iommu_id, domain_id, &iommu);
> +	if (!domain)
> +		goto err_unlock;
> +
> +	granule = 1 << __ffs(iommu->pgtable->cfg.pgsize_bitmap);
> +	if (!IS_ALIGNED(iova | paddr | pgsize, granule))
> +		goto err_unlock;
> +
> +	ret = __pkvm_host_share_dma(paddr, size, !(prot & IOMMU_MMIO));
> +	if (ret)
> +		goto err_unlock;
> +
> +	iopt = domain_to_iopt(iommu, domain, domain_id);
> +	while (pgcount) {
> +		ret = iopt_map_pages(&iopt, iova, paddr, pgsize, pgcount, prot,
> +				     0, &mapped);
> +		WARN_ON(!IS_ALIGNED(mapped, pgsize));
> +		pgcount -= mapped / pgsize;
> +		if (ret)
> +			goto err_unmap;
> +		iova += mapped;
> +		paddr += mapped;
> +	}
> +
> +	hyp_spin_unlock(&iommu_lock);
> +	return 0;
> +
> +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.

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.

And set to true from kvm_iommu_unmap_pages.

> +err_unlock:
> +	hyp_spin_unlock(&iommu_lock);
> +	return ret;
> +}
> +
> +int kvm_iommu_unmap_pages(pkvm_handle_t iommu_id, pkvm_handle_t domain_id,
> +			  unsigned long iova, size_t pgsize, size_t pgcount)
> +{
> +	size_t size;
> +	size_t granule;
> +	int ret = -EINVAL;
> +	struct io_pgtable iopt;
> +	struct kvm_hyp_iommu *iommu;
> +	struct kvm_hyp_iommu_domain *domain;
> +
> +	if (__builtin_mul_overflow(pgsize, pgcount, &size) ||
> +	    iova + size < iova)
> +		return -EOVERFLOW;
> +
> +	hyp_spin_lock(&iommu_lock);
> +	domain = handle_to_domain(iommu_id, domain_id, &iommu);
> +	if (!domain)
> +		goto out_unlock;
> +
> +	granule = 1 << __ffs(iommu->pgtable->cfg.pgsize_bitmap);
> +	if (!IS_ALIGNED(iova | pgsize, granule))
> +		goto out_unlock;
> +
> +	iopt = domain_to_iopt(iommu, domain, domain_id);
> +	ret = __kvm_iommu_unmap_pages(&iopt, iova, pgsize, pgcount);
> +out_unlock:
> +	hyp_spin_unlock(&iommu_lock);
> +	return ret;
> +}
> +
> +phys_addr_t kvm_iommu_iova_to_phys(pkvm_handle_t iommu_id,
> +				   pkvm_handle_t domain_id, unsigned long iova)
> +{
> +	phys_addr_t phys = 0;
> +	struct io_pgtable iopt;
> +	struct kvm_hyp_iommu *iommu;
> +	struct kvm_hyp_iommu_domain *domain;
> +
> +	hyp_spin_lock(&iommu_lock);
> +	domain = handle_to_domain(iommu_id, domain_id, &iommu);
> +	if (domain) {
> +		iopt = domain_to_iopt(iommu, domain, domain_id);
> +
> +		phys = iopt_iova_to_phys(&iopt, iova);
> +	}
> +	hyp_spin_unlock(&iommu_lock);
> +	return phys;
> +}
> +
>  int kvm_iommu_init_device(struct kvm_hyp_iommu *iommu)
>  {
>  	void *domains;
> -- 
> 2.39.0

Thanks,
Mostafa