Re: [PATCH V4 vfio 04/10] vfio: Add an IOVA bitmap support

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 26 Aug 2022 10:37:26 +0100
Joao Martins <joao.m.martins@oracle.com> wrote:
> On 8/26/22 00:15, Alex Williamson wrote:
> > On Thu, 25 Aug 2022 23:24:39 +0100
> > Joao Martins <joao.m.martins@oracle.com> wrote:  
> >> On 8/25/22 20:27, Alex Williamson wrote:  
> >>> Maybe it doesn't really make sense to differentiate the iterator from
> >>> the bitmap in the API.  In fact, couldn't we reduce the API to simply:
> >>>
> >>> int iova_bitmap_init(struct iova_bitmap *bitmap, dma_addr_t iova,
> >>> 		     size_t length, size_t page_size, u64 __user *data);
> >>>
> >>> int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *data,
> >>> 			 int (*fn)(void *data, dma_addr_t iova,
> >>> 			 	   size_t length,
> >>> 				   struct iova_bitmap *bitmap));
> >>>
> >>> void iova_bitmap_free(struct iova_bitmap *bitmap);
> >>>
> >>> unsigned long iova_bitmap_set(struct iova_bitmap *bitmap,
> >>> 			      dma_addr_t iova, size_t length);
> >>>
> >>> Removes the need for the API to have done, advance, iova, and length
> >>> functions.
> >>>     
> >> True, it would be simpler.
> >>
> >> Could also allow us to hide the iterator details enterily and switch to
> >> container_of() from iova_bitmap pointer. Though, from caller, it would be
> >> weird to do:
> >>
> >> struct iova_bitmap_iter iter;
> >>
> >> iova_bitmap_init(&iter.dirty, ....);
> >>
> >> Hmm, maybe not that strange.
> >>
> >> Unless you are trying to suggest to merge both struct iova_bitmap and
> >> iova_bitmap_iter together? I was trying to keep them separate more for
> >> the dirty tracker (IOMMUFD/VFIO, to just be limited to iova_bitmap_set()
> >> with the generic infra being the one managing that iterator state in a
> >> separate structure.  
> > 
> > Not suggesting the be merged, but why does the embedded mapping
> > structure need to be exposed to the API?  That's an implementation
> > detail that's causing confusion and naming issues for which structure
> > is passed and how do we represent that in the function name.  Thanks,  
> 
> I wanted the convention to be that the end 'device' tracker (IOMMU or VFIO
> vendor driver) does not have "direct" access to the iterator state. So it acesses
> or modifies only the mapped bitmap *data*. The hardware tracker is always *provided*
> with a iova_bitmap to set bits but it should not allocate, iterate or pin anything,
> making things simpler for tracker.
> 
> Thus the point was to have a clear division between how you iterate
> (iova_bitmap_iter* API) and the actual bits manipulation (so far only
> iova_bitmap_set()) including which data structures you access in the APIs, thus
> embedding the least accessed there (struct iova_bitmap).
> 
> The alternative is to reverse it and just allocate iter state in iova_bitmap_init()
> and have it stored as a pointer say as iova_bitmap::iter. We encapsulate both and mix
> the structures, which while not as clean but maybe this is not that big of a deal as
> I thought it would be

Is there really a need for struct iova_bitmap to be defined in a shared
header, or could we just have a forward declaration?  With the proposed
interface above, iova_bitmap could be opaque to the caller if it were
dynamically allocated, ex:

struct iova_bitmap* iova_bitmap_alloc(dma_addr_t iova, size_t length,
				      size_t page_size, u64 __user *bitmap);

Thanks,
Alex