Re: [PATCH v3 10/19] iommufd: Add IOMMU_HWPT_GET_DIRTY_IOVA

[Jason Gunthorpe <jgg@nvidia.com>]

On Sat, Sep 23, 2023 at 02:25:02AM +0100, Joao Martins wrote:

> +int iommufd_check_iova_range(struct iommufd_ioas *ioas,
> +			     struct iommufd_dirty_data *bitmap)
> +{
> +	unsigned long pgshift, npages;
> +	size_t iommu_pgsize;
> +	int rc = -EINVAL;
> +
> +	pgshift = __ffs(bitmap->page_size);
> +	npages = bitmap->length >> pgshift;
> +
> +	if (!npages || (npages > ULONG_MAX))
> +		return rc;
> +
> +	iommu_pgsize = 1 << __ffs(ioas->iopt.iova_alignment);

iova_alignment is not a bitmask, it is the alignment itself, so is
redundant.

> +	/* allow only smallest supported pgsize */
> +	if (bitmap->page_size != iommu_pgsize)
> +		return rc;

!= is smallest?

Why are we restricting this anyhow? I thought the iova bitmap stuff
did all the adaptation automatically?

I can sort of see restricting the start/stop iova


> +	if (bitmap->iova & (iommu_pgsize - 1))
> +		return rc;
> +
> +	if (!bitmap->length || bitmap->length & (iommu_pgsize - 1))
> +		return rc;
> +
> +	return 0;
> +}

> --- a/drivers/iommu/iommufd/main.c
> +++ b/drivers/iommu/iommufd/main.c
> @@ -316,6 +316,7 @@ union ucmd_buffer {
>  	struct iommu_option option;
>  	struct iommu_vfio_ioas vfio_ioas;
>  	struct iommu_hwpt_set_dirty set_dirty;
> +	struct iommu_hwpt_get_dirty_iova get_dirty_iova;
>  #ifdef CONFIG_IOMMUFD_TEST
>  	struct iommu_test_cmd test;
>  #endif
> @@ -361,6 +362,8 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
>  		 __reserved),
>  	IOCTL_OP(IOMMU_HWPT_SET_DIRTY, iommufd_hwpt_set_dirty,
>  		 struct iommu_hwpt_set_dirty, __reserved),
> +	IOCTL_OP(IOMMU_HWPT_GET_DIRTY_IOVA, iommufd_hwpt_get_dirty_iova,
> +		 struct iommu_hwpt_get_dirty_iova, bitmap.data),

Also keep sorted

>  #ifdef CONFIG_IOMMUFD_TEST
>  	IOCTL_OP(IOMMU_TEST_CMD, iommufd_test, struct iommu_test_cmd, last),
>  #endif
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index 37079e72d243..b35b7d0c4be0 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -48,6 +48,7 @@ enum {
>  	IOMMUFD_CMD_HWPT_ALLOC,
>  	IOMMUFD_CMD_GET_HW_INFO,
>  	IOMMUFD_CMD_HWPT_SET_DIRTY,
> +	IOMMUFD_CMD_HWPT_GET_DIRTY_IOVA,
>  };
>  
>  /**
> @@ -481,4 +482,39 @@ struct iommu_hwpt_set_dirty {
>  	__u32 __reserved;
>  };
>  #define IOMMU_HWPT_SET_DIRTY _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_SET_DIRTY)
> +
> +/**
> + * struct iommufd_dirty_bitmap - Dirty IOVA tracking bitmap
> + * @iova: base IOVA of the bitmap
> + * @length: IOVA size
> + * @page_size: page size granularity of each bit in the bitmap
> + * @data: bitmap where to set the dirty bits. The bitmap bits each
> + * represent a page_size which you deviate from an arbitrary iova.
> + * Checking a given IOVA is dirty:
> + *
> + *  data[(iova / page_size) / 64] & (1ULL << (iova % 64))
> + */
> +struct iommufd_dirty_data {
> +	__aligned_u64 iova;
> +	__aligned_u64 length;
> +	__aligned_u64 page_size;
> +	__aligned_u64 *data;
> +};

Is there a reason to add this struct? Does something else use it?

> +/**
> + * struct iommu_hwpt_get_dirty_iova - ioctl(IOMMU_HWPT_GET_DIRTY_IOVA)
> + * @size: sizeof(struct iommu_hwpt_get_dirty_iova)
> + * @hwpt_id: HW pagetable ID that represents the IOMMU domain.
> + * @flags: Flags to control dirty tracking status.
> + * @bitmap: Bitmap of the range of IOVA to read out
> + */
> +struct iommu_hwpt_get_dirty_iova {
> +	__u32 size;
> +	__u32 hwpt_id;
> +	__u32 flags;
> +	__u32 __reserved;
> +	struct iommufd_dirty_data bitmap;

vs inlining here?

I see you are passing it around the internal API, but that could
easily pass the whole command too

Jason