Re: [PATCH v14 Kernel 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.

[Alex Williamson]

On Fri, 20 Mar 2020 01:55:10 +0530
Kirti Wankhede <address@hidden> wrote:

> On 3/19/2020 9:52 PM, Alex Williamson wrote:
> > On Thu, 19 Mar 2020 20:22:41 +0530
> > Kirti Wankhede <address@hidden> wrote:
> >   
> >> On 3/19/2020 9:15 AM, Alex Williamson wrote:  
> >>> On Thu, 19 Mar 2020 01:11:11 +0530
> >>> Kirti Wankhede <address@hidden> wrote:
> >>>      
> 
> <snip>
> 
> >>>> +
> >>>> +static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
> >>>> +{
> >>>> +        uint64_t bsize;
> >>>> +
> >>>> +        if (!npages || !bitmap_size || bitmap_size > UINT_MAX)  
> >>>
> >>> As commented previously, how do we derive this UINT_MAX limitation?
> >>>      
> >>
> >> Sorry, I missed that earlier
> >>  
> >>   > UINT_MAX seems arbitrary, is this specified in our API?  The size of a
> >>   > vfio_dma is limited to what the user is able to pin, and therefore
> >>   > their locked memory limit, but do we have an explicit limit elsewhere
> >>   > that results in this limit here.  I think a 4GB bitmap would track
> >>   > something like 2^47 bytes of memory, that's pretty excessive, but still
> >>   > an arbitrary limit.  
> >>
> >> There has to be some upper limit check. In core KVM, in
> >> virt/kvm/kvm_main.c there is max number of pages check:
> >>
> >> if (new.npages > KVM_MEM_MAX_NR_PAGES)
> >>
> >> Where
> >> /*
> >>    * Some of the bitops functions do not support too long bitmaps.
> >>    * This number must be determined not to exceed such limits.
> >>    */
> >> #define KVM_MEM_MAX_NR_PAGES ((1UL << 31) - 1)
> >>
> >> Though I don't know which bitops functions do not support long bitmaps.
> >>
> >> Something similar as above can be done or same as you also mentioned of
> >> 4GB bitmap limit? that is U32_MAX instead of UINT_MAX?  
> > 
> > Let's see, we use bitmap_set():
> > 
> > void bitmap_set(unsigned long *map, unsigned int start, unsigned int nbits)
> > 
> > So we're limited to an unsigned int number of bits, but for an
> > unaligned, multi-bit operation this will call __bitmap_set():
> > 
> > void __bitmap_set(unsigned long *map, unsigned int start, int len)
> > 
> > So we're down to a signed int number of bits (seems like an API bug in
> > bitops there), so it makes sense that KVM is testing against MAX_INT
> > number of pages, ie. number of bits.  But that still suggests a bitmap
> > size of MAX_UINT is off by a factor of 16.  So we can have 2^31 bits
> > divided by 2^3 bits/byte yields a maximum bitmap size of 2^28 (ie.
> > 256MB), which maps 2^31 * 2^12 = 2^43 (8TB) on a 4K system.
> > 
> > Let's fix the limit check and put a nice comment explaining it.  Thanks,
> >   
> 
> Agreed. Adding DIRTY_BITMAP_SIZE_MAX macro and comment as below.
> 
> /*
>   * Input argument of number of bits to bitmap_set() is unsigned 
> integer, which
>   * further casts to signed integer for unaligned multi-bit operation,
>   * __bitmap_set().
>   * Then maximum bitmap size supported is 2^31 bits divided by 2^3 
> bits/byte,
>   * that is 2^28 (256 MB) which maps to 2^31 * 2^12 = 2^43 (8TB) on 4K page
>   * system.
>   */
> #define DIRTY_BITMAP_PAGES_MAX  ((1UL << 31) - 1)

nit, can we just use INT_MAX here?

> #define DIRTY_BITMAP_SIZE_MAX         \
>                       DIRTY_BITMAP_BYTES(DIRTY_BITMAP_PAGES_MAX)
> 
> 
> Thanks,
> Kirti
>