subject:"RE\: \[RFC PATCH 1\/6\] vfio\: Add interface for add\/del reserved iova region"

RE: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

2015-10-06 Thread Bhushan Bharat



> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, October 06, 2015 4:15 AM
> To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
> marc.zyng...@arm.com; will.dea...@arm.com
> Subject: Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova
> region
> 
> On Mon, 2015-10-05 at 04:55 +, Bhushan Bharat wrote:
> > Hi Alex,
> >
> > > -Original Message-
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Saturday, October 03, 2015 4:16 AM
> > > To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> > > Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> > > christoffer.d...@linaro.org; eric.au...@linaro.org;
> > > pranavku...@linaro.org; marc.zyng...@arm.com; will.dea...@arm.com
> > > Subject: Re: [RFC PATCH 1/6] vfio: Add interface for add/del
> > > reserved iova region
> > >
> > > On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > > > This Patch adds the VFIO APIs to add and remove reserved iova regions.
> > > > The reserved iova region can be used for mapping some specific
> > > > physical address in iommu.
> > > >
> > > > Currently we are planning to use this interface for adding iova
> > > > regions for creating iommu of msi-pages. But the API are designed
> > > > for future extension where some other physical address can be
> mapped.
> > > >
> > > > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > > > ---
> > > >  drivers/vfio/vfio_iommu_type1.c | 201
> > > +++-
> > > >  include/uapi/linux/vfio.h   |  43 +
> > > >  2 files changed, 243 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > > > b/drivers/vfio/vfio_iommu_type1.c index 57d8c37..fa5d3e4 100644
> > > > --- a/drivers/vfio/vfio_iommu_type1.c
> > > > +++ b/drivers/vfio/vfio_iommu_type1.c
> > > > @@ -59,6 +59,7 @@ struct vfio_iommu {
> > > > struct rb_root  dma_list;
> > > > boolv2;
> > > > boolnesting;
> > > > +   struct list_headreserved_iova_list;
> > >
> > > This alignment leads to poor packing in the structure, put it above the
> bools.
> >
> > ok
> >
> > >
> > > >  };
> > > >
> > > >  struct vfio_domain {
> > > > @@ -77,6 +78,15 @@ struct vfio_dma {
> > > > int prot;   /* IOMMU_READ/WRITE */
> > > >  };
> > > >
> > > > +struct vfio_resvd_region {
> > > > +   dma_addr_t  iova;
> > > > +   size_t  size;
> > > > +   int prot;   /* IOMMU_READ/WRITE */
> > > > +   int refcount;   /* ref count of 
> > > > mappings */
> > > > +   uint64_tmap_paddr;  /* Mapped Physical 
> > > > Address
> > > */
> > >
> > > phys_addr_t
> >
> > Ok,
> >
> > >
> > > > +   struct list_head next;
> > > > +};
> > > > +
> > > >  struct vfio_group {
> > > > struct iommu_group  *iommu_group;
> > > > struct list_headnext;
> > > > @@ -106,6 +116,38 @@ static struct vfio_dma *vfio_find_dma(struct
> > > vfio_iommu *iommu,
> > > > return NULL;
> > > >  }
> > > >
> > > > +/* This function must be called with iommu->lock held */ static
> > > > +bool vfio_overlap_with_resvd_region(struct vfio_iommu *iommu,
> > > > +  dma_addr_t start, size_t 
> > > > size) {
> > > > +   struct vfio_resvd_region *region;
> > > > +
> > > > +   list_for_each_entry(region, >reserved_iova_list, next) {
> > > > +   if (region->iova < start)
> > > > +   return (start - region->iova < region->size);
> > > > +   else if (start < region->iova)
> > > > +   return (region->iova

Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

2015-10-06 Thread Alex Williamson

On Tue, 2015-10-06 at 09:39 +, Bhushan Bharat wrote:
> 
> 
> > -Original Message-
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Tuesday, October 06, 2015 4:15 AM
> > To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> > Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> > christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
> > marc.zyng...@arm.com; will.dea...@arm.com
> > Subject: Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova
> > region
> > 
> > On Mon, 2015-10-05 at 04:55 +, Bhushan Bharat wrote:
> > > Hi Alex,
> > >
> > > > -Original Message-
> > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > Sent: Saturday, October 03, 2015 4:16 AM
> > > > To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> > > > Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> > > > christoffer.d...@linaro.org; eric.au...@linaro.org;
> > > > pranavku...@linaro.org; marc.zyng...@arm.com; will.dea...@arm.com
> > > > Subject: Re: [RFC PATCH 1/6] vfio: Add interface for add/del
> > > > reserved iova region
> > > >
> > > > On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > > > > This Patch adds the VFIO APIs to add and remove reserved iova regions.
> > > > > The reserved iova region can be used for mapping some specific
> > > > > physical address in iommu.
> > > > >
> > > > > Currently we are planning to use this interface for adding iova
> > > > > regions for creating iommu of msi-pages. But the API are designed
> > > > > for future extension where some other physical address can be
> > mapped.
> > > > >
> > > > > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > > > > ---
> > > > >  drivers/vfio/vfio_iommu_type1.c | 201
> > > > +++-
> > > > >  include/uapi/linux/vfio.h   |  43 +
> > > > >  2 files changed, 243 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > > > > b/drivers/vfio/vfio_iommu_type1.c index 57d8c37..fa5d3e4 100644
> > > > > --- a/drivers/vfio/vfio_iommu_type1.c
> > > > > +++ b/drivers/vfio/vfio_iommu_type1.c
> > > > > @@ -59,6 +59,7 @@ struct vfio_iommu {
> > > > >   struct rb_root  dma_list;
> > > > >   boolv2;
> > > > >   boolnesting;
> > > > > + struct list_headreserved_iova_list;
> > > >
> > > > This alignment leads to poor packing in the structure, put it above the
> > bools.
> > >
> > > ok
> > >
> > > >
> > > > >  };
> > > > >
> > > > >  struct vfio_domain {
> > > > > @@ -77,6 +78,15 @@ struct vfio_dma {
> > > > >   int prot;   /* IOMMU_READ/WRITE */
> > > > >  };
> > > > >
> > > > > +struct vfio_resvd_region {
> > > > > + dma_addr_t  iova;
> > > > > + size_t  size;
> > > > > + int prot;   /* IOMMU_READ/WRITE */
> > > > > + int refcount;   /* ref count of 
> > > > > mappings */
> > > > > + uint64_tmap_paddr;  /* Mapped Physical 
> > > > > Address
> > > > */
> > > >
> > > > phys_addr_t
> > >
> > > Ok,
> > >
> > > >
> > > > > + struct list_head next;
> > > > > +};
> > > > > +
> > > > >  struct vfio_group {
> > > > >   struct iommu_group  *iommu_group;
> > > > >   struct list_headnext;
> > > > > @@ -106,6 +116,38 @@ static struct vfio_dma *vfio_find_dma(struct
> > > > vfio_iommu *iommu,
> > > > >   return NULL;
> > > > >  }
> > > > >
> > > > > +/* This function must be called with iommu->lock held */ static
> > > > > +bool vfio_overlap_with_resvd_region(struct vfio_iommu *iommu,
> > > > > +dma_addr_t start, size

Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

2015-10-05 Thread Alex Williamson

On Mon, 2015-10-05 at 04:55 +, Bhushan Bharat wrote:
> Hi Alex,
> 
> > -Original Message-
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Saturday, October 03, 2015 4:16 AM
> > To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> > Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> > christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
> > marc.zyng...@arm.com; will.dea...@arm.com
> > Subject: Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova
> > region
> > 
> > On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > > This Patch adds the VFIO APIs to add and remove reserved iova regions.
> > > The reserved iova region can be used for mapping some specific
> > > physical address in iommu.
> > >
> > > Currently we are planning to use this interface for adding iova
> > > regions for creating iommu of msi-pages. But the API are designed for
> > > future extension where some other physical address can be mapped.
> > >
> > > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > > ---
> > >  drivers/vfio/vfio_iommu_type1.c | 201
> > +++-
> > >  include/uapi/linux/vfio.h   |  43 +
> > >  2 files changed, 243 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > > b/drivers/vfio/vfio_iommu_type1.c index 57d8c37..fa5d3e4 100644
> > > --- a/drivers/vfio/vfio_iommu_type1.c
> > > +++ b/drivers/vfio/vfio_iommu_type1.c
> > > @@ -59,6 +59,7 @@ struct vfio_iommu {
> > >   struct rb_root  dma_list;
> > >   boolv2;
> > >   boolnesting;
> > > + struct list_headreserved_iova_list;
> > 
> > This alignment leads to poor packing in the structure, put it above the 
> > bools.
> 
> ok
> 
> > 
> > >  };
> > >
> > >  struct vfio_domain {
> > > @@ -77,6 +78,15 @@ struct vfio_dma {
> > >   int prot;   /* IOMMU_READ/WRITE */
> > >  };
> > >
> > > +struct vfio_resvd_region {
> > > + dma_addr_t  iova;
> > > + size_t  size;
> > > + int prot;   /* IOMMU_READ/WRITE */
> > > + int refcount;   /* ref count of mappings */
> > > + uint64_tmap_paddr;  /* Mapped Physical Address
> > */
> > 
> > phys_addr_t
> 
> Ok,
> 
> > 
> > > + struct list_head next;
> > > +};
> > > +
> > >  struct vfio_group {
> > >   struct iommu_group  *iommu_group;
> > >   struct list_headnext;
> > > @@ -106,6 +116,38 @@ static struct vfio_dma *vfio_find_dma(struct
> > vfio_iommu *iommu,
> > >   return NULL;
> > >  }
> > >
> > > +/* This function must be called with iommu->lock held */ static bool
> > > +vfio_overlap_with_resvd_region(struct vfio_iommu *iommu,
> > > +dma_addr_t start, size_t size) {
> > > + struct vfio_resvd_region *region;
> > > +
> > > + list_for_each_entry(region, >reserved_iova_list, next) {
> > > + if (region->iova < start)
> > > + return (start - region->iova < region->size);
> > > + else if (start < region->iova)
> > > + return (region->iova - start < size);
> > 
> > <= on both of the return lines?
> 
> I think is should be "<" and not "=<", no ?

Yep, looks like you're right.  Maybe there's a more straightforward way
to do this.

> > 
> > > +
> > > + return (region->size > 0 && size > 0);
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> > > +/* This function must be called with iommu->lock held */ static
> > > +struct vfio_resvd_region *vfio_find_resvd_region(struct vfio_iommu
> > *iommu,
> > > +  dma_addr_t start, size_t
> > size) {
> > > + struct vfio_resvd_region *region;
> > > +
> > > + list_for_each_entry(region, >reserved_iova_list, next)
> > > + if (region->iova == start && region->size == size)
> > > + return region;
> > > +
> > > + return NULL;
> > > +}
> >

RE: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

2015-10-04 Thread Bhushan Bharat

Hi Alex,

> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Saturday, October 03, 2015 4:16 AM
> To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
> marc.zyng...@arm.com; will.dea...@arm.com
> Subject: Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova
> region
> 
> On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > This Patch adds the VFIO APIs to add and remove reserved iova regions.
> > The reserved iova region can be used for mapping some specific
> > physical address in iommu.
> >
> > Currently we are planning to use this interface for adding iova
> > regions for creating iommu of msi-pages. But the API are designed for
> > future extension where some other physical address can be mapped.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > ---
> >  drivers/vfio/vfio_iommu_type1.c | 201
> +++-
> >  include/uapi/linux/vfio.h   |  43 +
> >  2 files changed, 243 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index 57d8c37..fa5d3e4 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -59,6 +59,7 @@ struct vfio_iommu {
> > struct rb_root  dma_list;
> > boolv2;
> > boolnesting;
> > +   struct list_headreserved_iova_list;
> 
> This alignment leads to poor packing in the structure, put it above the bools.

ok

> 
> >  };
> >
> >  struct vfio_domain {
> > @@ -77,6 +78,15 @@ struct vfio_dma {
> > int prot;   /* IOMMU_READ/WRITE */
> >  };
> >
> > +struct vfio_resvd_region {
> > +   dma_addr_t  iova;
> > +   size_t  size;
> > +   int prot;   /* IOMMU_READ/WRITE */
> > +   int refcount;   /* ref count of mappings */
> > +   uint64_tmap_paddr;  /* Mapped Physical Address
> */
> 
> phys_addr_t

Ok,

> 
> > +   struct list_head next;
> > +};
> > +
> >  struct vfio_group {
> > struct iommu_group  *iommu_group;
> > struct list_headnext;
> > @@ -106,6 +116,38 @@ static struct vfio_dma *vfio_find_dma(struct
> vfio_iommu *iommu,
> > return NULL;
> >  }
> >
> > +/* This function must be called with iommu->lock held */ static bool
> > +vfio_overlap_with_resvd_region(struct vfio_iommu *iommu,
> > +  dma_addr_t start, size_t size) {
> > +   struct vfio_resvd_region *region;
> > +
> > +   list_for_each_entry(region, >reserved_iova_list, next) {
> > +   if (region->iova < start)
> > +   return (start - region->iova < region->size);
> > +   else if (start < region->iova)
> > +   return (region->iova - start < size);
> 
> <= on both of the return lines?

I think is should be "<" and not "=<", no ?

> 
> > +
> > +   return (region->size > 0 && size > 0);
> > +   }
> > +
> > +   return false;
> > +}
> > +
> > +/* This function must be called with iommu->lock held */ static
> > +struct vfio_resvd_region *vfio_find_resvd_region(struct vfio_iommu
> *iommu,
> > +dma_addr_t start, size_t
> size) {
> > +   struct vfio_resvd_region *region;
> > +
> > +   list_for_each_entry(region, >reserved_iova_list, next)
> > +   if (region->iova == start && region->size == size)
> > +   return region;
> > +
> > +   return NULL;
> > +}
> > +
> >  static void vfio_link_dma(struct vfio_iommu *iommu, struct vfio_dma
> > *new)  {
> > struct rb_node **link = >dma_list.rb_node, *parent =
> NULL; @@
> > -580,7 +622,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> >
> > mutex_lock(>lock);
> >
> > -   if (vfio_find_dma(iommu, iova, size)) {
> > +   if (vfio_find_dma(iommu, iova, size) ||
> > +   vfio_overlap_with_resvd_region(iommu, iova, size)) {
> > mutex_unlock(>lock);
> > return -EEXIST;
> > }
> > @@ -626,6 +66

Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

2015-10-02 Thread Alex Williamson

On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> This Patch adds the VFIO APIs to add and remove reserved iova
> regions. The reserved iova region can be used for mapping some
> specific physical address in iommu.
> 
> Currently we are planning to use this interface for adding iova
> regions for creating iommu of msi-pages. But the API are designed
> for future extension where some other physical address can be mapped.
> 
> Signed-off-by: Bharat Bhushan 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 201 
> +++-
>  include/uapi/linux/vfio.h   |  43 +
>  2 files changed, 243 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 57d8c37..fa5d3e4 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -59,6 +59,7 @@ struct vfio_iommu {
>   struct rb_root  dma_list;
>   boolv2;
>   boolnesting;
> + struct list_headreserved_iova_list;

This alignment leads to poor packing in the structure, put it above the
bools.

>  };
>  
>  struct vfio_domain {
> @@ -77,6 +78,15 @@ struct vfio_dma {
>   int prot;   /* IOMMU_READ/WRITE */
>  };
>  
> +struct vfio_resvd_region {
> + dma_addr_t  iova;
> + size_t  size;
> + int prot;   /* IOMMU_READ/WRITE */
> + int refcount;   /* ref count of mappings */
> + uint64_tmap_paddr;  /* Mapped Physical Address */

phys_addr_t

> + struct list_head next;
> +};
> +
>  struct vfio_group {
>   struct iommu_group  *iommu_group;
>   struct list_headnext;
> @@ -106,6 +116,38 @@ static struct vfio_dma *vfio_find_dma(struct vfio_iommu 
> *iommu,
>   return NULL;
>  }
>  
> +/* This function must be called with iommu->lock held */
> +static bool vfio_overlap_with_resvd_region(struct vfio_iommu *iommu,
> +dma_addr_t start, size_t size)
> +{
> + struct vfio_resvd_region *region;
> +
> + list_for_each_entry(region, >reserved_iova_list, next) {
> + if (region->iova < start)
> + return (start - region->iova < region->size);
> + else if (start < region->iova)
> + return (region->iova - start < size);

<= on both of the return lines?

> +
> + return (region->size > 0 && size > 0);
> + }
> +
> + return false;
> +}
> +
> +/* This function must be called with iommu->lock held */
> +static
> +struct vfio_resvd_region *vfio_find_resvd_region(struct vfio_iommu *iommu,
> +  dma_addr_t start, size_t size)
> +{
> + struct vfio_resvd_region *region;
> +
> + list_for_each_entry(region, >reserved_iova_list, next)
> + if (region->iova == start && region->size == size)
> + return region;
> +
> + return NULL;
> +}
> +
>  static void vfio_link_dma(struct vfio_iommu *iommu, struct vfio_dma *new)
>  {
>   struct rb_node **link = >dma_list.rb_node, *parent = NULL;
> @@ -580,7 +622,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>  
>   mutex_lock(>lock);
>  
> - if (vfio_find_dma(iommu, iova, size)) {
> + if (vfio_find_dma(iommu, iova, size) ||
> + vfio_overlap_with_resvd_region(iommu, iova, size)) {
>   mutex_unlock(>lock);
>   return -EEXIST;
>   }
> @@ -626,6 +669,127 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>   return ret;
>  }
>  
> +/* This function must be called with iommu->lock held */
> +static
> +int vfio_iommu_resvd_region_del(struct vfio_iommu *iommu,
> + dma_addr_t iova, size_t size, int prot)
> +{
> + struct vfio_resvd_region *res_region;

Have some consistency in naming, just use "region".
> +
> + res_region = vfio_find_resvd_region(iommu, iova, size);
> + /* Region should not be mapped in iommu */
> + if (res_region == NULL || res_region->map_paddr)
> + return -EINVAL;

Are these two separate errors?  !region is -EINVAL, but being mapped is
-EBUSY.

> +
> + list_del(_region->next);
> + kfree(res_region);
> + return 0;
> +}
> +
> +/* This function must be called with iommu->lock held */
> +static int vfio_iommu_resvd_region_add(struct vfio_iommu *iommu,
> +dma_addr_t iova, size_t size, int prot)
> +{
> + struct vfio_resvd_region *res_region;
> +
> + /* Check overlap with with dma maping and reserved regions */
> + if (vfio_find_dma(iommu, iova, size) ||
> + vfio_find_resvd_region(iommu, iova, size))
> + return -EEXIST;
> +
> + res_region = kzalloc(sizeof(*res_region), GFP_KERNEL);
> + if (res_region == NULL)
> + return -ENOMEM;
> +
> +

RE: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

RE: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

Re: [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region

5 matches

Site Navigation

Mail list logo

Footer information