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

Matthew Rosato posted 3 patches 5 years, 4 months ago
Maintainers: Richard Henderson <rth@twiddle.net>, Matthew Rosato <mjrosato@linux.ibm.com>, Alex Williamson <alex.williamson@redhat.com>, Cornelia Huck <cohuck@redhat.com>, David Hildenbrand <david@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Thomas Huth <thuth@redhat.com>, Halil Pasic <pasic@linux.ibm.com>
There is a newer version of this series
[PATCH v2 1/3] vfio: Find DMA available capability
Posted by Matthew Rosato 5 years, 4 months ago
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) {
+        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;
-- 
1.8.3.1


Re: [PATCH v2 1/3] vfio: Find DMA available capability
Posted by Philippe Mathieu-Daudé 5 years, 4 months ago
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?

> +        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;
> 


Re: [PATCH v2 1/3] vfio: Find DMA available capability
Posted by Cornelia Huck 5 years, 4 months ago
On Tue, 15 Sep 2020 08:14:24 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> 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(+)

(...)

> > +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 agree; if the purpose of this function is to check if limiting is in
place and only optionally return the actual limit, we should return
true for hdr != NULL and avail == NULL.

> 
> > +        return false;
> > +    }
> > +
> > +    cap = (void *) hdr;
> > +    *avail = cap->avail;
> > +    return true;
> > +}
> > +
> >  static int vfio_setup_region_sparse_mmaps(VFIORegion *region,
> >                                            struct vfio_region_info *info)
> >  {

(...)


Re: [PATCH v2 1/3] vfio: Find DMA available capability
Posted by Matthew Rosato 5 years, 4 months ago
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;
>>
> 


Re: [PATCH v2 1/3] vfio: Find DMA available capability
Posted by Cornelia Huck 5 years, 4 months ago
On Mon, 14 Sep 2020 18:29:28 -0400
Matthew Rosato <mjrosato@linux.ibm.com> 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(+)
> 

(...)

> +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);

...don't you need a headers sync first to get the new definitions?

(...)


Re: [PATCH v2 1/3] vfio: Find DMA available capability
Posted by Matthew Rosato 5 years, 4 months ago
On 9/15/20 6:33 AM, Cornelia Huck wrote:
> On Mon, 14 Sep 2020 18:29:28 -0400
> Matthew Rosato <mjrosato@linux.ibm.com> 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(+)
>>
> 
> (...)
> 
>> +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);
> 
> ...don't you need a headers sync first to get the new definitions?
> 

You are right of course, though the associated header change is not yet 
merged in the kernel so it's a bit flaky.  But bottom line:  yes, we 
need a header sync first, I'll include one in v3.

> (...)
> 


Re: [PATCH v2 1/3] vfio: Find DMA available capability
Posted by Cornelia Huck 5 years, 4 months ago
On Tue, 15 Sep 2020 09:57:03 -0400
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> On 9/15/20 6:33 AM, Cornelia Huck wrote:
> > On Mon, 14 Sep 2020 18:29:28 -0400
> > Matthew Rosato <mjrosato@linux.ibm.com> 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(+)
> >>  
> > 
> > (...)
> >   
> >> +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);  
> > 
> > ...don't you need a headers sync first to get the new definitions?
> >   
> 
> You are right of course, though the associated header change is not yet 
> merged in the kernel so it's a bit flaky.  But bottom line:  yes, we 
> need a header sync first, I'll include one in v3.

Just include a placeholder patch :) It's easy to replace them with a
real update prior to merging.