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

[Joao Martins <joao.m.martins@oracle.com>]

On 8/26/22 13:02, Alex Williamson wrote:
> 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:
> 

/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
>