Re: [PATCH v2 2/2] iommu/virtio: Return size mapped for a detached domain

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, May 15, 2023 at 12:39:50PM +0100, Jean-Philippe Brucker wrote:
> When map() is called on a detached domain, the domain does not exist in
> the device so we do not send a MAP request, but we do update the
> internal mapping tree, to be replayed on the next attach. Since this
> constitutes a successful iommu_map() call, return *mapped in this case
> too.
> 
> Fixes: 7e62edd7a33a ("iommu/virtio: Add map/unmap_pages() callbacks implementation")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/iommu/virtio-iommu.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index fd316a37d7562..3551ed057774e 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -833,25 +833,26 @@ static int viommu_map_pages(struct iommu_domain *domain, unsigned long iova,
>  	if (ret)
>  		return ret;
>  
> -	map = (struct virtio_iommu_req_map) {
> -		.head.type	= VIRTIO_IOMMU_T_MAP,
> -		.domain		= cpu_to_le32(vdomain->id),
> -		.virt_start	= cpu_to_le64(iova),
> -		.phys_start	= cpu_to_le64(paddr),
> -		.virt_end	= cpu_to_le64(end),
> -		.flags		= cpu_to_le32(flags),
> -	};
> +	if (vdomain->nr_endpoints) {
> +		map = (struct virtio_iommu_req_map) {
> +			.head.type	= VIRTIO_IOMMU_T_MAP,
> +			.domain		= cpu_to_le32(vdomain->id),
> +			.virt_start	= cpu_to_le64(iova),
> +			.phys_start	= cpu_to_le64(paddr),
> +			.virt_end	= cpu_to_le64(end),
> +			.flags		= cpu_to_le32(flags),
> +		};
>  
> -	if (!vdomain->nr_endpoints)
> -		return 0;
> -
> -	ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
> -	if (ret)
> -		viommu_del_mappings(vdomain, iova, end);
> -	else if (mapped)
> +		ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
> +		if (ret) {
> +			viommu_del_mappings(vdomain, iova, end);
> +			return ret;
> +		}
> +	}
> +	if (mapped)
>  		*mapped = size;

This looks right

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

But your unmap looks troubled:

	unmapped = viommu_del_mappings(vdomain, iova, iova + size - 1);
	if (unmapped < size)
		return 0;

It shouldn't unmap something and then return 0...

Also, these days this driver be much better to use a multi-order
xarray than in interval tree..

Jason