On 13/10/2023 17:22, Jason Gunthorpe wrote:
> 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.
>
Yes, let me remove it
>> + /* 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?
>
yes, it does
> I can sort of see restricting the start/stop iova
>
There's no fundamental reason to restricting it; I am probably just too obsessed
with making the most granular tracking, but I shouldn't restrict the user to
track at some other page granularity
>
>> + 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
>
OK
>> #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?
>
I was just reducing how much data I really need to pass around so consolidated
all that into a struct representing the bitmap data considering (...)
>> +/**
>> + * 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
I use it for the read_and_clear_dirty_data (and it's input validation). Kinda
weird to do:
iommu_read_and_clear(domain, flags, cmd)
Considering none of those functions pass command data around. If you prefer with
passing the whole command then I can go at it;
