Re: [Qemu-devel] [RFC PATCH] vfio: VFIO Driver core framework

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2011-11-08 at 20:17 -0800, Aaron Fabbri wrote:
> I'm going to send out chunks of comments as I go over this stuff.  Below
> I've covered the documentation file and vfio_iommu.c.  More comments coming
> soon...
> 
> On 11/3/11 1:12 PM, "Alex Williamson" <alex.williamson@redhat.com> wrote:
> 
> > VFIO provides a secure, IOMMU based interface for user space
> > drivers, including device assignment to virtual machines.
> > This provides the base management of IOMMU groups, devices,
> > and IOMMU objects.  See Documentation/vfio.txt included in
> > this patch for user and kernel API description.
> > 
> > Note, this implements the new API discussed at KVM Forum
> > 2011, as represented by the drvier version 0.2.  It's hoped
> > that this provides a modular enough interface to support PCI
> > and non-PCI userspace drivers across various architectures
> > and IOMMU implementations.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> <snip>
> > +
> > +Groups, Devices, IOMMUs, oh my
> > +-----------------------------------------------------------------------------
> > --
> > +
> > +A fundamental component of VFIO is the notion of IOMMU groups.  IOMMUs
> > +can't always distinguish transactions from each individual device in
> > +the system.  Sometimes this is because of the IOMMU design, such as with
> > +PEs, other times it's caused by the I/O topology, for instance a
> 
> Can you define this acronym the first time you use it, i.e.
> 
> + PEs (partitionable endpoints), ...

It was actually up in the <snip>:

... POWER systems with Partitionable Endpoints (PEs) ...

I tried to make sure I defined them, but let me know if anything else is
missing/non-obvious.

> > +PCIe-to-PCI bridge masking all devices behind it.  We call the sets of
> > +devices created by these restictions IOMMU groups (or just "groups" for
> 
> restrictions

Ugh, lost w/o a spell checker.  Fixed all these.

> > diff --git a/drivers/vfio/vfio_iommu.c b/drivers/vfio/vfio_iommu.c
> > new file mode 100644
> > index 0000000..029dae3
> > --- /dev/null
> > +++ b/drivers/vfio/vfio_iommu.c
> <snip>
> > +static struct dma_map_page *vfio_find_dma(struct vfio_iommu *iommu,
> > +                      dma_addr_t start, size_t size)
> > +{
> > +    struct list_head *pos;
> > +    struct dma_map_page *mlp;
> > +
> > +    list_for_each(pos, &iommu->dm_list) {
> > +        mlp = list_entry(pos, struct dma_map_page, list);
> > +        if (ranges_overlap(mlp->daddr, NPAGE_TO_SIZE(mlp->npage),
> > +                   start, size))
> > +            return mlp;
> > +    }
> > +    return NULL;
> > +}
> > +
> 
> This function below should be static.

Fixed

> > +int vfio_remove_dma_overlap(struct vfio_iommu *iommu, dma_addr_t start,
> > +                size_t size, struct dma_map_page *mlp)
> > +{
> > +    struct dma_map_page *split;
> > +    int npage_lo, npage_hi;
> > +
> > +    /* Existing dma region is completely covered, unmap all */
> > +    if (start <= mlp->daddr &&
> > +        start + size >= mlp->daddr + NPAGE_TO_SIZE(mlp->npage)) {
> > +        vfio_dma_unmap(iommu, mlp->daddr, mlp->npage, mlp->rdwr);
> > +        list_del(&mlp->list);
> > +        npage_lo = mlp->npage;
> > +        kfree(mlp);
> > +        return npage_lo;
> > +    }
> > +
> > +    /* Overlap low address of existing range */
> > +    if (start <= mlp->daddr) {
> > +        size_t overlap;
> > +
> > +        overlap = start + size - mlp->daddr;
> > +        npage_lo = overlap >> PAGE_SHIFT;
> > +        npage_hi = mlp->npage - npage_lo;
> 
> npage_hi not used.. Delete this line ^

Yep, and npage_lo in the next block.  I was setting them just for
symmetry, but they can be removed now.

> > +
> > +        vfio_dma_unmap(iommu, mlp->daddr, npage_lo, mlp->rdwr);
> > +        mlp->daddr += overlap;
> > +        mlp->vaddr += overlap;
> > +        mlp->npage -= npage_lo;
> > +        return npage_lo;
> > +    }
> > +
> > +    /* Overlap high address of existing range */
> > +    if (start + size >= mlp->daddr + NPAGE_TO_SIZE(mlp->npage)) {
> > +        size_t overlap;
> > +
> > +        overlap = mlp->daddr + NPAGE_TO_SIZE(mlp->npage) - start;
> > +        npage_hi = overlap >> PAGE_SHIFT;
> > +        npage_lo = mlp->npage - npage_hi;
> > +
> > +        vfio_dma_unmap(iommu, start, npage_hi, mlp->rdwr);
> > +        mlp->npage -= npage_hi;
> > +        return npage_hi;
> > +    }
> > +
> > +    /* Split existing */
> > +    npage_lo = (start - mlp->daddr) >> PAGE_SHIFT;
> > +    npage_hi = mlp->npage - (size >> PAGE_SHIFT) - npage_lo;
> > +
> > +    split = kzalloc(sizeof *split, GFP_KERNEL);
> > +    if (!split)
> > +        return -ENOMEM;
> > +
> > +    vfio_dma_unmap(iommu, start, size >> PAGE_SHIFT, mlp->rdwr);
> > +
> > +    mlp->npage = npage_lo;
> > +
> > +    split->npage = npage_hi;
> > +    split->daddr = start + size;
> > +    split->vaddr = mlp->vaddr + NPAGE_TO_SIZE(npage_lo) + size;
> > +    split->rdwr = mlp->rdwr;
> > +    list_add(&split->list, &iommu->dm_list);
> > +    return size >> PAGE_SHIFT;
> > +}
> > +
> 
> Function should be static.

Fixed

> > +int vfio_dma_unmap_dm(struct vfio_iommu *iommu, struct vfio_dma_map *dmp)
> > +{
> > +    int ret = 0;
> > +    size_t npage = dmp->size >> PAGE_SHIFT;
> > +    struct list_head *pos, *n;
> > +
> > +    if (dmp->dmaaddr & ~PAGE_MASK)
> > +        return -EINVAL;
> > +    if (dmp->size & ~PAGE_MASK)
> > +        return -EINVAL;
> > +
> > +    mutex_lock(&iommu->dgate);
> > +
> > +    list_for_each_safe(pos, n, &iommu->dm_list) {
> > +        struct dma_map_page *mlp;
> > +
> > +        mlp = list_entry(pos, struct dma_map_page, list);
> > +        if (ranges_overlap(mlp->daddr, NPAGE_TO_SIZE(mlp->npage),
> > +                   dmp->dmaaddr, dmp->size)) {
> > +            ret = vfio_remove_dma_overlap(iommu, dmp->dmaaddr,
> > +                              dmp->size, mlp);
> > +            if (ret > 0)
> > +                npage -= NPAGE_TO_SIZE(ret);
> 
> Why NPAGE_TO_SIZE here?

Looks like a bug, I'll change and test.

> > +            if (ret < 0 || npage == 0)
> > +                break;
> > +        }
> > +    }
> > +    mutex_unlock(&iommu->dgate);
> > +    return ret > 0 ? 0 : ret;
> > +}
> > +
> 
> Function should be static.

Fixed.

> > +int vfio_dma_map_dm(struct vfio_iommu *iommu, struct vfio_dma_map *dmp)
> > +{
> > +    int npage;
> > +    struct dma_map_page *mlp, *mmlp = NULL;
> > +    dma_addr_t daddr = dmp->dmaaddr;
> 

Thanks!

Alex