Re: [Qemu-devel] [PATCH qemu v16 16/19] vfio: Add host side DMA window capabilities

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]

On Fri, May 13, 2016 at 04:25:59PM -0600, Alex Williamson wrote:
> On Wed,  4 May 2016 16:52:28 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > There are going to be multiple IOMMUs per a container. This moves
> > the single host IOMMU parameter set to a list of VFIOHostDMAWindow.
> > 
> > This should cause no behavioral change and will be used later by
> > the SPAPR TCE IOMMU v2 which will also add a vfio_host_win_del() helper.
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > Changes:
> > v16:
> > * adjusted commit log with changes from v15
> > 
> > v15:
> > * s/vfio_host_iommu_add/vfio_host_win_add/
> > * s/VFIOHostIOMMU/VFIOHostDMAWindow/
> > ---
> >  hw/vfio/common.c              | 65 +++++++++++++++++++++++++++++++++----------
> >  include/hw/vfio/vfio-common.h |  9 ++++--
> >  2 files changed, 57 insertions(+), 17 deletions(-)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 496eb82..3f2fb23 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -29,6 +29,7 @@
> >  #include "exec/memory.h"
> >  #include "hw/hw.h"
> >  #include "qemu/error-report.h"
> > +#include "qemu/range.h"
> >  #include "sysemu/kvm.h"
> >  #include "trace.h"
> >  
> > @@ -239,6 +240,45 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova,
> >      return -errno;
> >  }
> >  
> > +static VFIOHostDMAWindow *vfio_host_win_lookup(VFIOContainer *container,
> > +                                               hwaddr min_iova, hwaddr max_iova)
> > +{
> > +    VFIOHostDMAWindow *hostwin;
> > +
> > +    QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
> > +        if (hostwin->min_iova <= min_iova && max_iova <= hostwin->max_iova) {
> > +            return hostwin;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +static int vfio_host_win_add(VFIOContainer *container,
> > +                             hwaddr min_iova, hwaddr max_iova,
> > +                             uint64_t iova_pgsizes)
> > +{
> > +    VFIOHostDMAWindow *hostwin;
> > +
> > +    QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
> > +        if (ranges_overlap(min_iova, max_iova - min_iova + 1,
> > +                           hostwin->min_iova,
> > +                           hostwin->max_iova - hostwin->min_iova + 1)) {
> 
> Why does vfio_host_win_lookup() not also use ranges_overlap()?  In
> fact, why don't we call vfio_host_win_lookup here to find the conflict?
> 
> > +            error_report("%s: Overlapped IOMMU are not enabled", __func__);
> > +            return -1;
> 
> Nobody here tests the return value, shouldn't this be fatal?

Hm, yes.  I think hw_error() would be the right choice here.  This
would represent either a qemu programming error, or seriously
unexpected behaviour from the host kernel.
-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]