On 8/26/22 13:02, Alex Williamson wrote: > 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: > /facepalm Oh yes -- even better! Let me try that along with the other comments. > struct iova_bitmap* iova_bitmap_alloc(dma_addr_t iova, size_t length, > size_t page_size, u64 __user *bitmap); > > Thanks, > Alex >
