On Mon, May 16, 2016 at 02:20:02PM -0600, Alex Williamson wrote:
> On Mon, 16 May 2016 14:52:41 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > On 05/14/2016 08:26 AM, Alex Williamson wrote:
> > > On Wed,  4 May 2016 16:52:30 +1000
> > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > >  
> > >> New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management.
> > >> This adds ability to VFIO common code to dynamically allocate/remove
> > >> DMA windows in the host kernel when new VFIO container is added/removed.
> > >>
> > >> This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_add
> > >> and adds just created IOMMU into the host IOMMU list; the opposite
> > >> action is taken in vfio_listener_region_del.
> > >>
> > >> When creating a new window, this uses heuristic to decide on the TCE table
> > >> levels number.
> > >>
> > >> This should cause no guest visible change in behavior.
> > >>
> > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > >> ---
> > >> Changes:
> > >> v16:
> > >> * used memory_region_iommu_get_page_sizes() in vfio_listener_region_add()
> > >> * enforced no intersections between windows
> > >>
> > >> v14:
> > >> * new to the series
> > >> ---
> > >>  hw/vfio/common.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> > >>  trace-events     |   2 +
> > >>  2 files changed, 125 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > >> index 03daf88..bd2dee8 100644
> > >> --- a/hw/vfio/common.c
> > >> +++ b/hw/vfio/common.c
> > >> @@ -240,6 +240,18 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova,
> > >>      return -errno;
> > >>  }
> > >>
> > >> +static bool range_contains(hwaddr start, hwaddr end, hwaddr addr)
> > >> +{
> > >> +    return start <= addr && addr <= end;
> > >> +}  
> > >
> > > a) If you want a "range_foo" function then put it in range.h
> > > b) I suspect there are already range.h functions that can do this.
> > >  
> > >> +
> > >> +static bool vfio_host_win_intersects(VFIOHostDMAWindow *hostwin,
> > >> +                                     hwaddr min_iova, hwaddr max_iova)
> > >> +{
> > >> +    return range_contains(hostwin->min_iova, hostwin->min_iova, min_iova) ||
> > >> +        range_contains(min_iova, max_iova, hostwin->min_iova);
> > >> +}  
> > >
> > > How is this different than ranges_overlap()?  
> > >> +
> > >>  static VFIOHostDMAWindow *vfio_host_win_lookup(VFIOContainer *container,
> > >>                                                 hwaddr min_iova, hwaddr max_iova)
> > >>  {
> > >> @@ -279,6 +291,14 @@ static int vfio_host_win_add(VFIOContainer *container,
> > >>      return 0;
> > >>  }
> > >>
> > >> +static void vfio_host_win_del(VFIOContainer *container, hwaddr min_iova)
> > >> +{
> > >> +    VFIOHostDMAWindow *hostwin = vfio_host_win_lookup(container, min_iova, 1);
> > >> +
> > >> +    g_assert(hostwin);  
> > >
> > > Handle the error please.  
> > 
> > Will this be enough?
> > 
> >      if (!hostwin) {
> >          error_report("%s: Cannot delete missing window at %"HWADDR_PRIx,
> >                       __func__, min_iova);
> >          return;
> >      }
> 
> Better.  I was really thinking to return error to the caller, but if
> the caller has no return path, perhaps this is as good as we can do.
> Expect that I will push back on any assert() calls added to vfio.

In this case I think returning an error makes more sense.  The caller
understands the context and so can make reasonable error handling
decisions.   .. which will probably just be a fatal error, but it
still makes more logical sense there.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson