On 4/29/22 13:08, Jason Gunthorpe wrote:
> On Thu, Apr 28, 2022 at 10:09:15PM +0100, Joao Martins wrote:
>> +
>> +unsigned int iommu_dirty_bitmap_record(struct iommu_dirty_bitmap *dirty,
>> + unsigned long iova, unsigned long length)
>> +{
>
> Lets put iommu_dirty_bitmap in its own patch, the VFIO driver side
> will want to use this same data structure.
>
OK.
>> + while (nbits > 0) {
>> + kaddr = kmap(dirty->pages[idx]) + start_offset;
>
> kmap_local?
>
/me nods
>> +/**
>> + * struct iommu_dirty_bitmap - Dirty IOVA bitmap state
>> + *
>> + * @iova: IOVA representing the start of the bitmap, the first bit of the bitmap
>> + * @pgshift: Page granularity of the bitmap
>> + * @gather: Range information for a pending IOTLB flush
>> + * @start_offset: Offset of the first user page
>> + * @pages: User pages representing the bitmap region
>> + * @npages: Number of user pages pinned
>> + */
>> +struct iommu_dirty_bitmap {
>> + unsigned long iova;
>> + unsigned long pgshift;
>> + struct iommu_iotlb_gather *gather;
>> + unsigned long start_offset;
>> + unsigned long npages;
>> + struct page **pages;
>
> In many (all?) cases I would expect this to be called from a process
> context, can we just store the __user pointer here, or is the idea
> that with modern kernels poking a u64 to userspace is slower than a
> kmap?
>
I have both options implemented, I'll need to measure it. Code-wise it would be
a lot simpler to just poke at the userspace addresses (that was my first
prototype of this) but felt that poking at kernel addresses was safer and
avoid assumptions over the context (from the iommu driver). I can bring back
the former alternative if this was the wrong thing to do.
> I'm particularly concerend that this starts to require high
> order allocations with more than 2M of bitmap.. Maybe one direction is
> to GUP 2M chunks at a time and walk the __user pointer.
>
That's what I am doing here. We GUP 2M of *bitmap* at a time.
Which is about 1 page for the struct page pointers. That is enough
for 64G of IOVA dirties read worst-case scenario (i.e. with base pages).
>> +static inline void iommu_dirty_bitmap_init(struct iommu_dirty_bitmap *dirty,
>> + unsigned long base,
>> + unsigned long pgshift,
>> + struct iommu_iotlb_gather *gather)
>> +{
>> + memset(dirty, 0, sizeof(*dirty));
>> + dirty->iova = base;
>> + dirty->pgshift = pgshift;
>> + dirty->gather = gather;
>> +
>> + if (gather)
>> + iommu_iotlb_gather_init(dirty->gather);
>> +}
>
> I would expect all the GUPing logic to be here too?
I had this in the iommufd_dirty_iter logic given that the iommu iteration
logic is in the parent structure that stores iommu_dirty_data.
My thinking with this patch was just to have what the IOMMU driver needs.
Which actually if anything this helper above ought to be in a later patch.
