On Fri, 26 Aug 2022 10:37:26 +0100 Joao Martins <joao.m.martins@xxxxxxxxxx> wrote: > On 8/26/22 00:15, Alex Williamson wrote: > > On Thu, 25 Aug 2022 23:24:39 +0100 > > Joao Martins <joao.m.martins@xxxxxxxxxx> 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
