Re: [PATCH v2 1/3] vfio: Find DMA available capability

[Matthew Rosato]

On 9/15/20 2:14 AM, Philippe Mathieu-Daudé wrote:
> Hi Matthew,
>
> On 9/15/20 12:29 AM, Matthew Rosato wrote:
> > The underlying host may be limiting the number of outstanding DMA
> > requests for type 1 IOMMU.  Add helper functions to check for the
> > DMA available capability and retrieve the current number of DMA
> > mappings allowed.
> >
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > ---
> >   hw/vfio/common.c              | 37 +++++++++++++++++++++++++++++++++++++
> >   include/hw/vfio/vfio-common.h |  2 ++
> >   2 files changed, 39 insertions(+)
> >
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 3335714..7f4a338 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -844,6 +844,43 @@ vfio_get_region_info_cap(struct vfio_region_info *info, 
> > uint16_t id)
> >       return NULL;
> >   }
> >   
> > +static struct vfio_info_cap_header *
> > +vfio_get_iommu_type1_info_cap(struct vfio_iommu_type1_info *info, uint16_t id)
> > +{
> > +    struct vfio_info_cap_header *hdr;
> > +    void *ptr = info;
> > +
> > +    if (!(info->flags & VFIO_IOMMU_INFO_CAPS)) {
> > +        return NULL;
> > +    }
> > +
> > +    for (hdr = ptr + info->cap_offset; hdr != ptr; hdr = ptr + hdr->next) {
> > +        if (hdr->id == id) {
> > +            return hdr;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +bool vfio_get_info_dma_avail(struct vfio_iommu_type1_info *info,
> > +                             unsigned int *avail)
> > +{
> > +    struct vfio_info_cap_header *hdr;
> > +    struct vfio_iommu_type1_info_dma_avail *cap;
> > +
> > +    /* If the capability cannot be found, assume no DMA limiting */
> > +    hdr = vfio_get_iommu_type1_info_cap(info,
> > +                                        VFIO_IOMMU_TYPE1_INFO_DMA_AVAIL);
> > +    if (hdr == NULL || avail == NULL) {
>
> If you expect the caller to use avail=NULL, then why
> return false when there is available information?

I was not expecting the caller to use avail=NULL as there would be 
nowhere to stash the dma_avail count.  But you're right, we can at least 
still know that the capability is enabled/disabled when avail=NULL.

I can change this by returning true/false solely based upon the 
existence of the capability (whether or not hdr==NULL) while only 
updating the caller's *avail value when avail!=NULL.

If that's no good, then the alternative would be an assert()

> > +        return false;
> > +    }
> > +
> > +    cap = (void *) hdr;
> > +    *avail = cap->avail;
> > +    return true;
> > +}
> > +
> >   static int vfio_setup_region_sparse_mmaps(VFIORegion *region,
> >                                             struct vfio_region_info *info)
> >   {
> > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> > index c78f3ff..661a380 100644
> > --- a/include/hw/vfio/vfio-common.h
> > +++ b/include/hw/vfio/vfio-common.h
> > @@ -178,6 +178,8 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, 
> > Error **errp);
> >   void vfio_put_group(VFIOGroup *group);
> >   int vfio_get_device(VFIOGroup *group, const char *name,
> >                       VFIODevice *vbasedev, Error **errp);
> > +bool vfio_get_info_dma_avail(struct vfio_iommu_type1_info *info,
> > +                             unsigned int *avail);
> >   
> >   extern const MemoryRegionOps vfio_region_ops;
> >   typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;