Message-ID: <20220901143625.4cbd3394.alex.williamson@redhat.com>
Date:   Thu, 1 Sep 2022 14:36:25 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     joao.m.martins@...cle.com
Cc:     Yishai Hadas <yishaih@...dia.com>, jgg@...dia.com,
        saeedm@...dia.com, kvm@...r.kernel.org, netdev@...r.kernel.org,
        kuba@...nel.org, kevin.tian@...el.com, leonro@...dia.com,
        maorg@...dia.com, cohuck@...hat.com
Subject: Re: [PATCH V5 vfio 04/10] vfio: Add an IOVA bitmap support

On Thu, 1 Sep 2022 20:39:40 +0100
joao.m.martins@...cle.com wrote:

> On 01/09/2022 19:47, Alex Williamson wrote:
> > On Thu, 1 Sep 2022 12:38:47 +0300
> > Yishai Hadas <yishaih@...dia.com> wrote:
> >> + * An example of the APIs on how to use/iterate over the IOVA bitmap:
> >> + *
> >> + *   bitmap = iova_bitmap_alloc(iova, length, page_size, data);
> >> + *   if (IS_ERR(bitmap))
> >> + *       return PTR_ERR(bitmap);
> >> + *
> >> + *   ret = iova_bitmap_for_each(bitmap, arg, dirty_reporter_fn);
> >> + *
> >> + *   iova_bitmap_free(bitmap);
> >> + *
> >> + * An implementation of the lower end (referred to above as
> >> + * dirty_reporter_fn to exemplify), that is tracking dirty bits would mark
> >> + * an IOVA as dirty as following:
> >> + *     iova_bitmap_set(bitmap, iova, page_size);
> >> + * Or a contiguous range (example two pages):
> >> + *     iova_bitmap_set(bitmap, iova, 2 * page_size);  
> > 
> > This seems like it implies a stronger correlation to the
> > iova_bitmap_alloc() page_size than actually exists.  The implementation
> > of the dirty_reporter_fn() may not know the reporting page_size.  The
> > value here is just a size_t and iova_bitmap handles the rest, right?
> >   
> Correct. 
> 
> The intent was to show an example of what the different usage have
> an effect in the end bitmap data (1 page and then 2 pages). An alternative
> would be:
> 
> 	An implementation of the lower end (referred to above as
> 	dirty_reporter_fn to exemplify), that is tracking dirty bits would mark
> 	an IOVA range spanning @iova_length as dirty, using the configured
> 	@page_size:
> 
>   	  iova_bitmap_set(bitmap, iova, iova_length)
> 
> But with a different length variable (i.e. iova_length) to avoid being confused with
> the length in iova_bitmap_alloc right before this paragraph. But the example in the
> patch looks a bit more clear on the outcomes to me personally.

How about:

  Each iteration of the dirty_reporter_fn is called with a unique @iova
  and @length argument, indicating the current range available through
  the iova_bitmap.  The dirty_reporter_fn uses iova_bitmap_set() to
  mark dirty areas within that provided range

?

...
> >> +/**
> >> + * iova_bitmap_for_each() - Iterates over the bitmap
> >> + * @bitmap: IOVA bitmap to iterate
> >> + * @opaque: Additional argument to pass to the callback
> >> + * @fn: Function that gets called for each IOVA range
> >> + *
> >> + * Helper function to iterate over bitmap data representing a portion of IOVA
> >> + * space. It hides the complexity of iterating bitmaps and translating the
> >> + * mapped bitmap user pages into IOVA ranges to process.
> >> + *
> >> + * Return: 0 on success, and an error on failure either upon
> >> + * iteration or when the callback returns an error.
> >> + */
> >> +int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque,
> >> +			 int (*fn)(struct iova_bitmap *bitmap,
> >> +				   unsigned long iova, size_t length,
> >> +				   void *opaque))  
> > 
> > It might make sense to typedef an iova_bitmap_fn_t in the header to use
> > here.
> >  
> OK, will do. I wasn't sure which style was preferred so went with simplest on
> first take.

It looks like it would be a little cleaner, but yeah, probably largely
style.

> >> +{
> >> +	int ret = 0;
> >> +
> >> +	for (; !iova_bitmap_done(bitmap) && !ret;
> >> +	     ret = iova_bitmap_advance(bitmap)) {
> >> +		ret = fn(bitmap, iova_bitmap_mapped_iova(bitmap),
> >> +			 iova_bitmap_mapped_length(bitmap), opaque);
> >> +		if (ret)
> >> +			break;
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +/**
> >> + * iova_bitmap_set() - Records an IOVA range in bitmap
> >> + * @bitmap: IOVA bitmap
> >> + * @iova: IOVA to start
> >> + * @length: IOVA range length
> >> + *
> >> + * Set the bits corresponding to the range [iova .. iova+length-1] in
> >> + * the user bitmap.
> >> + *
> >> + * Return: The number of bits set.  
> > 
> > Is this relevant to the caller?
> >   
> The thinking that number of bits was a way for caller to validate that
> 'some bits' was set, i.e. sort of error return value. But none of the callers
> use it today, it's true. Suppose I should remove it, following bitmap_set()
> returning void too.

I think 0/-errno are sufficient if we need an error path, otherwise
void is fine.  As above, the reporter fn isn't strongly tied to the
page size of the bitmap, so number of bits just didn't make sense to me.

> >> + */
> >> +unsigned long iova_bitmap_set(struct iova_bitmap *bitmap,
> >> +			      unsigned long iova, size_t length)
> >> +{
> >> +	struct iova_bitmap_map *mapped = &bitmap->mapped;
> >> +	unsigned long nbits = max(1UL, length >> mapped->pgshift), set = nbits;
> >> +	unsigned long offset = (iova - mapped->iova) >> mapped->pgshift;  
> > 
> > There's no sanity testing here that the caller provided an iova within
> > the mapped ranged.  Thanks,
> >   
> 
> Much of the bitmap helpers don't check that the offset is within the range
> of the passed ulong array. So I followed the same thinking and the
> caller is /provided/ with the range that the IOVA bitmap covers. The intention
> was minimizing the number of operations given that this function sits on the
> hot path. I can add this extra check.

Maybe Jason can quote a standard here, audit the callers vs sanitize
the input.  It'd certainly be fair even if the test were a BUG_ON since
it violates the defined calling conventions and we're not taking
arbitrary input, but it could also pretty easily and quietly go into
the weeds if we do nothing.  Thanks,

Alex

Powered by blists - more mailing lists