RE: [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state

[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 3/6] vfio: Extend iommu-info to return MSIs
> automap state
> 
> On Mon, 2015-10-05 at 06:00 +, Bhushan Bharat wrote:
> > > -1138,6 +1156,8 @@
> > > > static long vfio_iommu_type1_ioctl(void *iommu_data,
> > > > }
> > > > } else if (cmd == VFIO_IOMMU_GET_INFO) {
> > > > struct vfio_iommu_type1_info info;
> > > > +   struct iommu_domain_msi_maps msi_maps;
> > > > +   int ret;
> > > >
> > > > minsz = offsetofend(struct vfio_iommu_type1_info,
> > > iova_pgsizes);
> > > >
> > > > @@ -1149,6 +1169,18 @@ static long vfio_iommu_type1_ioctl(void
> > > > *iommu_data,
> > > >
> > > > info.flags = 0;
> > > >
> > > > +   ret = vfio_domains_get_msi_maps(iommu, _maps);
> > > > +   if (ret)
> > > > +   return ret;
> > >
> > > And now ioctl(VFIO_IOMMU_GET_INFO) no longer works for any
> IOMMU
> > > implementing domain_get_attr but not supporting
> > > DOMAIN_ATTR_MSI_MAPPING.
> >
> > With this current patch version this will get the default assumed behavior
> as you commented on previous patch.
> 
> How so?

You are right, the ioctl will return failure. But that should be ok, right?

> 
> +   msi_maps->automap = true;
> +   msi_maps->override_automap = false;
> +
> +   if (domain->ops->domain_get_attr)
> +   ret = domain->ops->domain_get_attr(domain, attr,
> + data);
> 
> If domain_get_attr is implemented, but DOMAIN_ATTR_MSI_MAPPING is
> not, ret should be an error code.

Currently it returns same error code returned by 
domain->ops->domain_get_attr(). 
I do not think we want to complicate that we return an error to user-space that 
msi's probably cannot be used but user-space can continue with Legacy 
interrupt, or you want that?

Thanks
-Bharat

Re: [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state

[Alex Williamson]

On Tue, 2015-10-06 at 08:53 +, 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 3/6] vfio: Extend iommu-info to return MSIs
> > automap state
> > 
> > On Mon, 2015-10-05 at 06:00 +, Bhushan Bharat wrote:
> > > > -1138,6 +1156,8 @@
> > > > > static long vfio_iommu_type1_ioctl(void *iommu_data,
> > > > >   }
> > > > >   } else if (cmd == VFIO_IOMMU_GET_INFO) {
> > > > >   struct vfio_iommu_type1_info info;
> > > > > + struct iommu_domain_msi_maps msi_maps;
> > > > > + int ret;
> > > > >
> > > > >   minsz = offsetofend(struct vfio_iommu_type1_info,
> > > > iova_pgsizes);
> > > > >
> > > > > @@ -1149,6 +1169,18 @@ static long vfio_iommu_type1_ioctl(void
> > > > > *iommu_data,
> > > > >
> > > > >   info.flags = 0;
> > > > >
> > > > > + ret = vfio_domains_get_msi_maps(iommu, _maps);
> > > > > + if (ret)
> > > > > + return ret;
> > > >
> > > > And now ioctl(VFIO_IOMMU_GET_INFO) no longer works for any
> > IOMMU
> > > > implementing domain_get_attr but not supporting
> > > > DOMAIN_ATTR_MSI_MAPPING.
> > >
> > > With this current patch version this will get the default assumed behavior
> > as you commented on previous patch.
> > 
> > How so?
> 
> You are right, the ioctl will return failure. But that should be ok, right?

Not remotely.  ioctl(VFIO_IOMMU_GET_INFO) can't suddenly stop working on
some platforms.

> > 
> > +   msi_maps->automap = true;
> > +   msi_maps->override_automap = false;
> > +
> > +   if (domain->ops->domain_get_attr)
> > +   ret = domain->ops->domain_get_attr(domain, attr,
> > + data);
> > 
> > If domain_get_attr is implemented, but DOMAIN_ATTR_MSI_MAPPING is
> > not, ret should be an error code.
> 
> Currently it returns same error code returned by 
> domain->ops->domain_get_attr(). 
> I do not think we want to complicate that we return an error to user-space 
> that msi's probably cannot be used but user-space can continue with Legacy 
> interrupt, or you want that?

I can't really parse your statement, but ioctl(VFIO_IOMMU_GET_INFO)
works today and it must work with your changes.  Your change should only
affect whether some flags are visible, MSI has worked just fine up to
this point on other platforms.



--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state

[Bhushan Bharat]

> -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 3/6] vfio: Extend iommu-info to return MSIs
> automap state
> 
> On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > This patch allows the user-space to know whether msi-pages are
> > automatically mapped with some magic iova or not.
> >
> > Even if the msi-pages are automatically mapped, still user-space wants
> > to over-ride the automatic iova selection for msi-mapping.
> > For this user-space need to know whether it is allowed to change the
> > automatic mapping or not and this API provides this mechanism.
> > Follow up patches will provide how to over-ride this.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > ---
> >  drivers/vfio/vfio_iommu_type1.c | 32
> 
> >  include/uapi/linux/vfio.h   |  3 +++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index fa5d3e4..3315fb6 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;
> > +   boolallow_msi_reconfig;
> > struct list_headreserved_iova_list;
> >  };
> >
> > @@ -1117,6 +1118,23 @@ static int
> vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> > return ret;
> >  }
> >
> > +static
> > +int vfio_domains_get_msi_maps(struct vfio_iommu *iommu,
> > + struct iommu_domain_msi_maps *msi_maps) {
> > +   struct vfio_domain *d;
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   /* All domains have same msi-automap property, pick first */
> > +   d = list_first_entry(>domain_list, struct vfio_domain, next);
> > +   ret = iommu_domain_get_attr(d->domain,
> DOMAIN_ATTR_MSI_MAPPING,
> > +   msi_maps);
> > +   mutex_unlock(>lock);
> > +
> > +   return ret;
> > +}
> > +
> >  static long vfio_iommu_type1_ioctl(void *iommu_data,
> >unsigned int cmd, unsigned long arg)  { @@
> -1138,6 +1156,8 @@
> > static long vfio_iommu_type1_ioctl(void *iommu_data,
> > }
> > } else if (cmd == VFIO_IOMMU_GET_INFO) {
> > struct vfio_iommu_type1_info info;
> > +   struct iommu_domain_msi_maps msi_maps;
> > +   int ret;
> >
> > minsz = offsetofend(struct vfio_iommu_type1_info,
> iova_pgsizes);
> >
> > @@ -1149,6 +1169,18 @@ static long vfio_iommu_type1_ioctl(void
> > *iommu_data,
> >
> > info.flags = 0;
> >
> > +   ret = vfio_domains_get_msi_maps(iommu, _maps);
> > +   if (ret)
> > +   return ret;
> 
> And now ioctl(VFIO_IOMMU_GET_INFO) no longer works for any IOMMU
> implementing domain_get_attr but not supporting
> DOMAIN_ATTR_MSI_MAPPING.

With this current patch version this will get the default assumed behavior as 
you commented on previous patch. 

> 
> > +
> > +   if (msi_maps.override_automap) {
> > +   info.flags |=
> VFIO_IOMMU_INFO_MSI_ALLOW_RECONFIG;
> > +   iommu->allow_msi_reconfig = true;
> > +   }
> > +
> > +   if (msi_maps.automap)
> > +   info.flags |= VFIO_IOMMU_INFO_MSI_AUTOMAP;
> > +
> > info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
> >
> > return copy_to_user((void __user *)arg, , minsz); diff --
> git
> > a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index
> > 1abd1a9..9998f6e 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -391,6 +391,9 @@ struct vfio_iommu_type1_info {
> > __u32   argsz;
> > __u32   flags;
> >  #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)   /* supported page
> sizes info */
> > +#define VFIO_IOMMU_INFO_MSI_AUTOMAP (1 << 1)   /* MSI pages
> are auto-mapped
> > +  in iommu */
> > +#define VFIO_IOMMU_INFO_MSI_ALLOW_RECONFIG (1 << 2) /* Allows
> > +reconfig automap*/
> > __u64   iova_pgsizes;   /* Bitmap of supported page sizes */
> >  };
> >
> 
> Once again, exposing interfaces to the user before they actually do anything
> is backwards.

Will change the order.

Thanks
-Bharat

Re: [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state

[Alex Williamson]

On Mon, 2015-10-05 at 06:00 +, Bhushan Bharat wrote:
> > -1138,6 +1156,8 @@
> > > static long vfio_iommu_type1_ioctl(void *iommu_data,
> > >   }
> > >   } else if (cmd == VFIO_IOMMU_GET_INFO) {
> > >   struct vfio_iommu_type1_info info;
> > > + struct iommu_domain_msi_maps msi_maps;
> > > + int ret;
> > >
> > >   minsz = offsetofend(struct vfio_iommu_type1_info,
> > iova_pgsizes);
> > >
> > > @@ -1149,6 +1169,18 @@ static long vfio_iommu_type1_ioctl(void
> > > *iommu_data,
> > >
> > >   info.flags = 0;
> > >
> > > + ret = vfio_domains_get_msi_maps(iommu, _maps);
> > > + if (ret)
> > > + return ret;
> > 
> > And now ioctl(VFIO_IOMMU_GET_INFO) no longer works for any IOMMU
> > implementing domain_get_attr but not supporting
> > DOMAIN_ATTR_MSI_MAPPING.
> 
> With this current patch version this will get the default assumed behavior as 
> you commented on previous patch. 

How so?

+   msi_maps->automap = true;
+   msi_maps->override_automap = false;
+
+   if (domain->ops->domain_get_attr)
+   ret = domain->ops->domain_get_attr(domain, attr, data);

If domain_get_attr is implemented, but DOMAIN_ATTR_MSI_MAPPING is not,
ret should be an error code.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state

[Alex Williamson]

On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> This patch allows the user-space to know whether msi-pages
> are automatically mapped with some magic iova or not.
> 
> Even if the msi-pages are automatically mapped, still user-space
> wants to over-ride the automatic iova selection for msi-mapping.
> For this user-space need to know whether it is allowed to change
> the automatic mapping or not and this API provides this mechanism.
> Follow up patches will provide how to over-ride this.
> 
> Signed-off-by: Bharat Bhushan 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 32 
>  include/uapi/linux/vfio.h   |  3 +++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index fa5d3e4..3315fb6 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;
> + boolallow_msi_reconfig;
>   struct list_headreserved_iova_list;
>  };
>  
> @@ -1117,6 +1118,23 @@ static int vfio_domains_have_iommu_cache(struct 
> vfio_iommu *iommu)
>   return ret;
>  }
>  
> +static
> +int vfio_domains_get_msi_maps(struct vfio_iommu *iommu,
> +   struct iommu_domain_msi_maps *msi_maps)
> +{
> + struct vfio_domain *d;
> + int ret;
> +
> + mutex_lock(>lock);
> + /* All domains have same msi-automap property, pick first */
> + d = list_first_entry(>domain_list, struct vfio_domain, next);
> + ret = iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_MAPPING,
> + msi_maps);
> + mutex_unlock(>lock);
> +
> + return ret;
> +}
> +
>  static long vfio_iommu_type1_ioctl(void *iommu_data,
>  unsigned int cmd, unsigned long arg)
>  {
> @@ -1138,6 +1156,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>   }
>   } else if (cmd == VFIO_IOMMU_GET_INFO) {
>   struct vfio_iommu_type1_info info;
> + struct iommu_domain_msi_maps msi_maps;
> + int ret;
>  
>   minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
>  
> @@ -1149,6 +1169,18 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  
>   info.flags = 0;
>  
> + ret = vfio_domains_get_msi_maps(iommu, _maps);
> + if (ret)
> + return ret;

And now ioctl(VFIO_IOMMU_GET_INFO) no longer works for any IOMMU
implementing domain_get_attr but not supporting DOMAIN_ATTR_MSI_MAPPING.

> +
> + if (msi_maps.override_automap) {
> + info.flags |= VFIO_IOMMU_INFO_MSI_ALLOW_RECONFIG;
> + iommu->allow_msi_reconfig = true;
> + }
> +
> + if (msi_maps.automap)
> + info.flags |= VFIO_IOMMU_INFO_MSI_AUTOMAP;
> +
>   info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
>  
>   return copy_to_user((void __user *)arg, , minsz);
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1abd1a9..9998f6e 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -391,6 +391,9 @@ struct vfio_iommu_type1_info {
>   __u32   argsz;
>   __u32   flags;
>  #define VFIO_IOMMU_INFO_PGSIZES (1 << 0) /* supported page sizes info */
> +#define VFIO_IOMMU_INFO_MSI_AUTOMAP (1 << 1) /* MSI pages are auto-mapped
> +in iommu */
> +#define VFIO_IOMMU_INFO_MSI_ALLOW_RECONFIG (1 << 2) /* Allows reconfig 
> automap*/
>   __u64   iova_pgsizes;   /* Bitmap of supported page sizes */
>  };
>  

Once again, exposing interfaces to the user before they actually do
anything is backwards.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html