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
