| Message ID | 1600122570-12941-2-git-send-email-mjrosato@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/pci: Accomodate vfio DMA limiting | expand |
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; >
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) > > { (...)
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? (...)
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; >> >
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. > (...) >
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.
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;
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(+)