[2/7] iommu: add api to get iommu_domain of a device

Message ID	1379575763-2091-3-git-send-email-Bharat.Bhushan@freescale.com
State	Not Applicable
Headers	show Return-Path: <linux-pci-owner@vger.kernel.org> From: Bharat Bhushan <r65777@freescale.com> To: <alex.williamson@redhat.com>, <joro@8bytes.org>, <benh@kernel.crashing.org>, <galak@kernel.crashing.org>, <linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>, <linux-pci@vger.kernel.org>, <agraf@suse.de>, <scottwood@freescale.com>, <iommu@lists.linux-foundation.org> CC: Bharat Bhushan <Bharat.Bhushan@freescale.com>, Bharat Bhushan <bharat.bhushan@freescale.com> Subject: [PATCH 2/7] iommu: add api to get iommu_domain of a device Date: Thu, 19 Sep 2013 12:59:18 +0530 Message-ID: <1379575763-2091-3-git-send-email-Bharat.Bhushan@freescale.com> In-Reply-To: <1379575763-2091-1-git-send-email-Bharat.Bhushan@freescale.com> References: <1379575763-2091-1-git-send-email-Bharat.Bhushan@freescale.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-pci-owner@vger.kernel.org Precedence: bulk

Bharat Bhushan Sept. 19, 2013, 7:29 a.m. UTC

This api return the iommu domain to which the device is attached.
The iommu_domain is required for making API calls related to iommu.
Follow up patches which use this API to know iommu maping.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 drivers/iommu/iommu.c |   10 ++++++++++
 include/linux/iommu.h |    7 +++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

Alex Williamson Sept. 25, 2013, 4:45 p.m. UTC | #1

On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> This api return the iommu domain to which the device is attached.
> The iommu_domain is required for making API calls related to iommu.
> Follow up patches which use this API to know iommu maping.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
>  drivers/iommu/iommu.c |   10 ++++++++++
>  include/linux/iommu.h |    7 +++++++
>  2 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index fbe9ca7..6ac5f50 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(iommu_detach_device);
>  
> +struct iommu_domain *iommu_get_dev_domain(struct device *dev)
> +{
> +	struct iommu_ops *ops = dev->bus->iommu_ops;
> +
> +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))
> +		return NULL;
> +
> +	return ops->get_dev_iommu_domain(dev);
> +}
> +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);

What prevents this from racing iommu_domain_free()?  There's no
references acquired, so there's no reason for the caller to assume the
pointer is valid.

>  /*
>   * IOMMU groups are really the natrual working unit of the IOMMU, but
>   * the IOMMU API works on domains and devices.  Bridge that gap by
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 7ea319e..fa046bd 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -127,6 +127,7 @@ struct iommu_ops {
>  	int (*domain_set_windows)(struct iommu_domain *domain, u32 w_count);
>  	/* Get the numer of window per domain */
>  	u32 (*domain_get_windows)(struct iommu_domain *domain);
> +	struct iommu_domain *(*get_dev_iommu_domain)(struct device *dev);
>  
>  	unsigned long pgsize_bitmap;
>  };
> @@ -190,6 +191,7 @@ extern int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr,
>  				      phys_addr_t offset, u64 size,
>  				      int prot);
>  extern void iommu_domain_window_disable(struct iommu_domain *domain, u32 wnd_nr);
> +extern struct iommu_domain *iommu_get_dev_domain(struct device *dev);
>  /**
>   * report_iommu_fault() - report about an IOMMU fault to the IOMMU framework
>   * @domain: the iommu domain where the fault has happened
> @@ -284,6 +286,11 @@ static inline void iommu_domain_window_disable(struct iommu_domain *domain,
>  {
>  }
>  
> +static inline struct iommu_domain *iommu_get_dev_domain(struct device *dev)
> +{
> +	return NULL;
> +}
> +
>  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
>  {
>  	return 0;



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

Bharat Bhushan Oct. 4, 2013, 9:54 a.m. UTC | #2

> -----Original Message-----

> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-owner@vger.kernel.org]

> On Behalf Of Alex Williamson

> Sent: Wednesday, September 25, 2013 10:16 PM

> To: Bhushan Bharat-R65777

> Cc: joro@8bytes.org; benh@kernel.crashing.org; galak@kernel.crashing.org; linux-

> kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-

> pci@vger.kernel.org; agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-

> foundation.org; Bhushan Bharat-R65777

> Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device

> 

> On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:

> > This api return the iommu domain to which the device is attached.

> > The iommu_domain is required for making API calls related to iommu.

> > Follow up patches which use this API to know iommu maping.

> >

> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

> > ---

> >  drivers/iommu/iommu.c |   10 ++++++++++

> >  include/linux/iommu.h |    7 +++++++

> >  2 files changed, 17 insertions(+), 0 deletions(-)

> >

> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index

> > fbe9ca7..6ac5f50 100644

> > --- a/drivers/iommu/iommu.c

> > +++ b/drivers/iommu/iommu.c

> > @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain

> > *domain, struct device *dev)  }

> > EXPORT_SYMBOL_GPL(iommu_detach_device);

> >

> > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {

> > +	struct iommu_ops *ops = dev->bus->iommu_ops;

> > +

> > +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))

> > +		return NULL;

> > +

> > +	return ops->get_dev_iommu_domain(dev); }

> > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);

> 

> What prevents this from racing iommu_domain_free()?  There's no references

> acquired, so there's no reason for the caller to assume the pointer is valid.


Sorry for late query, somehow this email went into a folder and escaped;

Just to be sure, there is not lock at generic "struct iommu_domain", but IP specific structure (link FSL domain) linked in iommu_domain->priv have a lock, so we need to ensure this race in FSL iommu code (say drivers/iommu/fsl_pamu_domain.c), right?

Thanks
-Bharat

> 

> >  /*

> >   * IOMMU groups are really the natrual working unit of the IOMMU, but

> >   * the IOMMU API works on domains and devices.  Bridge that gap by

> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index

> > 7ea319e..fa046bd 100644

> > --- a/include/linux/iommu.h

> > +++ b/include/linux/iommu.h

> > @@ -127,6 +127,7 @@ struct iommu_ops {

> >  	int (*domain_set_windows)(struct iommu_domain *domain, u32 w_count);

> >  	/* Get the numer of window per domain */

> >  	u32 (*domain_get_windows)(struct iommu_domain *domain);

> > +	struct iommu_domain *(*get_dev_iommu_domain)(struct device *dev);

> >

> >  	unsigned long pgsize_bitmap;

> >  };

> > @@ -190,6 +191,7 @@ extern int iommu_domain_window_enable(struct iommu_domain

> *domain, u32 wnd_nr,

> >  				      phys_addr_t offset, u64 size,

> >  				      int prot);

> >  extern void iommu_domain_window_disable(struct iommu_domain *domain,

> > u32 wnd_nr);

> > +extern struct iommu_domain *iommu_get_dev_domain(struct device *dev);

> >  /**

> >   * report_iommu_fault() - report about an IOMMU fault to the IOMMU framework

> >   * @domain: the iommu domain where the fault has happened @@ -284,6

> > +286,11 @@ static inline void iommu_domain_window_disable(struct

> > iommu_domain *domain,  {  }

> >

> > +static inline struct iommu_domain *iommu_get_dev_domain(struct device

> > +*dev) {

> > +	return NULL;

> > +}

> > +

> >  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain

> > *domain, dma_addr_t iova)  {

> >  	return 0;

> 

> 

> 

> --

> To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body

> of a message to majordomo@vger.kernel.org More majordomo info at

> http://vger.kernel.org/majordomo-info.html

Bharat Bhushan Oct. 4, 2013, 10:42 a.m. UTC | #3

> -----Original Message-----

> From: Bhushan Bharat-R65777

> Sent: Friday, October 04, 2013 3:24 PM

> To: 'Alex Williamson'

> Cc: joro@8bytes.org; benh@kernel.crashing.org; galak@kernel.crashing.org; linux-

> kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-

> pci@vger.kernel.org; agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-

> foundation.org

> Subject: RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device

> 

> 

> 

> > -----Original Message-----

> > From: linux-pci-owner@vger.kernel.org

> > [mailto:linux-pci-owner@vger.kernel.org]

> > On Behalf Of Alex Williamson

> > Sent: Wednesday, September 25, 2013 10:16 PM

> > To: Bhushan Bharat-R65777

> > Cc: joro@8bytes.org; benh@kernel.crashing.org;

> > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;

> > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;

> > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org;

> > Bhushan Bharat-R65777

> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a

> > device

> >

> > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:

> > > This api return the iommu domain to which the device is attached.

> > > The iommu_domain is required for making API calls related to iommu.

> > > Follow up patches which use this API to know iommu maping.

> > >

> > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

> > > ---

> > >  drivers/iommu/iommu.c |   10 ++++++++++

> > >  include/linux/iommu.h |    7 +++++++

> > >  2 files changed, 17 insertions(+), 0 deletions(-)

> > >

> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index

> > > fbe9ca7..6ac5f50 100644

> > > --- a/drivers/iommu/iommu.c

> > > +++ b/drivers/iommu/iommu.c

> > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain

> > > *domain, struct device *dev)  }

> > > EXPORT_SYMBOL_GPL(iommu_detach_device);

> > >

> > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {

> > > +	struct iommu_ops *ops = dev->bus->iommu_ops;

> > > +

> > > +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))

> > > +		return NULL;

> > > +

> > > +	return ops->get_dev_iommu_domain(dev); }

> > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);

> >

> > What prevents this from racing iommu_domain_free()?  There's no

> > references acquired, so there's no reason for the caller to assume the pointer

> is valid.

> 

> Sorry for late query, somehow this email went into a folder and escaped;

> 

> Just to be sure, there is not lock at generic "struct iommu_domain", but IP

> specific structure (link FSL domain) linked in iommu_domain->priv have a lock,

> so we need to ensure this race in FSL iommu code (say

> drivers/iommu/fsl_pamu_domain.c), right?


Further thinking of this, there are more problems here:
 - Like MSI subsystem will call iommu_get_dev_domain(), which will take a lock, find the domain pointer, release the lock, and return the domain
 - Now if domain in freed up
 - While MSI subsystem tries to do work on domain (like get_attribute/set_attribute etc) ???

So can we do like iommu_get_dev_domain() will return domain with the lock held, and iommu_put_dev_domain() will release the lock? And iommu_get_dev_domain() must always be followed by iommu_get_dev_domain()

Thanks
-Bharat

> 

> Thanks

> -Bharat

> 

> >

> > >  /*

> > >   * IOMMU groups are really the natrual working unit of the IOMMU, but

> > >   * the IOMMU API works on domains and devices.  Bridge that gap by

> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index

> > > 7ea319e..fa046bd 100644

> > > --- a/include/linux/iommu.h

> > > +++ b/include/linux/iommu.h

> > > @@ -127,6 +127,7 @@ struct iommu_ops {

> > >  	int (*domain_set_windows)(struct iommu_domain *domain, u32 w_count);

> > >  	/* Get the numer of window per domain */

> > >  	u32 (*domain_get_windows)(struct iommu_domain *domain);

> > > +	struct iommu_domain *(*get_dev_iommu_domain)(struct device *dev);

> > >

> > >  	unsigned long pgsize_bitmap;

> > >  };

> > > @@ -190,6 +191,7 @@ extern int iommu_domain_window_enable(struct

> > > iommu_domain

> > *domain, u32 wnd_nr,

> > >  				      phys_addr_t offset, u64 size,

> > >  				      int prot);

> > >  extern void iommu_domain_window_disable(struct iommu_domain

> > > *domain,

> > > u32 wnd_nr);

> > > +extern struct iommu_domain *iommu_get_dev_domain(struct device

> > > +*dev);

> > >  /**

> > >   * report_iommu_fault() - report about an IOMMU fault to the IOMMU

> framework

> > >   * @domain: the iommu domain where the fault has happened @@ -284,6

> > > +286,11 @@ static inline void iommu_domain_window_disable(struct

> > > iommu_domain *domain,  {  }

> > >

> > > +static inline struct iommu_domain *iommu_get_dev_domain(struct

> > > +device

> > > +*dev) {

> > > +	return NULL;

> > > +}

> > > +

> > >  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain

> > > *domain, dma_addr_t iova)  {

> > >  	return 0;

> >

> >

> >

> > --

> > To unsubscribe from this list: send the line "unsubscribe linux-pci"

> > in the body of a message to majordomo@vger.kernel.org More majordomo

> > info at http://vger.kernel.org/majordomo-info.html

Alex Williamson Oct. 4, 2013, 3:45 p.m. UTC | #4

On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-owner@vger.kernel.org]
> > On Behalf Of Alex Williamson
> > Sent: Wednesday, September 25, 2013 10:16 PM
> > To: Bhushan Bharat-R65777
> > Cc: joro@8bytes.org; benh@kernel.crashing.org; galak@kernel.crashing.org; linux-
> > kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > pci@vger.kernel.org; agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > foundation.org; Bhushan Bharat-R65777
> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> > 
> > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > This api return the iommu domain to which the device is attached.
> > > The iommu_domain is required for making API calls related to iommu.
> > > Follow up patches which use this API to know iommu maping.
> > >
> > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > > ---
> > >  drivers/iommu/iommu.c |   10 ++++++++++
> > >  include/linux/iommu.h |    7 +++++++
> > >  2 files changed, 17 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> > > fbe9ca7..6ac5f50 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain
> > > *domain, struct device *dev)  }
> > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > >
> > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > > +	struct iommu_ops *ops = dev->bus->iommu_ops;
> > > +
> > > +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))
> > > +		return NULL;
> > > +
> > > +	return ops->get_dev_iommu_domain(dev); }
> > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > 
> > What prevents this from racing iommu_domain_free()?  There's no references
> > acquired, so there's no reason for the caller to assume the pointer is valid.
> 
> Sorry for late query, somehow this email went into a folder and escaped;
> 
> Just to be sure, there is not lock at generic "struct iommu_domain", but IP specific structure (link FSL domain) linked in iommu_domain->priv have a lock, so we need to ensure this race in FSL iommu code (say drivers/iommu/fsl_pamu_domain.c), right?

No, it's not sufficient to make sure that your use of the interface is
race free.  The interface itself needs to be designed so that it's
difficult to use incorrectly.  That's not the case here.  This is a
backdoor to get the iommu domain from the iommu driver regardless of who
is using it or how.  The iommu domain is created and managed by vfio, so
shouldn't we be looking at how to do this through vfio?  It seems like
you'd want to use your device to get a vfio group reference, from which
you could do something with the vfio external user interface and get the
iommu domain reference.  Thanks,

Alex
 
> > >  /*
> > >   * IOMMU groups are really the natrual working unit of the IOMMU, but
> > >   * the IOMMU API works on domains and devices.  Bridge that gap by
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > > 7ea319e..fa046bd 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -127,6 +127,7 @@ struct iommu_ops {
> > >  	int (*domain_set_windows)(struct iommu_domain *domain, u32 w_count);
> > >  	/* Get the numer of window per domain */
> > >  	u32 (*domain_get_windows)(struct iommu_domain *domain);
> > > +	struct iommu_domain *(*get_dev_iommu_domain)(struct device *dev);
> > >
> > >  	unsigned long pgsize_bitmap;
> > >  };
> > > @@ -190,6 +191,7 @@ extern int iommu_domain_window_enable(struct iommu_domain
> > *domain, u32 wnd_nr,
> > >  				      phys_addr_t offset, u64 size,
> > >  				      int prot);
> > >  extern void iommu_domain_window_disable(struct iommu_domain *domain,
> > > u32 wnd_nr);
> > > +extern struct iommu_domain *iommu_get_dev_domain(struct device *dev);
> > >  /**
> > >   * report_iommu_fault() - report about an IOMMU fault to the IOMMU framework
> > >   * @domain: the iommu domain where the fault has happened @@ -284,6
> > > +286,11 @@ static inline void iommu_domain_window_disable(struct
> > > iommu_domain *domain,  {  }
> > >
> > > +static inline struct iommu_domain *iommu_get_dev_domain(struct device
> > > +*dev) {
> > > +	return NULL;
> > > +}
> > > +
> > >  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain
> > > *domain, dma_addr_t iova)  {
> > >  	return 0;
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body
> > of a message to majordomo@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html
> 



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

Bharat Bhushan Oct. 4, 2013, 4:47 p.m. UTC | #5

> -----Original Message-----

> From: Alex Williamson [mailto:alex.williamson@redhat.com]

> Sent: Friday, October 04, 2013 9:15 PM

> To: Bhushan Bharat-R65777

> Cc: joro@8bytes.org; benh@kernel.crashing.org; galak@kernel.crashing.org; linux-

> kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-

> pci@vger.kernel.org; agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-

> foundation.org

> Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device

> 

> On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777 wrote:

> >

> > > -----Original Message-----

> > > From: linux-pci-owner@vger.kernel.org

> > > [mailto:linux-pci-owner@vger.kernel.org]

> > > On Behalf Of Alex Williamson

> > > Sent: Wednesday, September 25, 2013 10:16 PM

> > > To: Bhushan Bharat-R65777

> > > Cc: joro@8bytes.org; benh@kernel.crashing.org;

> > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;

> > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;

> > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org;

> > > Bhushan Bharat-R65777

> > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a

> > > device

> > >

> > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:

> > > > This api return the iommu domain to which the device is attached.

> > > > The iommu_domain is required for making API calls related to iommu.

> > > > Follow up patches which use this API to know iommu maping.

> > > >

> > > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

> > > > ---

> > > >  drivers/iommu/iommu.c |   10 ++++++++++

> > > >  include/linux/iommu.h |    7 +++++++

> > > >  2 files changed, 17 insertions(+), 0 deletions(-)

> > > >

> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index

> > > > fbe9ca7..6ac5f50 100644

> > > > --- a/drivers/iommu/iommu.c

> > > > +++ b/drivers/iommu/iommu.c

> > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain

> > > > *domain, struct device *dev)  }

> > > > EXPORT_SYMBOL_GPL(iommu_detach_device);

> > > >

> > > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {

> > > > +	struct iommu_ops *ops = dev->bus->iommu_ops;

> > > > +

> > > > +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))

> > > > +		return NULL;

> > > > +

> > > > +	return ops->get_dev_iommu_domain(dev); }

> > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);

> > >

> > > What prevents this from racing iommu_domain_free()?  There's no

> > > references acquired, so there's no reason for the caller to assume the

> pointer is valid.

> >

> > Sorry for late query, somehow this email went into a folder and

> > escaped;

> >

> > Just to be sure, there is not lock at generic "struct iommu_domain", but IP

> specific structure (link FSL domain) linked in iommu_domain->priv have a lock,

> so we need to ensure this race in FSL iommu code (say

> drivers/iommu/fsl_pamu_domain.c), right?

> 

> No, it's not sufficient to make sure that your use of the interface is race

> free.  The interface itself needs to be designed so that it's difficult to use

> incorrectly.


So we can define iommu_get_dev_domain()/iommu_put_dev_domain();  iommu_get_dev_domain() will return domain with the lock held, and iommu_put_dev_domain() will release the lock? And iommu_get_dev_domain() must always be followed by iommu_get_dev_domain().


> That's not the case here.  This is a backdoor to get the iommu

> domain from the iommu driver regardless of who is using it or how.  The iommu

> domain is created and managed by vfio, so shouldn't we be looking at how to do

> this through vfio?


Let me first describe what we are doing here:
During initialization:-
 - vfio talks to MSI system to know the MSI-page and size
 - vfio then interacts with iommu to map the MSI-page in iommu (IOVA is decided by userspace and physical address is the MSI-page)
 - So the IOVA subwindow mapping is created in iommu and yes VFIO know about this mapping.

Now do SET_IRQ(MSI/MSIX) ioctl:
 - calls pci_enable_msix()/pci_enable_msi_block(): which is supposed to set MSI address/data in device.
 - So in current implementation (this patchset) msi-subsystem gets the IOVA from iommu via this defined interface.
 - Are you saying that rather than getting this from iommu, we should get this from vfio? What difference does this make?

Thanks
-Bharat

> It seems like you'd want to use your device to get a vfio

> group reference, from which you could do something with the vfio external user

> interface and get the iommu domain reference.  Thanks,

> 

> Alex

> 

> > > >  /*

> > > >   * IOMMU groups are really the natrual working unit of the IOMMU, but

> > > >   * the IOMMU API works on domains and devices.  Bridge that gap

> > > > by diff --git a/include/linux/iommu.h b/include/linux/iommu.h

> > > > index 7ea319e..fa046bd 100644

> > > > --- a/include/linux/iommu.h

> > > > +++ b/include/linux/iommu.h

> > > > @@ -127,6 +127,7 @@ struct iommu_ops {

> > > >  	int (*domain_set_windows)(struct iommu_domain *domain, u32

> w_count);

> > > >  	/* Get the numer of window per domain */

> > > >  	u32 (*domain_get_windows)(struct iommu_domain *domain);

> > > > +	struct iommu_domain *(*get_dev_iommu_domain)(struct device

> > > > +*dev);

> > > >

> > > >  	unsigned long pgsize_bitmap;

> > > >  };

> > > > @@ -190,6 +191,7 @@ extern int iommu_domain_window_enable(struct

> > > > iommu_domain

> > > *domain, u32 wnd_nr,

> > > >  				      phys_addr_t offset, u64 size,

> > > >  				      int prot);

> > > >  extern void iommu_domain_window_disable(struct iommu_domain

> > > > *domain,

> > > > u32 wnd_nr);

> > > > +extern struct iommu_domain *iommu_get_dev_domain(struct device

> > > > +*dev);

> > > >  /**

> > > >   * report_iommu_fault() - report about an IOMMU fault to the IOMMU

> framework

> > > >   * @domain: the iommu domain where the fault has happened @@

> > > > -284,6

> > > > +286,11 @@ static inline void iommu_domain_window_disable(struct

> > > > iommu_domain *domain,  {  }

> > > >

> > > > +static inline struct iommu_domain *iommu_get_dev_domain(struct

> > > > +device

> > > > +*dev) {

> > > > +	return NULL;

> > > > +}

> > > > +

> > > >  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain

> > > > *domain, dma_addr_t iova)  {

> > > >  	return 0;

> > >

> > >

> > >

> > > --

> > > To unsubscribe from this list: send the line "unsubscribe linux-pci"

> > > in the body of a message to majordomo@vger.kernel.org More majordomo

> > > info at http://vger.kernel.org/majordomo-info.html

> >

> 

> 

>

Alex Williamson Oct. 4, 2013, 5:12 p.m. UTC | #6

On Fri, 2013-10-04 at 16:47 +0000, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Friday, October 04, 2013 9:15 PM
> > To: Bhushan Bharat-R65777
> > Cc: joro@8bytes.org; benh@kernel.crashing.org; galak@kernel.crashing.org; linux-
> > kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > pci@vger.kernel.org; agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > foundation.org
> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> > 
> > On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: linux-pci-owner@vger.kernel.org
> > > > [mailto:linux-pci-owner@vger.kernel.org]
> > > > On Behalf Of Alex Williamson
> > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org;
> > > > Bhushan Bharat-R65777
> > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > device
> > > >
> > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > > This api return the iommu domain to which the device is attached.
> > > > > The iommu_domain is required for making API calls related to iommu.
> > > > > Follow up patches which use this API to know iommu maping.
> > > > >
> > > > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > > > > ---
> > > > >  drivers/iommu/iommu.c |   10 ++++++++++
> > > > >  include/linux/iommu.h |    7 +++++++
> > > > >  2 files changed, 17 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> > > > > fbe9ca7..6ac5f50 100644
> > > > > --- a/drivers/iommu/iommu.c
> > > > > +++ b/drivers/iommu/iommu.c
> > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct iommu_domain
> > > > > *domain, struct device *dev)  }
> > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > >
> > > > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > > > > +	struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > > +
> > > > > +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))
> > > > > +		return NULL;
> > > > > +
> > > > > +	return ops->get_dev_iommu_domain(dev); }
> > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > > >
> > > > What prevents this from racing iommu_domain_free()?  There's no
> > > > references acquired, so there's no reason for the caller to assume the
> > pointer is valid.
> > >
> > > Sorry for late query, somehow this email went into a folder and
> > > escaped;
> > >
> > > Just to be sure, there is not lock at generic "struct iommu_domain", but IP
> > specific structure (link FSL domain) linked in iommu_domain->priv have a lock,
> > so we need to ensure this race in FSL iommu code (say
> > drivers/iommu/fsl_pamu_domain.c), right?
> > 
> > No, it's not sufficient to make sure that your use of the interface is race
> > free.  The interface itself needs to be designed so that it's difficult to use
> > incorrectly.
> 
> So we can define iommu_get_dev_domain()/iommu_put_dev_domain();
> iommu_get_dev_domain() will return domain with the lock held, and
> iommu_put_dev_domain() will release the lock? And
> iommu_get_dev_domain() must always be followed by
> iommu_get_dev_domain().

What lock?  get/put are generally used for reference counting, not
locking in the kernel.

> > That's not the case here.  This is a backdoor to get the iommu
> > domain from the iommu driver regardless of who is using it or how.  The iommu
> > domain is created and managed by vfio, so shouldn't we be looking at how to do
> > this through vfio?
> 
> Let me first describe what we are doing here:
> During initialization:-
>  - vfio talks to MSI system to know the MSI-page and size
>  - vfio then interacts with iommu to map the MSI-page in iommu (IOVA is decided by userspace and physical address is the MSI-page)
>  - So the IOVA subwindow mapping is created in iommu and yes VFIO know about this mapping.
> 
> Now do SET_IRQ(MSI/MSIX) ioctl:
>  - calls pci_enable_msix()/pci_enable_msi_block(): which is supposed to set MSI address/data in device.
>  - So in current implementation (this patchset) msi-subsystem gets the IOVA from iommu via this defined interface.
>  - Are you saying that rather than getting this from iommu, we should get this from vfio? What difference does this make?

Yes, you just said above that vfio knows the msi to iova mapping, so why
go outside of vfio to find it later?  The difference is one case you can
have a proper reference to data structures to make sure the pointer you
get back actually has meaning at the time you're using it vs the code
here where you're defining an API that returns a meaningless value
because you can't check or enforce that an arbitrary caller is using it
correctly.  It's not maintainable.  Thanks,

Alex

> > It seems like you'd want to use your device to get a vfio
> > group reference, from which you could do something with the vfio external user
> > interface and get the iommu domain reference.  Thanks,
> > 
> > Alex
> > 
> > > > >  /*
> > > > >   * IOMMU groups are really the natrual working unit of the IOMMU, but
> > > > >   * the IOMMU API works on domains and devices.  Bridge that gap
> > > > > by diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > > > index 7ea319e..fa046bd 100644
> > > > > --- a/include/linux/iommu.h
> > > > > +++ b/include/linux/iommu.h
> > > > > @@ -127,6 +127,7 @@ struct iommu_ops {
> > > > >  	int (*domain_set_windows)(struct iommu_domain *domain, u32
> > w_count);
> > > > >  	/* Get the numer of window per domain */
> > > > >  	u32 (*domain_get_windows)(struct iommu_domain *domain);
> > > > > +	struct iommu_domain *(*get_dev_iommu_domain)(struct device
> > > > > +*dev);
> > > > >
> > > > >  	unsigned long pgsize_bitmap;
> > > > >  };
> > > > > @@ -190,6 +191,7 @@ extern int iommu_domain_window_enable(struct
> > > > > iommu_domain
> > > > *domain, u32 wnd_nr,
> > > > >  				      phys_addr_t offset, u64 size,
> > > > >  				      int prot);
> > > > >  extern void iommu_domain_window_disable(struct iommu_domain
> > > > > *domain,
> > > > > u32 wnd_nr);
> > > > > +extern struct iommu_domain *iommu_get_dev_domain(struct device
> > > > > +*dev);
> > > > >  /**
> > > > >   * report_iommu_fault() - report about an IOMMU fault to the IOMMU
> > framework
> > > > >   * @domain: the iommu domain where the fault has happened @@
> > > > > -284,6
> > > > > +286,11 @@ static inline void iommu_domain_window_disable(struct
> > > > > iommu_domain *domain,  {  }
> > > > >
> > > > > +static inline struct iommu_domain *iommu_get_dev_domain(struct
> > > > > +device
> > > > > +*dev) {
> > > > > +	return NULL;
> > > > > +}
> > > > > +
> > > > >  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain
> > > > > *domain, dma_addr_t iova)  {
> > > > >  	return 0;
> > > >
> > > >
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> > > > in the body of a message to majordomo@vger.kernel.org More majordomo
> > > > info at http://vger.kernel.org/majordomo-info.html
> > >
> > 
> > 
> > 
> 



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

Bharat Bhushan Oct. 4, 2013, 5:23 p.m. UTC | #7

> -----Original Message-----

> From: Alex Williamson [mailto:alex.williamson@redhat.com]

> Sent: Friday, October 04, 2013 10:43 PM

> To: Bhushan Bharat-R65777

> Cc: joro@8bytes.org; benh@kernel.crashing.org; galak@kernel.crashing.org; linux-

> kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-

> pci@vger.kernel.org; agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-

> foundation.org

> Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device

> 

> On Fri, 2013-10-04 at 16:47 +0000, Bhushan Bharat-R65777 wrote:

> >

> > > -----Original Message-----

> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]

> > > Sent: Friday, October 04, 2013 9:15 PM

> > > To: Bhushan Bharat-R65777

> > > Cc: joro@8bytes.org; benh@kernel.crashing.org;

> > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;

> > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;

> > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org

> > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a

> > > device

> > >

> > > On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777 wrote:

> > > >

> > > > > -----Original Message-----

> > > > > From: linux-pci-owner@vger.kernel.org

> > > > > [mailto:linux-pci-owner@vger.kernel.org]

> > > > > On Behalf Of Alex Williamson

> > > > > Sent: Wednesday, September 25, 2013 10:16 PM

> > > > > To: Bhushan Bharat-R65777

> > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;

> > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;

> > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;

> > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-

> > > > > foundation.org; Bhushan Bharat-R65777

> > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a

> > > > > device

> > > > >

> > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:

> > > > > > This api return the iommu domain to which the device is attached.

> > > > > > The iommu_domain is required for making API calls related to iommu.

> > > > > > Follow up patches which use this API to know iommu maping.

> > > > > >

> > > > > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

> > > > > > ---

> > > > > >  drivers/iommu/iommu.c |   10 ++++++++++

> > > > > >  include/linux/iommu.h |    7 +++++++

> > > > > >  2 files changed, 17 insertions(+), 0 deletions(-)

> > > > > >

> > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c

> > > > > > index

> > > > > > fbe9ca7..6ac5f50 100644

> > > > > > --- a/drivers/iommu/iommu.c

> > > > > > +++ b/drivers/iommu/iommu.c

> > > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct

> > > > > > iommu_domain *domain, struct device *dev)  }

> > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);

> > > > > >

> > > > > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {

> > > > > > +	struct iommu_ops *ops = dev->bus->iommu_ops;

> > > > > > +

> > > > > > +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))

> > > > > > +		return NULL;

> > > > > > +

> > > > > > +	return ops->get_dev_iommu_domain(dev); }

> > > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);

> > > > >

> > > > > What prevents this from racing iommu_domain_free()?  There's no

> > > > > references acquired, so there's no reason for the caller to

> > > > > assume the

> > > pointer is valid.

> > > >

> > > > Sorry for late query, somehow this email went into a folder and

> > > > escaped;

> > > >

> > > > Just to be sure, there is not lock at generic "struct

> > > > iommu_domain", but IP

> > > specific structure (link FSL domain) linked in iommu_domain->priv

> > > have a lock, so we need to ensure this race in FSL iommu code (say

> > > drivers/iommu/fsl_pamu_domain.c), right?

> > >

> > > No, it's not sufficient to make sure that your use of the interface

> > > is race free.  The interface itself needs to be designed so that

> > > it's difficult to use incorrectly.

> >

> > So we can define iommu_get_dev_domain()/iommu_put_dev_domain();

> > iommu_get_dev_domain() will return domain with the lock held, and

> > iommu_put_dev_domain() will release the lock? And

> > iommu_get_dev_domain() must always be followed by

> > iommu_get_dev_domain().

> 

> What lock?  get/put are generally used for reference counting, not locking in

> the kernel.

> 

> > > That's not the case here.  This is a backdoor to get the iommu

> > > domain from the iommu driver regardless of who is using it or how.

> > > The iommu domain is created and managed by vfio, so shouldn't we be

> > > looking at how to do this through vfio?

> >

> > Let me first describe what we are doing here:

> > During initialization:-

> >  - vfio talks to MSI system to know the MSI-page and size

> >  - vfio then interacts with iommu to map the MSI-page in iommu (IOVA

> > is decided by userspace and physical address is the MSI-page)

> >  - So the IOVA subwindow mapping is created in iommu and yes VFIO know about

> this mapping.

> >

> > Now do SET_IRQ(MSI/MSIX) ioctl:

> >  - calls pci_enable_msix()/pci_enable_msi_block(): which is supposed to set

> MSI address/data in device.

> >  - So in current implementation (this patchset) msi-subsystem gets the IOVA

> from iommu via this defined interface.

> >  - Are you saying that rather than getting this from iommu, we should get this

> from vfio? What difference does this make?

> 

> Yes, you just said above that vfio knows the msi to iova mapping, so why go

> outside of vfio to find it later?  The difference is one case you can have a

> proper reference to data structures to make sure the pointer you get back

> actually has meaning at the time you're using it vs the code here where you're

> defining an API that returns a meaningless value


With FSL-PAMU we will always get consistant data from iommu or vfio-data structure.

> because you can't check or

> enforce that an arbitrary caller is using it correctly.


I am not sure what is arbitrary caller? pdev is known to vfio, so vfio will only make pci_enable_msix()/pci_enable_msi_block() for this pdev. If anyother code makes then it is some other unexpectedly thing happening in system, no?


>  It's not maintainable.

> Thanks,


I do not have any issue with this as well, can you also describe the type of API you are envisioning;
I can think of defining some function in vfio.c/vfio_iommu*.c, make them global and declare then in include/Linux/vfio.h
And include <Linux/vfio.h> in caller file (arch/powerpc/kernel/msi.c)

Thanks
-Bharat
> 

> Alex

> 

> > > It seems like you'd want to use your device to get a vfio group

> > > reference, from which you could do something with the vfio external

> > > user interface and get the iommu domain reference.  Thanks,

> > >

> > > Alex

> > >

> > > > > >  /*

> > > > > >   * IOMMU groups are really the natrual working unit of the IOMMU, but

> > > > > >   * the IOMMU API works on domains and devices.  Bridge that

> > > > > > gap by diff --git a/include/linux/iommu.h

> > > > > > b/include/linux/iommu.h index 7ea319e..fa046bd 100644

> > > > > > --- a/include/linux/iommu.h

> > > > > > +++ b/include/linux/iommu.h

> > > > > > @@ -127,6 +127,7 @@ struct iommu_ops {

> > > > > >  	int (*domain_set_windows)(struct iommu_domain *domain, u32

> > > w_count);

> > > > > >  	/* Get the numer of window per domain */

> > > > > >  	u32 (*domain_get_windows)(struct iommu_domain *domain);

> > > > > > +	struct iommu_domain *(*get_dev_iommu_domain)(struct device

> > > > > > +*dev);

> > > > > >

> > > > > >  	unsigned long pgsize_bitmap;  }; @@ -190,6 +191,7 @@ extern

> > > > > > int iommu_domain_window_enable(struct iommu_domain

> > > > > *domain, u32 wnd_nr,

> > > > > >  				      phys_addr_t offset, u64 size,

> > > > > >  				      int prot);

> > > > > >  extern void iommu_domain_window_disable(struct iommu_domain

> > > > > > *domain,

> > > > > > u32 wnd_nr);

> > > > > > +extern struct iommu_domain *iommu_get_dev_domain(struct

> > > > > > +device *dev);

> > > > > >  /**

> > > > > >   * report_iommu_fault() - report about an IOMMU fault to the

> > > > > > IOMMU

> > > framework

> > > > > >   * @domain: the iommu domain where the fault has happened @@

> > > > > > -284,6

> > > > > > +286,11 @@ static inline void

> > > > > > +iommu_domain_window_disable(struct

> > > > > > iommu_domain *domain,  {  }

> > > > > >

> > > > > > +static inline struct iommu_domain

> > > > > > +*iommu_get_dev_domain(struct device

> > > > > > +*dev) {

> > > > > > +	return NULL;

> > > > > > +}

> > > > > > +

> > > > > >  static inline phys_addr_t iommu_iova_to_phys(struct

> > > > > > iommu_domain *domain, dma_addr_t iova)  {

> > > > > >  	return 0;

> > > > >

> > > > >

> > > > >

> > > > > --

> > > > > To unsubscribe from this list: send the line "unsubscribe linux-pci"

> > > > > in the body of a message to majordomo@vger.kernel.org More

> > > > > majordomo info at http://vger.kernel.org/majordomo-info.html

> > > >

> > >

> > >

> > >

> >

> 

> 

>

Alex Williamson Oct. 4, 2013, 6:12 p.m. UTC | #8

On Fri, 2013-10-04 at 17:23 +0000, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Friday, October 04, 2013 10:43 PM
> > To: Bhushan Bharat-R65777
> > Cc: joro@8bytes.org; benh@kernel.crashing.org; galak@kernel.crashing.org; linux-
> > kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > pci@vger.kernel.org; agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > foundation.org
> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> > 
> > On Fri, 2013-10-04 at 16:47 +0000, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > Sent: Friday, October 04, 2013 9:15 PM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org
> > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > device
> > > >
> > > > On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777 wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: linux-pci-owner@vger.kernel.org
> > > > > > [mailto:linux-pci-owner@vger.kernel.org]
> > > > > > On Behalf Of Alex Williamson
> > > > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > > > To: Bhushan Bharat-R65777
> > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > foundation.org; Bhushan Bharat-R65777
> > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > > > device
> > > > > >
> > > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > > > > This api return the iommu domain to which the device is attached.
> > > > > > > The iommu_domain is required for making API calls related to iommu.
> > > > > > > Follow up patches which use this API to know iommu maping.
> > > > > > >
> > > > > > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > > > > > > ---
> > > > > > >  drivers/iommu/iommu.c |   10 ++++++++++
> > > > > > >  include/linux/iommu.h |    7 +++++++
> > > > > > >  2 files changed, 17 insertions(+), 0 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > > > index
> > > > > > > fbe9ca7..6ac5f50 100644
> > > > > > > --- a/drivers/iommu/iommu.c
> > > > > > > +++ b/drivers/iommu/iommu.c
> > > > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct
> > > > > > > iommu_domain *domain, struct device *dev)  }
> > > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > > > >
> > > > > > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > > > > > > +	struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > > > > +
> > > > > > > +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain == NULL))
> > > > > > > +		return NULL;
> > > > > > > +
> > > > > > > +	return ops->get_dev_iommu_domain(dev); }
> > > > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > > > > >
> > > > > > What prevents this from racing iommu_domain_free()?  There's no
> > > > > > references acquired, so there's no reason for the caller to
> > > > > > assume the
> > > > pointer is valid.
> > > > >
> > > > > Sorry for late query, somehow this email went into a folder and
> > > > > escaped;
> > > > >
> > > > > Just to be sure, there is not lock at generic "struct
> > > > > iommu_domain", but IP
> > > > specific structure (link FSL domain) linked in iommu_domain->priv
> > > > have a lock, so we need to ensure this race in FSL iommu code (say
> > > > drivers/iommu/fsl_pamu_domain.c), right?
> > > >
> > > > No, it's not sufficient to make sure that your use of the interface
> > > > is race free.  The interface itself needs to be designed so that
> > > > it's difficult to use incorrectly.
> > >
> > > So we can define iommu_get_dev_domain()/iommu_put_dev_domain();
> > > iommu_get_dev_domain() will return domain with the lock held, and
> > > iommu_put_dev_domain() will release the lock? And
> > > iommu_get_dev_domain() must always be followed by
> > > iommu_get_dev_domain().
> > 
> > What lock?  get/put are generally used for reference counting, not locking in
> > the kernel.
> > 
> > > > That's not the case here.  This is a backdoor to get the iommu
> > > > domain from the iommu driver regardless of who is using it or how.
> > > > The iommu domain is created and managed by vfio, so shouldn't we be
> > > > looking at how to do this through vfio?
> > >
> > > Let me first describe what we are doing here:
> > > During initialization:-
> > >  - vfio talks to MSI system to know the MSI-page and size
> > >  - vfio then interacts with iommu to map the MSI-page in iommu (IOVA
> > > is decided by userspace and physical address is the MSI-page)
> > >  - So the IOVA subwindow mapping is created in iommu and yes VFIO know about
> > this mapping.
> > >
> > > Now do SET_IRQ(MSI/MSIX) ioctl:
> > >  - calls pci_enable_msix()/pci_enable_msi_block(): which is supposed to set
> > MSI address/data in device.
> > >  - So in current implementation (this patchset) msi-subsystem gets the IOVA
> > from iommu via this defined interface.
> > >  - Are you saying that rather than getting this from iommu, we should get this
> > from vfio? What difference does this make?
> > 
> > Yes, you just said above that vfio knows the msi to iova mapping, so why go
> > outside of vfio to find it later?  The difference is one case you can have a
> > proper reference to data structures to make sure the pointer you get back
> > actually has meaning at the time you're using it vs the code here where you're
> > defining an API that returns a meaningless value
> 
> With FSL-PAMU we will always get consistant data from iommu or vfio-data structure.

Great, but you're trying to add a generic API to the IOMMU subsystem
that's difficult to use correctly.  The fact that you use it correctly
does not justify the API.

> > because you can't check or
> > enforce that an arbitrary caller is using it correctly.
> 
> I am not sure what is arbitrary caller? pdev is known to vfio, so vfio
> will only make pci_enable_msix()/pci_enable_msi_block() for this pdev.
> If anyother code makes then it is some other unexpectedly thing
> happening in system, no?

What's proposed here is a generic IOMMU API.  Anybody can call this.
What if the host SCSI driver decides to go get the iommu domain for it's
device (or any other device)?  Does that fit your usage model?

> >  It's not maintainable.
> > Thanks,
> 
> I do not have any issue with this as well, can you also describe the type of API you are envisioning;
> I can think of defining some function in vfio.c/vfio_iommu*.c, make them global and declare then in include/Linux/vfio.h
> And include <Linux/vfio.h> in caller file (arch/powerpc/kernel/msi.c)

Do you really want module dependencies between vfio and your core kernel
MSI setup?  Look at the vfio external user interface that we've already
defined.  That allows other components of the kernel to get a proper
reference to a vfio group.  From there you can work out how to get what
you want.  Another alternative is that vfio could register an MSI to
IOVA mapping with architecture code when the mapping is created.  The
MSI setup path could then do a lookup in architecture code for the
mapping.  You could even store the MSI to IOVA mapping in VFIO and
create an interface where SET_IRQ passes that mapping into setup code.
Thanks,

Alex

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

Bharat Bhushan Oct. 7, 2013, 5:46 a.m. UTC | #9

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQWxleCBXaWxsaWFtc29u
IFttYWlsdG86YWxleC53aWxsaWFtc29uQHJlZGhhdC5jb21dDQo+IFNlbnQ6IEZyaWRheSwgT2N0
b2JlciAwNCwgMjAxMyAxMTo0MiBQTQ0KPiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3DQo+IENj
OiBqb3JvQDhieXRlcy5vcmc7IGJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZzsgZ2FsYWtAa2VybmVs
LmNyYXNoaW5nLm9yZzsgbGludXgtDQo+IGtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4cHBj
LWRldkBsaXN0cy5vemxhYnMub3JnOyBsaW51eC0NCj4gcGNpQHZnZXIua2VybmVsLm9yZzsgYWdy
YWZAc3VzZS5kZTsgV29vZCBTY290dC1CMDc0MjE7IGlvbW11QGxpc3RzLmxpbnV4LQ0KPiBmb3Vu
ZGF0aW9uLm9yZw0KPiBTdWJqZWN0OiBSZTogW1BBVENIIDIvN10gaW9tbXU6IGFkZCBhcGkgdG8g
Z2V0IGlvbW11X2RvbWFpbiBvZiBhIGRldmljZQ0KPiANCj4gT24gRnJpLCAyMDEzLTEwLTA0IGF0
IDE3OjIzICswMDAwLCBCaHVzaGFuIEJoYXJhdC1SNjU3Nzcgd3JvdGU6DQo+ID4NCj4gPiA+IC0t
LS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4gPiBGcm9tOiBBbGV4IFdpbGxpYW1zb24gW21h
aWx0bzphbGV4LndpbGxpYW1zb25AcmVkaGF0LmNvbV0NCj4gPiA+IFNlbnQ6IEZyaWRheSwgT2N0
b2JlciAwNCwgMjAxMyAxMDo0MyBQTQ0KPiA+ID4gVG86IEJodXNoYW4gQmhhcmF0LVI2NTc3Nw0K
PiA+ID4gQ2M6IGpvcm9AOGJ5dGVzLm9yZzsgYmVuaEBrZXJuZWwuY3Jhc2hpbmcub3JnOw0KPiA+
ID4gZ2FsYWtAa2VybmVsLmNyYXNoaW5nLm9yZzsgbGludXgtIGtlcm5lbEB2Z2VyLmtlcm5lbC5v
cmc7DQo+ID4gPiBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZzsgbGludXgtIHBjaUB2Z2Vy
Lmtlcm5lbC5vcmc7DQo+ID4gPiBhZ3JhZkBzdXNlLmRlOyBXb29kIFNjb3R0LUIwNzQyMTsgaW9t
bXVAbGlzdHMubGludXgtIGZvdW5kYXRpb24ub3JnDQo+ID4gPiBTdWJqZWN0OiBSZTogW1BBVENI
IDIvN10gaW9tbXU6IGFkZCBhcGkgdG8gZ2V0IGlvbW11X2RvbWFpbiBvZiBhDQo+ID4gPiBkZXZp
Y2UNCj4gPiA+DQo+ID4gPiBPbiBGcmksIDIwMTMtMTAtMDQgYXQgMTY6NDcgKzAwMDAsIEJodXNo
YW4gQmhhcmF0LVI2NTc3NyB3cm90ZToNCj4gPiA+ID4NCj4gPiA+ID4gPiAtLS0tLU9yaWdpbmFs
IE1lc3NhZ2UtLS0tLQ0KPiA+ID4gPiA+IEZyb206IEFsZXggV2lsbGlhbXNvbiBbbWFpbHRvOmFs
ZXgud2lsbGlhbXNvbkByZWRoYXQuY29tXQ0KPiA+ID4gPiA+IFNlbnQ6IEZyaWRheSwgT2N0b2Jl
ciAwNCwgMjAxMyA5OjE1IFBNDQo+ID4gPiA+ID4gVG86IEJodXNoYW4gQmhhcmF0LVI2NTc3Nw0K
PiA+ID4gPiA+IENjOiBqb3JvQDhieXRlcy5vcmc7IGJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZzsN
Cj4gPiA+ID4gPiBnYWxha0BrZXJuZWwuY3Jhc2hpbmcub3JnOyBsaW51eC0ga2VybmVsQHZnZXIu
a2VybmVsLm9yZzsNCj4gPiA+ID4gPiBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZzsgbGlu
dXgtIHBjaUB2Z2VyLmtlcm5lbC5vcmc7DQo+ID4gPiA+ID4gYWdyYWZAc3VzZS5kZTsgV29vZCBT
Y290dC1CMDc0MjE7IGlvbW11QGxpc3RzLmxpbnV4LQ0KPiA+ID4gPiA+IGZvdW5kYXRpb24ub3Jn
DQo+ID4gPiA+ID4gU3ViamVjdDogUmU6IFtQQVRDSCAyLzddIGlvbW11OiBhZGQgYXBpIHRvIGdl
dCBpb21tdV9kb21haW4gb2YgYQ0KPiA+ID4gPiA+IGRldmljZQ0KPiA+ID4gPiA+DQo+ID4gPiA+
ID4gT24gRnJpLCAyMDEzLTEwLTA0IGF0IDA5OjU0ICswMDAwLCBCaHVzaGFuIEJoYXJhdC1SNjU3
Nzcgd3JvdGU6DQo+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiAtLS0tLU9yaWdpbmFsIE1lc3Nh
Z2UtLS0tLQ0KPiA+ID4gPiA+ID4gPiBGcm9tOiBsaW51eC1wY2ktb3duZXJAdmdlci5rZXJuZWwu
b3JnDQo+ID4gPiA+ID4gPiA+IFttYWlsdG86bGludXgtcGNpLW93bmVyQHZnZXIua2VybmVsLm9y
Z10NCj4gPiA+ID4gPiA+ID4gT24gQmVoYWxmIE9mIEFsZXggV2lsbGlhbXNvbg0KPiA+ID4gPiA+
ID4gPiBTZW50OiBXZWRuZXNkYXksIFNlcHRlbWJlciAyNSwgMjAxMyAxMDoxNiBQTQ0KPiA+ID4g
PiA+ID4gPiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3DQo+ID4gPiA+ID4gPiA+IENjOiBqb3Jv
QDhieXRlcy5vcmc7IGJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZzsNCj4gPiA+ID4gPiA+ID4gZ2Fs
YWtAa2VybmVsLmNyYXNoaW5nLm9yZzsgbGludXgtIGtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7DQo+
ID4gPiA+ID4gPiA+IGxpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMub3JnOyBsaW51eC0gcGNpQHZn
ZXIua2VybmVsLm9yZzsNCj4gPiA+ID4gPiA+ID4gYWdyYWZAc3VzZS5kZTsgV29vZCBTY290dC1C
MDc0MjE7IGlvbW11QGxpc3RzLmxpbnV4LQ0KPiA+ID4gPiA+ID4gPiBmb3VuZGF0aW9uLm9yZzsg
Qmh1c2hhbiBCaGFyYXQtUjY1Nzc3DQo+ID4gPiA+ID4gPiA+IFN1YmplY3Q6IFJlOiBbUEFUQ0gg
Mi83XSBpb21tdTogYWRkIGFwaSB0byBnZXQgaW9tbXVfZG9tYWluDQo+ID4gPiA+ID4gPiA+IG9m
IGEgZGV2aWNlDQo+ID4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiA+IE9uIFRodSwgMjAxMy0wOS0x
OSBhdCAxMjo1OSArMDUzMCwgQmhhcmF0IEJodXNoYW4gd3JvdGU6DQo+ID4gPiA+ID4gPiA+ID4g
VGhpcyBhcGkgcmV0dXJuIHRoZSBpb21tdSBkb21haW4gdG8gd2hpY2ggdGhlIGRldmljZSBpcyBh
dHRhY2hlZC4NCj4gPiA+ID4gPiA+ID4gPiBUaGUgaW9tbXVfZG9tYWluIGlzIHJlcXVpcmVkIGZv
ciBtYWtpbmcgQVBJIGNhbGxzIHJlbGF0ZWQgdG8NCj4gaW9tbXUuDQo+ID4gPiA+ID4gPiA+ID4g
Rm9sbG93IHVwIHBhdGNoZXMgd2hpY2ggdXNlIHRoaXMgQVBJIHRvIGtub3cgaW9tbXUgbWFwaW5n
Lg0KPiA+ID4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiA+ID4gU2lnbmVkLW9mZi1ieTogQmhhcmF0
IEJodXNoYW4NCj4gPiA+ID4gPiA+ID4gPiA8YmhhcmF0LmJodXNoYW5AZnJlZXNjYWxlLmNvbT4N
Cj4gPiA+ID4gPiA+ID4gPiAtLS0NCj4gPiA+ID4gPiA+ID4gPiAgZHJpdmVycy9pb21tdS9pb21t
dS5jIHwgICAxMCArKysrKysrKysrDQo+ID4gPiA+ID4gPiA+ID4gIGluY2x1ZGUvbGludXgvaW9t
bXUuaCB8ICAgIDcgKysrKysrKw0KPiA+ID4gPiA+ID4gPiA+ICAyIGZpbGVzIGNoYW5nZWQsIDE3
IGluc2VydGlvbnMoKyksIDAgZGVsZXRpb25zKC0pDQo+ID4gPiA+ID4gPiA+ID4NCj4gPiA+ID4g
PiA+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9pb21tdS9pb21tdS5jIGIvZHJpdmVycy9pb21t
dS9pb21tdS5jDQo+ID4gPiA+ID4gPiA+ID4gaW5kZXgNCj4gPiA+ID4gPiA+ID4gPiBmYmU5Y2E3
Li42YWM1ZjUwIDEwMDY0NA0KPiA+ID4gPiA+ID4gPiA+IC0tLSBhL2RyaXZlcnMvaW9tbXUvaW9t
bXUuYw0KPiA+ID4gPiA+ID4gPiA+ICsrKyBiL2RyaXZlcnMvaW9tbXUvaW9tbXUuYw0KPiA+ID4g
PiA+ID4gPiA+IEBAIC02OTYsNiArNjk2LDE2IEBAIHZvaWQgaW9tbXVfZGV0YWNoX2RldmljZShz
dHJ1Y3QNCj4gPiA+ID4gPiA+ID4gPiBpb21tdV9kb21haW4gKmRvbWFpbiwgc3RydWN0IGRldmlj
ZSAqZGV2KSAgfQ0KPiA+ID4gPiA+ID4gPiA+IEVYUE9SVF9TWU1CT0xfR1BMKGlvbW11X2RldGFj
aF9kZXZpY2UpOw0KPiA+ID4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiA+ID4gK3N0cnVjdCBpb21t
dV9kb21haW4gKmlvbW11X2dldF9kZXZfZG9tYWluKHN0cnVjdCBkZXZpY2UgKmRldikgew0KPiA+
ID4gPiA+ID4gPiA+ICsJc3RydWN0IGlvbW11X29wcyAqb3BzID0gZGV2LT5idXMtPmlvbW11X29w
czsNCj4gPiA+ID4gPiA+ID4gPiArDQo+ID4gPiA+ID4gPiA+ID4gKwlpZiAodW5saWtlbHkob3Bz
ID09IE5VTEwgfHwgb3BzLT5nZXRfZGV2X2lvbW11X2RvbWFpbiA9PQ0KPiBOVUxMKSkNCj4gPiA+
ID4gPiA+ID4gPiArCQlyZXR1cm4gTlVMTDsNCj4gPiA+ID4gPiA+ID4gPiArDQo+ID4gPiA+ID4g
PiA+ID4gKwlyZXR1cm4gb3BzLT5nZXRfZGV2X2lvbW11X2RvbWFpbihkZXYpOyB9DQo+ID4gPiA+
ID4gPiA+ID4gK0VYUE9SVF9TWU1CT0xfR1BMKGlvbW11X2dldF9kZXZfZG9tYWluKTsNCj4gPiA+
ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gV2hhdCBwcmV2ZW50cyB0aGlzIGZyb20gcmFjaW5nIGlv
bW11X2RvbWFpbl9mcmVlKCk/ICBUaGVyZSdzDQo+ID4gPiA+ID4gPiA+IG5vIHJlZmVyZW5jZXMg
YWNxdWlyZWQsIHNvIHRoZXJlJ3Mgbm8gcmVhc29uIGZvciB0aGUgY2FsbGVyDQo+ID4gPiA+ID4g
PiA+IHRvIGFzc3VtZSB0aGUNCj4gPiA+ID4gPiBwb2ludGVyIGlzIHZhbGlkLg0KPiA+ID4gPiA+
ID4NCj4gPiA+ID4gPiA+IFNvcnJ5IGZvciBsYXRlIHF1ZXJ5LCBzb21laG93IHRoaXMgZW1haWwg
d2VudCBpbnRvIGEgZm9sZGVyDQo+ID4gPiA+ID4gPiBhbmQgZXNjYXBlZDsNCj4gPiA+ID4gPiA+
DQo+ID4gPiA+ID4gPiBKdXN0IHRvIGJlIHN1cmUsIHRoZXJlIGlzIG5vdCBsb2NrIGF0IGdlbmVy
aWMgInN0cnVjdA0KPiA+ID4gPiA+ID4gaW9tbXVfZG9tYWluIiwgYnV0IElQDQo+ID4gPiA+ID4g
c3BlY2lmaWMgc3RydWN0dXJlIChsaW5rIEZTTCBkb21haW4pIGxpbmtlZCBpbg0KPiA+ID4gPiA+
IGlvbW11X2RvbWFpbi0+cHJpdiBoYXZlIGEgbG9jaywgc28gd2UgbmVlZCB0byBlbnN1cmUgdGhp
cyByYWNlDQo+ID4gPiA+ID4gaW4gRlNMIGlvbW11IGNvZGUgKHNheSBkcml2ZXJzL2lvbW11L2Zz
bF9wYW11X2RvbWFpbi5jKSwgcmlnaHQ/DQo+ID4gPiA+ID4NCj4gPiA+ID4gPiBObywgaXQncyBu
b3Qgc3VmZmljaWVudCB0byBtYWtlIHN1cmUgdGhhdCB5b3VyIHVzZSBvZiB0aGUNCj4gPiA+ID4g
PiBpbnRlcmZhY2UgaXMgcmFjZSBmcmVlLiAgVGhlIGludGVyZmFjZSBpdHNlbGYgbmVlZHMgdG8g
YmUNCj4gPiA+ID4gPiBkZXNpZ25lZCBzbyB0aGF0IGl0J3MgZGlmZmljdWx0IHRvIHVzZSBpbmNv
cnJlY3RseS4NCj4gPiA+ID4NCj4gPiA+ID4gU28gd2UgY2FuIGRlZmluZSBpb21tdV9nZXRfZGV2
X2RvbWFpbigpL2lvbW11X3B1dF9kZXZfZG9tYWluKCk7DQo+ID4gPiA+IGlvbW11X2dldF9kZXZf
ZG9tYWluKCkgd2lsbCByZXR1cm4gZG9tYWluIHdpdGggdGhlIGxvY2sgaGVsZCwgYW5kDQo+ID4g
PiA+IGlvbW11X3B1dF9kZXZfZG9tYWluKCkgd2lsbCByZWxlYXNlIHRoZSBsb2NrPyBBbmQNCj4g
PiA+ID4gaW9tbXVfZ2V0X2Rldl9kb21haW4oKSBtdXN0IGFsd2F5cyBiZSBmb2xsb3dlZCBieQ0K
PiA+ID4gPiBpb21tdV9nZXRfZGV2X2RvbWFpbigpLg0KPiA+ID4NCj4gPiA+IFdoYXQgbG9jaz8g
IGdldC9wdXQgYXJlIGdlbmVyYWxseSB1c2VkIGZvciByZWZlcmVuY2UgY291bnRpbmcsIG5vdA0K
PiA+ID4gbG9ja2luZyBpbiB0aGUga2VybmVsLg0KPiA+ID4NCj4gPiA+ID4gPiBUaGF0J3Mgbm90
IHRoZSBjYXNlIGhlcmUuICBUaGlzIGlzIGEgYmFja2Rvb3IgdG8gZ2V0IHRoZSBpb21tdQ0KPiA+
ID4gPiA+IGRvbWFpbiBmcm9tIHRoZSBpb21tdSBkcml2ZXIgcmVnYXJkbGVzcyBvZiB3aG8gaXMg
dXNpbmcgaXQgb3IgaG93Lg0KPiA+ID4gPiA+IFRoZSBpb21tdSBkb21haW4gaXMgY3JlYXRlZCBh
bmQgbWFuYWdlZCBieSB2ZmlvLCBzbyBzaG91bGRuJ3Qgd2UNCj4gPiA+ID4gPiBiZSBsb29raW5n
IGF0IGhvdyB0byBkbyB0aGlzIHRocm91Z2ggdmZpbz8NCj4gPiA+ID4NCj4gPiA+ID4gTGV0IG1l
IGZpcnN0IGRlc2NyaWJlIHdoYXQgd2UgYXJlIGRvaW5nIGhlcmU6DQo+ID4gPiA+IER1cmluZyBp
bml0aWFsaXphdGlvbjotDQo+ID4gPiA+ICAtIHZmaW8gdGFsa3MgdG8gTVNJIHN5c3RlbSB0byBr
bm93IHRoZSBNU0ktcGFnZSBhbmQgc2l6ZQ0KPiA+ID4gPiAgLSB2ZmlvIHRoZW4gaW50ZXJhY3Rz
IHdpdGggaW9tbXUgdG8gbWFwIHRoZSBNU0ktcGFnZSBpbiBpb21tdQ0KPiA+ID4gPiAoSU9WQSBp
cyBkZWNpZGVkIGJ5IHVzZXJzcGFjZSBhbmQgcGh5c2ljYWwgYWRkcmVzcyBpcyB0aGUNCj4gPiA+
ID4gTVNJLXBhZ2UpDQo+ID4gPiA+ICAtIFNvIHRoZSBJT1ZBIHN1YndpbmRvdyBtYXBwaW5nIGlz
IGNyZWF0ZWQgaW4gaW9tbXUgYW5kIHllcyBWRklPDQo+ID4gPiA+IGtub3cgYWJvdXQNCj4gPiA+
IHRoaXMgbWFwcGluZy4NCj4gPiA+ID4NCj4gPiA+ID4gTm93IGRvIFNFVF9JUlEoTVNJL01TSVgp
IGlvY3RsOg0KPiA+ID4gPiAgLSBjYWxscyBwY2lfZW5hYmxlX21zaXgoKS9wY2lfZW5hYmxlX21z
aV9ibG9jaygpOiB3aGljaCBpcw0KPiA+ID4gPiBzdXBwb3NlZCB0byBzZXQNCj4gPiA+IE1TSSBh
ZGRyZXNzL2RhdGEgaW4gZGV2aWNlLg0KPiA+ID4gPiAgLSBTbyBpbiBjdXJyZW50IGltcGxlbWVu
dGF0aW9uICh0aGlzIHBhdGNoc2V0KSBtc2ktc3Vic3lzdGVtIGdldHMNCj4gPiA+ID4gdGhlIElP
VkENCj4gPiA+IGZyb20gaW9tbXUgdmlhIHRoaXMgZGVmaW5lZCBpbnRlcmZhY2UuDQo+ID4gPiA+
ICAtIEFyZSB5b3Ugc2F5aW5nIHRoYXQgcmF0aGVyIHRoYW4gZ2V0dGluZyB0aGlzIGZyb20gaW9t
bXUsIHdlDQo+ID4gPiA+IHNob3VsZCBnZXQgdGhpcw0KPiA+ID4gZnJvbSB2ZmlvPyBXaGF0IGRp
ZmZlcmVuY2UgZG9lcyB0aGlzIG1ha2U/DQo+ID4gPg0KPiA+ID4gWWVzLCB5b3UganVzdCBzYWlk
IGFib3ZlIHRoYXQgdmZpbyBrbm93cyB0aGUgbXNpIHRvIGlvdmEgbWFwcGluZywgc28NCj4gPiA+
IHdoeSBnbyBvdXRzaWRlIG9mIHZmaW8gdG8gZmluZCBpdCBsYXRlcj8gIFRoZSBkaWZmZXJlbmNl
IGlzIG9uZSBjYXNlDQo+ID4gPiB5b3UgY2FuIGhhdmUgYSBwcm9wZXIgcmVmZXJlbmNlIHRvIGRh
dGEgc3RydWN0dXJlcyB0byBtYWtlIHN1cmUgdGhlDQo+ID4gPiBwb2ludGVyIHlvdSBnZXQgYmFj
ayBhY3R1YWxseSBoYXMgbWVhbmluZyBhdCB0aGUgdGltZSB5b3UncmUgdXNpbmcNCj4gPiA+IGl0
IHZzIHRoZSBjb2RlIGhlcmUgd2hlcmUgeW91J3JlIGRlZmluaW5nIGFuIEFQSSB0aGF0IHJldHVy
bnMgYQ0KPiA+ID4gbWVhbmluZ2xlc3MgdmFsdWUNCj4gPg0KPiA+IFdpdGggRlNMLVBBTVUgd2Ug
d2lsbCBhbHdheXMgZ2V0IGNvbnNpc3RhbnQgZGF0YSBmcm9tIGlvbW11IG9yIHZmaW8tZGF0YQ0K
PiBzdHJ1Y3R1cmUuDQo+IA0KPiBHcmVhdCwgYnV0IHlvdSdyZSB0cnlpbmcgdG8gYWRkIGEgZ2Vu
ZXJpYyBBUEkgdG8gdGhlIElPTU1VIHN1YnN5c3RlbSB0aGF0J3MNCj4gZGlmZmljdWx0IHRvIHVz
ZSBjb3JyZWN0bHkuICBUaGUgZmFjdCB0aGF0IHlvdSB1c2UgaXQgY29ycmVjdGx5IGRvZXMgbm90
IGp1c3RpZnkNCj4gdGhlIEFQSS4NCj4gDQo+ID4gPiBiZWNhdXNlIHlvdSBjYW4ndCBjaGVjayBv
cg0KPiA+ID4gZW5mb3JjZSB0aGF0IGFuIGFyYml0cmFyeSBjYWxsZXIgaXMgdXNpbmcgaXQgY29y
cmVjdGx5Lg0KPiA+DQo+ID4gSSBhbSBub3Qgc3VyZSB3aGF0IGlzIGFyYml0cmFyeSBjYWxsZXI/
IHBkZXYgaXMga25vd24gdG8gdmZpbywgc28gdmZpbw0KPiA+IHdpbGwgb25seSBtYWtlIHBjaV9l
bmFibGVfbXNpeCgpL3BjaV9lbmFibGVfbXNpX2Jsb2NrKCkgZm9yIHRoaXMgcGRldi4NCj4gPiBJ
ZiBhbnlvdGhlciBjb2RlIG1ha2VzIHRoZW4gaXQgaXMgc29tZSBvdGhlciB1bmV4cGVjdGVkbHkg
dGhpbmcNCj4gPiBoYXBwZW5pbmcgaW4gc3lzdGVtLCBubz8NCj4gDQo+IFdoYXQncyBwcm9wb3Nl
ZCBoZXJlIGlzIGEgZ2VuZXJpYyBJT01NVSBBUEkuICBBbnlib2R5IGNhbiBjYWxsIHRoaXMuDQo+
IFdoYXQgaWYgdGhlIGhvc3QgU0NTSSBkcml2ZXIgZGVjaWRlcyB0byBnbyBnZXQgdGhlIGlvbW11
IGRvbWFpbiBmb3IgaXQncyBkZXZpY2UNCj4gKG9yIGFueSBvdGhlciBkZXZpY2UpPyAgRG9lcyB0
aGF0IGZpdCB5b3VyIHVzYWdlIG1vZGVsPw0KPiANCj4gPiA+ICBJdCdzIG5vdCBtYWludGFpbmFi
bGUuDQo+ID4gPiBUaGFua3MsDQo+ID4NCj4gPiBJIGRvIG5vdCBoYXZlIGFueSBpc3N1ZSB3aXRo
IHRoaXMgYXMgd2VsbCwgY2FuIHlvdSBhbHNvIGRlc2NyaWJlIHRoZQ0KPiA+IHR5cGUgb2YgQVBJ
IHlvdSBhcmUgZW52aXNpb25pbmc7IEkgY2FuIHRoaW5rIG9mIGRlZmluaW5nIHNvbWUgZnVuY3Rp
b24NCj4gPiBpbiB2ZmlvLmMvdmZpb19pb21tdSouYywgbWFrZSB0aGVtIGdsb2JhbCBhbmQgZGVj
bGFyZSB0aGVuIGluDQo+ID4gaW5jbHVkZS9MaW51eC92ZmlvLmggQW5kIGluY2x1ZGUgPExpbnV4
L3ZmaW8uaD4gaW4gY2FsbGVyIGZpbGUNCj4gPiAoYXJjaC9wb3dlcnBjL2tlcm5lbC9tc2kuYykN
Cj4gDQo+IERvIHlvdSByZWFsbHkgd2FudCBtb2R1bGUgZGVwZW5kZW5jaWVzIGJldHdlZW4gdmZp
byBhbmQgeW91ciBjb3JlIGtlcm5lbCBNU0kNCj4gc2V0dXA/ICBMb29rIGF0IHRoZSB2ZmlvIGV4
dGVybmFsIHVzZXIgaW50ZXJmYWNlIHRoYXQgd2UndmUgYWxyZWFkeSBkZWZpbmVkLg0KPiBUaGF0
IGFsbG93cyBvdGhlciBjb21wb25lbnRzIG9mIHRoZSBrZXJuZWwgdG8gZ2V0IGEgcHJvcGVyIHJl
ZmVyZW5jZSB0byBhIHZmaW8NCj4gZ3JvdXAuICBGcm9tIHRoZXJlIHlvdSBjYW4gd29yayBvdXQg
aG93IHRvIGdldCB3aGF0IHlvdSB3YW50LiAgQW5vdGhlcg0KPiBhbHRlcm5hdGl2ZSBpcyB0aGF0
IHZmaW8gY291bGQgcmVnaXN0ZXIgYW4gTVNJIHRvIElPVkEgbWFwcGluZyB3aXRoIGFyY2hpdGVj
dHVyZQ0KPiBjb2RlIHdoZW4gdGhlIG1hcHBpbmcgaXMgY3JlYXRlZC4gIFRoZSBNU0kgc2V0dXAg
cGF0aCBjb3VsZCB0aGVuIGRvIGEgbG9va3VwIGluDQo+IGFyY2hpdGVjdHVyZSBjb2RlIGZvciB0
aGUgbWFwcGluZy4gIFlvdSBjb3VsZCBldmVuIHN0b3JlIHRoZSBNU0kgdG8gSU9WQSBtYXBwaW5n
DQo+IGluIFZGSU8gYW5kIGNyZWF0ZSBhbiBpbnRlcmZhY2Ugd2hlcmUgU0VUX0lSUSBwYXNzZXMg
dGhhdCBtYXBwaW5nIGludG8gc2V0dXANCj4gY29kZS4NCg0KT2ssIFdoYXQgSSB3YW50IGlzIHRv
IGdldCBJT1ZBIGFzc29jaWF0ZWQgd2l0aCBhIHBoeXNpY2FsIGFkZHJlc3MgKHBoeXNpY2FsIGFk
ZHJlc3Mgb2YgTVNJLWJhbmspLg0KQW5kIGN1cnJlbnRseSBJIGRvIG5vdCBzZWUgYSB3YXkgdG8g
a25vdyBJT1ZBIG9mIGEgcGh5c2ljYWwgYWRkcmVzcyBhbmQgZG9pbmcgYWxsIHRoaXMgZG9tYWlu
IGdldCBhbmQgdGhlbiBzZWFyY2ggdGhyb3VnaCBhbGwgb2YgaW9tbXUtd2luZG93cyBvZiB0aGF0
IGRvbWFpbi4NCg0KV2hhdCBpZiB3ZSBhZGQgYW4gaW9tbXUtQVBJIHdoaWNoIGNhbiByZXR1cm4g
dGhlIElPVkEgbWFwcGluZyBvZiBhIHBoeXNpY2FsIGFkZHJlc3MuIEN1cnJlbnQgdXNlIGNhc2Ug
aXMgc2V0dGluZyB1cCBNU0kncyBmb3IgYXBlcnR1cmUgdHlwZSBvZiBJT01NVSBhbHNvIGdldHRp
bmcgYSBwaHlzX3RvX2lvdmEoKSBtYXBwaW5nIGlzIGluZGVwZW5kZW50IG9mIFZGSU8sIHlvdXIg
dGhvdWdodD8NCg0KVGhhbmtzDQotQmhhcmF0DQoNCj4gVGhhbmtzLA0KPiANCj4gQWxleA0KPiAN
Cg0K

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

Alex Williamson Oct. 8, 2013, 3:13 a.m. UTC | #10

On Mon, 2013-10-07 at 05:46 +0000, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Friday, October 04, 2013 11:42 PM
> > To: Bhushan Bharat-R65777
> > Cc: joro@8bytes.org; benh@kernel.crashing.org; galak@kernel.crashing.org; linux-
> > kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > pci@vger.kernel.org; agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > foundation.org
> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> > 
> > On Fri, 2013-10-04 at 17:23 +0000, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > Sent: Friday, October 04, 2013 10:43 PM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org
> > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > device
> > > >
> > > > On Fri, 2013-10-04 at 16:47 +0000, Bhushan Bharat-R65777 wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > > Sent: Friday, October 04, 2013 9:15 PM
> > > > > > To: Bhushan Bharat-R65777
> > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > foundation.org
> > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > > > device
> > > > > >
> > > > > > On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777 wrote:
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: linux-pci-owner@vger.kernel.org
> > > > > > > > [mailto:linux-pci-owner@vger.kernel.org]
> > > > > > > > On Behalf Of Alex Williamson
> > > > > > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > > > > > To: Bhushan Bharat-R65777
> > > > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > > > foundation.org; Bhushan Bharat-R65777
> > > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain
> > > > > > > > of a device
> > > > > > > >
> > > > > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > > > > > > This api return the iommu domain to which the device is attached.
> > > > > > > > > The iommu_domain is required for making API calls related to
> > iommu.
> > > > > > > > > Follow up patches which use this API to know iommu maping.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Bharat Bhushan
> > > > > > > > > <bharat.bhushan@freescale.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/iommu/iommu.c |   10 ++++++++++
> > > > > > > > >  include/linux/iommu.h |    7 +++++++
> > > > > > > > >  2 files changed, 17 insertions(+), 0 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > > > > > index
> > > > > > > > > fbe9ca7..6ac5f50 100644
> > > > > > > > > --- a/drivers/iommu/iommu.c
> > > > > > > > > +++ b/drivers/iommu/iommu.c
> > > > > > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct
> > > > > > > > > iommu_domain *domain, struct device *dev)  }
> > > > > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > > > > > >
> > > > > > > > > +struct iommu_domain *iommu_get_dev_domain(struct device *dev) {
> > > > > > > > > +	struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > > > > > > +
> > > > > > > > > +	if (unlikely(ops == NULL || ops->get_dev_iommu_domain ==
> > NULL))
> > > > > > > > > +		return NULL;
> > > > > > > > > +
> > > > > > > > > +	return ops->get_dev_iommu_domain(dev); }
> > > > > > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > > > > > > >
> > > > > > > > What prevents this from racing iommu_domain_free()?  There's
> > > > > > > > no references acquired, so there's no reason for the caller
> > > > > > > > to assume the
> > > > > > pointer is valid.
> > > > > > >
> > > > > > > Sorry for late query, somehow this email went into a folder
> > > > > > > and escaped;
> > > > > > >
> > > > > > > Just to be sure, there is not lock at generic "struct
> > > > > > > iommu_domain", but IP
> > > > > > specific structure (link FSL domain) linked in
> > > > > > iommu_domain->priv have a lock, so we need to ensure this race
> > > > > > in FSL iommu code (say drivers/iommu/fsl_pamu_domain.c), right?
> > > > > >
> > > > > > No, it's not sufficient to make sure that your use of the
> > > > > > interface is race free.  The interface itself needs to be
> > > > > > designed so that it's difficult to use incorrectly.
> > > > >
> > > > > So we can define iommu_get_dev_domain()/iommu_put_dev_domain();
> > > > > iommu_get_dev_domain() will return domain with the lock held, and
> > > > > iommu_put_dev_domain() will release the lock? And
> > > > > iommu_get_dev_domain() must always be followed by
> > > > > iommu_get_dev_domain().
> > > >
> > > > What lock?  get/put are generally used for reference counting, not
> > > > locking in the kernel.
> > > >
> > > > > > That's not the case here.  This is a backdoor to get the iommu
> > > > > > domain from the iommu driver regardless of who is using it or how.
> > > > > > The iommu domain is created and managed by vfio, so shouldn't we
> > > > > > be looking at how to do this through vfio?
> > > > >
> > > > > Let me first describe what we are doing here:
> > > > > During initialization:-
> > > > >  - vfio talks to MSI system to know the MSI-page and size
> > > > >  - vfio then interacts with iommu to map the MSI-page in iommu
> > > > > (IOVA is decided by userspace and physical address is the
> > > > > MSI-page)
> > > > >  - So the IOVA subwindow mapping is created in iommu and yes VFIO
> > > > > know about
> > > > this mapping.
> > > > >
> > > > > Now do SET_IRQ(MSI/MSIX) ioctl:
> > > > >  - calls pci_enable_msix()/pci_enable_msi_block(): which is
> > > > > supposed to set
> > > > MSI address/data in device.
> > > > >  - So in current implementation (this patchset) msi-subsystem gets
> > > > > the IOVA
> > > > from iommu via this defined interface.
> > > > >  - Are you saying that rather than getting this from iommu, we
> > > > > should get this
> > > > from vfio? What difference does this make?
> > > >
> > > > Yes, you just said above that vfio knows the msi to iova mapping, so
> > > > why go outside of vfio to find it later?  The difference is one case
> > > > you can have a proper reference to data structures to make sure the
> > > > pointer you get back actually has meaning at the time you're using
> > > > it vs the code here where you're defining an API that returns a
> > > > meaningless value
> > >
> > > With FSL-PAMU we will always get consistant data from iommu or vfio-data
> > structure.
> > 
> > Great, but you're trying to add a generic API to the IOMMU subsystem that's
> > difficult to use correctly.  The fact that you use it correctly does not justify
> > the API.
> > 
> > > > because you can't check or
> > > > enforce that an arbitrary caller is using it correctly.
> > >
> > > I am not sure what is arbitrary caller? pdev is known to vfio, so vfio
> > > will only make pci_enable_msix()/pci_enable_msi_block() for this pdev.
> > > If anyother code makes then it is some other unexpectedly thing
> > > happening in system, no?
> > 
> > What's proposed here is a generic IOMMU API.  Anybody can call this.
> > What if the host SCSI driver decides to go get the iommu domain for it's device
> > (or any other device)?  Does that fit your usage model?
> > 
> > > >  It's not maintainable.
> > > > Thanks,
> > >
> > > I do not have any issue with this as well, can you also describe the
> > > type of API you are envisioning; I can think of defining some function
> > > in vfio.c/vfio_iommu*.c, make them global and declare then in
> > > include/Linux/vfio.h And include <Linux/vfio.h> in caller file
> > > (arch/powerpc/kernel/msi.c)
> > 
> > Do you really want module dependencies between vfio and your core kernel MSI
> > setup?  Look at the vfio external user interface that we've already defined.
> > That allows other components of the kernel to get a proper reference to a vfio
> > group.  From there you can work out how to get what you want.  Another
> > alternative is that vfio could register an MSI to IOVA mapping with architecture
> > code when the mapping is created.  The MSI setup path could then do a lookup in
> > architecture code for the mapping.  You could even store the MSI to IOVA mapping
> > in VFIO and create an interface where SET_IRQ passes that mapping into setup
> > code.
> 
> Ok, What I want is to get IOVA associated with a physical address
> (physical address of MSI-bank).
> And currently I do not see a way to know IOVA of a physical address
> and doing all this domain get and then search through all of
> iommu-windows of that domain.
> 
> What if we add an iommu-API which can return the IOVA mapping of a
> physical address. Current use case is setting up MSI's for aperture
> type of IOMMU also getting a phys_to_iova() mapping is independent of
> VFIO, your thought?

A physical address can be mapped to multiple IOVAs, so the interface
seems flawed by design.  It also has the same problem as above, it's a
backdoor that can be called asynchronous to the owner of the domain, so
what reason is there to believe the result?  It just replaces an
iommu_domain pointer with an IOVA.  VFIO knows this mapping, so why are
we trying to go behind its back and ask the IOMMU?  Thanks,

Alex

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

Bharat Bhushan Oct. 8, 2013, 3:42 a.m. UTC | #11

PiA+ID4gRG8geW91IHJlYWxseSB3YW50IG1vZHVsZSBkZXBlbmRlbmNpZXMgYmV0d2VlbiB2Zmlv
IGFuZCB5b3VyIGNvcmUNCj4gPiA+IGtlcm5lbCBNU0kgc2V0dXA/ICBMb29rIGF0IHRoZSB2Zmlv
IGV4dGVybmFsIHVzZXIgaW50ZXJmYWNlIHRoYXQgd2UndmUNCj4gYWxyZWFkeSBkZWZpbmVkLg0K
PiA+ID4gVGhhdCBhbGxvd3Mgb3RoZXIgY29tcG9uZW50cyBvZiB0aGUga2VybmVsIHRvIGdldCBh
IHByb3BlciByZWZlcmVuY2UNCj4gPiA+IHRvIGEgdmZpbyBncm91cC4gIEZyb20gdGhlcmUgeW91
IGNhbiB3b3JrIG91dCBob3cgdG8gZ2V0IHdoYXQgeW91DQo+ID4gPiB3YW50LiAgQW5vdGhlciBh
bHRlcm5hdGl2ZSBpcyB0aGF0IHZmaW8gY291bGQgcmVnaXN0ZXIgYW4gTVNJIHRvDQo+ID4gPiBJ
T1ZBIG1hcHBpbmcgd2l0aCBhcmNoaXRlY3R1cmUgY29kZSB3aGVuIHRoZSBtYXBwaW5nIGlzIGNy
ZWF0ZWQuDQo+ID4gPiBUaGUgTVNJIHNldHVwIHBhdGggY291bGQgdGhlbiBkbyBhIGxvb2t1cCBp
biBhcmNoaXRlY3R1cmUgY29kZSBmb3INCj4gPiA+IHRoZSBtYXBwaW5nLiAgWW91IGNvdWxkIGV2
ZW4gc3RvcmUgdGhlIE1TSSB0byBJT1ZBIG1hcHBpbmcgaW4gVkZJTw0KPiA+ID4gYW5kIGNyZWF0
ZSBhbiBpbnRlcmZhY2Ugd2hlcmUgU0VUX0lSUSBwYXNzZXMgdGhhdCBtYXBwaW5nIGludG8gc2V0
dXAgY29kZS4NCj4gPg0KPiA+IE9rLCBXaGF0IEkgd2FudCBpcyB0byBnZXQgSU9WQSBhc3NvY2lh
dGVkIHdpdGggYSBwaHlzaWNhbCBhZGRyZXNzDQo+ID4gKHBoeXNpY2FsIGFkZHJlc3Mgb2YgTVNJ
LWJhbmspLg0KPiA+IEFuZCBjdXJyZW50bHkgSSBkbyBub3Qgc2VlIGEgd2F5IHRvIGtub3cgSU9W
QSBvZiBhIHBoeXNpY2FsIGFkZHJlc3MNCj4gPiBhbmQgZG9pbmcgYWxsIHRoaXMgZG9tYWluIGdl
dCBhbmQgdGhlbiBzZWFyY2ggdGhyb3VnaCBhbGwgb2YNCj4gPiBpb21tdS13aW5kb3dzIG9mIHRo
YXQgZG9tYWluLg0KPiA+DQo+ID4gV2hhdCBpZiB3ZSBhZGQgYW4gaW9tbXUtQVBJIHdoaWNoIGNh
biByZXR1cm4gdGhlIElPVkEgbWFwcGluZyBvZiBhDQo+ID4gcGh5c2ljYWwgYWRkcmVzcy4gQ3Vy
cmVudCB1c2UgY2FzZSBpcyBzZXR0aW5nIHVwIE1TSSdzIGZvciBhcGVydHVyZQ0KPiA+IHR5cGUg
b2YgSU9NTVUgYWxzbyBnZXR0aW5nIGEgcGh5c190b19pb3ZhKCkgbWFwcGluZyBpcyBpbmRlcGVu
ZGVudCBvZg0KPiA+IFZGSU8sIHlvdXIgdGhvdWdodD8NCj4gDQo+IEEgcGh5c2ljYWwgYWRkcmVz
cyBjYW4gYmUgbWFwcGVkIHRvIG11bHRpcGxlIElPVkFzLCBzbyB0aGUgaW50ZXJmYWNlIHNlZW1z
DQo+IGZsYXdlZCBieSBkZXNpZ24uICBJdCBhbHNvIGhhcyB0aGUgc2FtZSBwcm9ibGVtIGFzIGFi
b3ZlLCBpdCdzIGEgYmFja2Rvb3IgdGhhdA0KPiBjYW4gYmUgY2FsbGVkIGFzeW5jaHJvbm91cyB0
byB0aGUgb3duZXIgb2YgdGhlIGRvbWFpbiwgc28gd2hhdCByZWFzb24gaXMgdGhlcmUNCj4gdG8g
YmVsaWV2ZSB0aGUgcmVzdWx0PyAgSXQganVzdCByZXBsYWNlcyBhbiBpb21tdV9kb21haW4gcG9p
bnRlciB3aXRoIGFuIElPVkEuDQo+IFZGSU8ga25vd3MgdGhpcyBtYXBwaW5nLCBzbyB3aHkgYXJl
IHdlIHRyeWluZyB0byBnbyBiZWhpbmQgaXRzIGJhY2sgYW5kIGFzayB0aGUNCj4gSU9NTVU/DQpJ
T01NVSBpcyB0aGUgZmluYWwgcGxhY2Ugd2hlcmUgbWFwcGluZyBpcyBjcmVhdGVkLCBzbyBtYXkg
YmUgdG9kYXkgaXQgaXMgY2FsbGluZyBvbiBiZWhhbGYgb2YgVkZJTywgdG9tb3Jyb3cgaXQgY2Fu
IGJlIGZvciBub3JtYWwgTGludXggb3Igc29tZSBvdGhlciBpbnRlcmZhY2UuIEJ1dCBJIGFtIGZp
bmUgdG8gZGlyZWN0bHkgdGFsayB0byB2ZmlvIGFuZCB3aWxsIG5vdCB0cnkgdG8gc29sdmUgYSBw
cm9ibGVtIHdoaWNoIGRvZXMgbm90IGV4aXN0cyB0b2RheS4NCg0KTVNJIHN1YnN5c3RlbSBrbm93
cyBwZGV2IChwY2kgZGV2aWNlKSBhbmQgcGh5c2ljYWwgYWRkcmVzcywgdGhlbiB3aGF0IGludGVy
ZmFjZSBpdCB3aWxsIHVzZSB0byBnZXQgdGhlIElPVkEgZnJvbSBWRklPPw0KDQpUaGFua3MNCi1C
aGFyYXQNCg0KPiAgVGhhbmtzLA0KPiANCj4gQWxleA0KPiANCg0K

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

Sethi Varun-B16395 Oct. 10, 2013, 8:09 p.m. UTC | #12

> -----Original Message-----
> From: iommu-bounces@lists.linux-foundation.org [mailto:iommu-
> bounces@lists.linux-foundation.org] On Behalf Of Alex Williamson
> Sent: Tuesday, October 08, 2013 8:43 AM
> To: Bhushan Bharat-R65777
> Cc: agraf@suse.de; Wood Scott-B07421; linux-pci@vger.kernel.org;
> galak@kernel.crashing.org; linux-kernel@vger.kernel.org;
> iommu@lists.linux-foundation.org; benh@kernel.crashing.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> 
> On Mon, 2013-10-07 at 05:46 +0000, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Friday, October 04, 2013 11:42 PM
> > > To: Bhushan Bharat-R65777
> > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org
> > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > device
> > >
> > > On Fri, 2013-10-04 at 17:23 +0000, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > Sent: Friday, October 04, 2013 10:43 PM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > foundation.org
> > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > > device
> > > > >
> > > > > On Fri, 2013-10-04 at 16:47 +0000, Bhushan Bharat-R65777 wrote:
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > > > Sent: Friday, October 04, 2013 9:15 PM
> > > > > > > To: Bhushan Bharat-R65777
> > > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > > foundation.org
> > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain
> > > > > > > of a device
> > > > > > >
> > > > > > > On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777
> wrote:
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: linux-pci-owner@vger.kernel.org
> > > > > > > > > [mailto:linux-pci-owner@vger.kernel.org]
> > > > > > > > > On Behalf Of Alex Williamson
> > > > > > > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > > > > > > To: Bhushan Bharat-R65777
> > > > > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > > > > galak@kernel.crashing.org; linux-
> > > > > > > > > kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > > > > > > linux- pci@vger.kernel.org; agraf@suse.de; Wood
> > > > > > > > > Scott-B07421; iommu@lists.linux- foundation.org; Bhushan
> > > > > > > > > Bharat-R65777
> > > > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get
> > > > > > > > > iommu_domain of a device
> > > > > > > > >
> > > > > > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > > > > > > > This api return the iommu domain to which the device is
> attached.
> > > > > > > > > > The iommu_domain is required for making API calls
> > > > > > > > > > related to
> > > iommu.
> > > > > > > > > > Follow up patches which use this API to know iommu
> maping.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Bharat Bhushan
> > > > > > > > > > <bharat.bhushan@freescale.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/iommu/iommu.c |   10 ++++++++++
> > > > > > > > > >  include/linux/iommu.h |    7 +++++++
> > > > > > > > > >  2 files changed, 17 insertions(+), 0 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/iommu/iommu.c
> > > > > > > > > > b/drivers/iommu/iommu.c index
> > > > > > > > > > fbe9ca7..6ac5f50 100644
> > > > > > > > > > --- a/drivers/iommu/iommu.c
> > > > > > > > > > +++ b/drivers/iommu/iommu.c
> > > > > > > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct
> > > > > > > > > > iommu_domain *domain, struct device *dev)  }
> > > > > > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > > > > > > >
> > > > > > > > > > +struct iommu_domain *iommu_get_dev_domain(struct
> device *dev) {
> > > > > > > > > > +	struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > > > > > > > +
> > > > > > > > > > +	if (unlikely(ops == NULL ||
> > > > > > > > > > +ops->get_dev_iommu_domain ==
> > > NULL))
> > > > > > > > > > +		return NULL;
> > > > > > > > > > +
> > > > > > > > > > +	return ops->get_dev_iommu_domain(dev); }
> > > > > > > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > > > > > > > >
> > > > > > > > > What prevents this from racing iommu_domain_free()?
> > > > > > > > > There's no references acquired, so there's no reason for
> > > > > > > > > the caller to assume the
> > > > > > > pointer is valid.
> > > > > > > >
> > > > > > > > Sorry for late query, somehow this email went into a
> > > > > > > > folder and escaped;
> > > > > > > >
> > > > > > > > Just to be sure, there is not lock at generic "struct
> > > > > > > > iommu_domain", but IP
> > > > > > > specific structure (link FSL domain) linked in
> > > > > > > iommu_domain->priv have a lock, so we need to ensure this
> > > > > > > race in FSL iommu code (say drivers/iommu/fsl_pamu_domain.c),
> right?
> > > > > > >
> > > > > > > No, it's not sufficient to make sure that your use of the
> > > > > > > interface is race free.  The interface itself needs to be
> > > > > > > designed so that it's difficult to use incorrectly.
> > > > > >
> > > > > > So we can define
> > > > > > iommu_get_dev_domain()/iommu_put_dev_domain();
> > > > > > iommu_get_dev_domain() will return domain with the lock held,
> > > > > > and
> > > > > > iommu_put_dev_domain() will release the lock? And
> > > > > > iommu_get_dev_domain() must always be followed by
> > > > > > iommu_get_dev_domain().
> > > > >
> > > > > What lock?  get/put are generally used for reference counting,
> > > > > not locking in the kernel.
> > > > >
> > > > > > > That's not the case here.  This is a backdoor to get the
> > > > > > > iommu domain from the iommu driver regardless of who is using
> it or how.
> > > > > > > The iommu domain is created and managed by vfio, so
> > > > > > > shouldn't we be looking at how to do this through vfio?
> > > > > >
> > > > > > Let me first describe what we are doing here:
> > > > > > During initialization:-
> > > > > >  - vfio talks to MSI system to know the MSI-page and size
> > > > > >  - vfio then interacts with iommu to map the MSI-page in iommu
> > > > > > (IOVA is decided by userspace and physical address is the
> > > > > > MSI-page)
> > > > > >  - So the IOVA subwindow mapping is created in iommu and yes
> > > > > > VFIO know about
> > > > > this mapping.
> > > > > >
> > > > > > Now do SET_IRQ(MSI/MSIX) ioctl:
> > > > > >  - calls pci_enable_msix()/pci_enable_msi_block(): which is
> > > > > > supposed to set
> > > > > MSI address/data in device.
> > > > > >  - So in current implementation (this patchset) msi-subsystem
> > > > > > gets the IOVA
> > > > > from iommu via this defined interface.
> > > > > >  - Are you saying that rather than getting this from iommu, we
> > > > > > should get this
> > > > > from vfio? What difference does this make?
> > > > >
> > > > > Yes, you just said above that vfio knows the msi to iova
> > > > > mapping, so why go outside of vfio to find it later?  The
> > > > > difference is one case you can have a proper reference to data
> > > > > structures to make sure the pointer you get back actually has
> > > > > meaning at the time you're using it vs the code here where
> > > > > you're defining an API that returns a meaningless value
> > > >
> > > > With FSL-PAMU we will always get consistant data from iommu or
> > > > vfio-data
> > > structure.
> > >
> > > Great, but you're trying to add a generic API to the IOMMU subsystem
> > > that's difficult to use correctly.  The fact that you use it
> > > correctly does not justify the API.
> > >
> > > > > because you can't check or
> > > > > enforce that an arbitrary caller is using it correctly.
> > > >
> > > > I am not sure what is arbitrary caller? pdev is known to vfio, so
> > > > vfio will only make pci_enable_msix()/pci_enable_msi_block() for
> this pdev.
> > > > If anyother code makes then it is some other unexpectedly thing
> > > > happening in system, no?
> > >
> > > What's proposed here is a generic IOMMU API.  Anybody can call this.
> > > What if the host SCSI driver decides to go get the iommu domain for
> > > it's device (or any other device)?  Does that fit your usage model?
> > >
> > > > >  It's not maintainable.
> > > > > Thanks,
> > > >
> > > > I do not have any issue with this as well, can you also describe
> > > > the type of API you are envisioning; I can think of defining some
> > > > function in vfio.c/vfio_iommu*.c, make them global and declare
> > > > then in include/Linux/vfio.h And include <Linux/vfio.h> in caller
> > > > file
> > > > (arch/powerpc/kernel/msi.c)
> > >
> > > Do you really want module dependencies between vfio and your core
> > > kernel MSI setup?  Look at the vfio external user interface that
> we've already defined.
> > > That allows other components of the kernel to get a proper reference
> > > to a vfio group.  From there you can work out how to get what you
> > > want.  Another alternative is that vfio could register an MSI to
> > > IOVA mapping with architecture code when the mapping is created.
> > > The MSI setup path could then do a lookup in architecture code for
> > > the mapping.  You could even store the MSI to IOVA mapping in VFIO
> > > and create an interface where SET_IRQ passes that mapping into setup
> code.
[Sethi Varun-B16395] What you are suggesting is that the MSI setup path queries the vfio subsystem for the mapping, rather than directly querying the iommu subsystem. So, say if we add an interface for getting MSI to IOVA mapping in the msi setup path, wouldn't this again be specific to vfio? I reckon that this interface would again ppc machine specific interface.

-Varun

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

Alex Williamson Oct. 10, 2013, 8:41 p.m. UTC | #13

On Thu, 2013-10-10 at 20:09 +0000, Sethi Varun-B16395 wrote:
> 
> > -----Original Message-----
> > From: iommu-bounces@lists.linux-foundation.org [mailto:iommu-
> > bounces@lists.linux-foundation.org] On Behalf Of Alex Williamson
> > Sent: Tuesday, October 08, 2013 8:43 AM
> > To: Bhushan Bharat-R65777
> > Cc: agraf@suse.de; Wood Scott-B07421; linux-pci@vger.kernel.org;
> > galak@kernel.crashing.org; linux-kernel@vger.kernel.org;
> > iommu@lists.linux-foundation.org; benh@kernel.crashing.org; linuxppc-
> > dev@lists.ozlabs.org
> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> > 
> > On Mon, 2013-10-07 at 05:46 +0000, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > Sent: Friday, October 04, 2013 11:42 PM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org
> > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > device
> > > >
> > > > On Fri, 2013-10-04 at 17:23 +0000, Bhushan Bharat-R65777 wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > > Sent: Friday, October 04, 2013 10:43 PM
> > > > > > To: Bhushan Bharat-R65777
> > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > foundation.org
> > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > > > device
> > > > > >
> > > > > > On Fri, 2013-10-04 at 16:47 +0000, Bhushan Bharat-R65777 wrote:
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > > > > Sent: Friday, October 04, 2013 9:15 PM
> > > > > > > > To: Bhushan Bharat-R65777
> > > > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > > > foundation.org
> > > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain
> > > > > > > > of a device
> > > > > > > >
> > > > > > > > On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777
> > wrote:
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: linux-pci-owner@vger.kernel.org
> > > > > > > > > > [mailto:linux-pci-owner@vger.kernel.org]
> > > > > > > > > > On Behalf Of Alex Williamson
> > > > > > > > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > > > > > > > To: Bhushan Bharat-R65777
> > > > > > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > > > > > galak@kernel.crashing.org; linux-
> > > > > > > > > > kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > > > > > > > linux- pci@vger.kernel.org; agraf@suse.de; Wood
> > > > > > > > > > Scott-B07421; iommu@lists.linux- foundation.org; Bhushan
> > > > > > > > > > Bharat-R65777
> > > > > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get
> > > > > > > > > > iommu_domain of a device
> > > > > > > > > >
> > > > > > > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > > > > > > > > This api return the iommu domain to which the device is
> > attached.
> > > > > > > > > > > The iommu_domain is required for making API calls
> > > > > > > > > > > related to
> > > > iommu.
> > > > > > > > > > > Follow up patches which use this API to know iommu
> > maping.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Bharat Bhushan
> > > > > > > > > > > <bharat.bhushan@freescale.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/iommu/iommu.c |   10 ++++++++++
> > > > > > > > > > >  include/linux/iommu.h |    7 +++++++
> > > > > > > > > > >  2 files changed, 17 insertions(+), 0 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/iommu/iommu.c
> > > > > > > > > > > b/drivers/iommu/iommu.c index
> > > > > > > > > > > fbe9ca7..6ac5f50 100644
> > > > > > > > > > > --- a/drivers/iommu/iommu.c
> > > > > > > > > > > +++ b/drivers/iommu/iommu.c
> > > > > > > > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct
> > > > > > > > > > > iommu_domain *domain, struct device *dev)  }
> > > > > > > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > > > > > > > >
> > > > > > > > > > > +struct iommu_domain *iommu_get_dev_domain(struct
> > device *dev) {
> > > > > > > > > > > +	struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > > > > > > > > +
> > > > > > > > > > > +	if (unlikely(ops == NULL ||
> > > > > > > > > > > +ops->get_dev_iommu_domain ==
> > > > NULL))
> > > > > > > > > > > +		return NULL;
> > > > > > > > > > > +
> > > > > > > > > > > +	return ops->get_dev_iommu_domain(dev); }
> > > > > > > > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > > > > > > > > >
> > > > > > > > > > What prevents this from racing iommu_domain_free()?
> > > > > > > > > > There's no references acquired, so there's no reason for
> > > > > > > > > > the caller to assume the
> > > > > > > > pointer is valid.
> > > > > > > > >
> > > > > > > > > Sorry for late query, somehow this email went into a
> > > > > > > > > folder and escaped;
> > > > > > > > >
> > > > > > > > > Just to be sure, there is not lock at generic "struct
> > > > > > > > > iommu_domain", but IP
> > > > > > > > specific structure (link FSL domain) linked in
> > > > > > > > iommu_domain->priv have a lock, so we need to ensure this
> > > > > > > > race in FSL iommu code (say drivers/iommu/fsl_pamu_domain.c),
> > right?
> > > > > > > >
> > > > > > > > No, it's not sufficient to make sure that your use of the
> > > > > > > > interface is race free.  The interface itself needs to be
> > > > > > > > designed so that it's difficult to use incorrectly.
> > > > > > >
> > > > > > > So we can define
> > > > > > > iommu_get_dev_domain()/iommu_put_dev_domain();
> > > > > > > iommu_get_dev_domain() will return domain with the lock held,
> > > > > > > and
> > > > > > > iommu_put_dev_domain() will release the lock? And
> > > > > > > iommu_get_dev_domain() must always be followed by
> > > > > > > iommu_get_dev_domain().
> > > > > >
> > > > > > What lock?  get/put are generally used for reference counting,
> > > > > > not locking in the kernel.
> > > > > >
> > > > > > > > That's not the case here.  This is a backdoor to get the
> > > > > > > > iommu domain from the iommu driver regardless of who is using
> > it or how.
> > > > > > > > The iommu domain is created and managed by vfio, so
> > > > > > > > shouldn't we be looking at how to do this through vfio?
> > > > > > >
> > > > > > > Let me first describe what we are doing here:
> > > > > > > During initialization:-
> > > > > > >  - vfio talks to MSI system to know the MSI-page and size
> > > > > > >  - vfio then interacts with iommu to map the MSI-page in iommu
> > > > > > > (IOVA is decided by userspace and physical address is the
> > > > > > > MSI-page)
> > > > > > >  - So the IOVA subwindow mapping is created in iommu and yes
> > > > > > > VFIO know about
> > > > > > this mapping.
> > > > > > >
> > > > > > > Now do SET_IRQ(MSI/MSIX) ioctl:
> > > > > > >  - calls pci_enable_msix()/pci_enable_msi_block(): which is
> > > > > > > supposed to set
> > > > > > MSI address/data in device.
> > > > > > >  - So in current implementation (this patchset) msi-subsystem
> > > > > > > gets the IOVA
> > > > > > from iommu via this defined interface.
> > > > > > >  - Are you saying that rather than getting this from iommu, we
> > > > > > > should get this
> > > > > > from vfio? What difference does this make?
> > > > > >
> > > > > > Yes, you just said above that vfio knows the msi to iova
> > > > > > mapping, so why go outside of vfio to find it later?  The
> > > > > > difference is one case you can have a proper reference to data
> > > > > > structures to make sure the pointer you get back actually has
> > > > > > meaning at the time you're using it vs the code here where
> > > > > > you're defining an API that returns a meaningless value
> > > > >
> > > > > With FSL-PAMU we will always get consistant data from iommu or
> > > > > vfio-data
> > > > structure.
> > > >
> > > > Great, but you're trying to add a generic API to the IOMMU subsystem
> > > > that's difficult to use correctly.  The fact that you use it
> > > > correctly does not justify the API.
> > > >
> > > > > > because you can't check or
> > > > > > enforce that an arbitrary caller is using it correctly.
> > > > >
> > > > > I am not sure what is arbitrary caller? pdev is known to vfio, so
> > > > > vfio will only make pci_enable_msix()/pci_enable_msi_block() for
> > this pdev.
> > > > > If anyother code makes then it is some other unexpectedly thing
> > > > > happening in system, no?
> > > >
> > > > What's proposed here is a generic IOMMU API.  Anybody can call this.
> > > > What if the host SCSI driver decides to go get the iommu domain for
> > > > it's device (or any other device)?  Does that fit your usage model?
> > > >
> > > > > >  It's not maintainable.
> > > > > > Thanks,
> > > > >
> > > > > I do not have any issue with this as well, can you also describe
> > > > > the type of API you are envisioning; I can think of defining some
> > > > > function in vfio.c/vfio_iommu*.c, make them global and declare
> > > > > then in include/Linux/vfio.h And include <Linux/vfio.h> in caller
> > > > > file
> > > > > (arch/powerpc/kernel/msi.c)
> > > >
> > > > Do you really want module dependencies between vfio and your core
> > > > kernel MSI setup?  Look at the vfio external user interface that
> > we've already defined.
> > > > That allows other components of the kernel to get a proper reference
> > > > to a vfio group.  From there you can work out how to get what you
> > > > want.  Another alternative is that vfio could register an MSI to
> > > > IOVA mapping with architecture code when the mapping is created.
> > > > The MSI setup path could then do a lookup in architecture code for
> > > > the mapping.  You could even store the MSI to IOVA mapping in VFIO
> > > > and create an interface where SET_IRQ passes that mapping into setup
> > code.
> [Sethi Varun-B16395] What you are suggesting is that the MSI setup
> path queries the vfio subsystem for the mapping, rather than directly
> querying the iommu subsystem. So, say if we add an interface for
> getting MSI to IOVA mapping in the msi setup path, wouldn't this again
> be specific to vfio? I reckon that this interface would again ppc
> machine specific interface.

Sure, if this is a generic MSI setup path then clearly vfio should not
be involved.  But by that same notion, if this is a generic MSI setup
path, how can the proposed solutions guarantee that the iommu_domain or
iova returned is still valid in all cases?  It's racy.  If the caller
trying to setup MSI has the information needed, why doesn't it pass it
in as part of the setup?  Thanks,

Alex



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

Sethi Varun-B16395 Oct. 14, 2013, 12:58 p.m. UTC | #14

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQWxleCBXaWxsaWFtc29u
IFttYWlsdG86YWxleC53aWxsaWFtc29uQHJlZGhhdC5jb21dDQo+IFNlbnQ6IEZyaWRheSwgT2N0
b2JlciAxMSwgMjAxMyAyOjEyIEFNDQo+IFRvOiBTZXRoaSBWYXJ1bi1CMTYzOTUNCj4gQ2M6IEJo
dXNoYW4gQmhhcmF0LVI2NTc3NzsgYWdyYWZAc3VzZS5kZTsgV29vZCBTY290dC1CMDc0MjE7IGxp
bnV4LQ0KPiBwY2lAdmdlci5rZXJuZWwub3JnOyBnYWxha0BrZXJuZWwuY3Jhc2hpbmcub3JnOyBs
aW51eC0NCj4ga2VybmVsQHZnZXIua2VybmVsLm9yZzsgaW9tbXVAbGlzdHMubGludXgtZm91bmRh
dGlvbi5vcmc7DQo+IGJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZzsgbGludXhwcGMtZGV2QGxpc3Rz
Lm96bGFicy5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSCAyLzddIGlvbW11OiBhZGQgYXBpIHRv
IGdldCBpb21tdV9kb21haW4gb2YgYSBkZXZpY2UNCj4gDQo+IE9uIFRodSwgMjAxMy0xMC0xMCBh
dCAyMDowOSArMDAwMCwgU2V0aGkgVmFydW4tQjE2Mzk1IHdyb3RlOg0KPiA+DQo+ID4gPiAtLS0t
LU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+ID4gRnJvbTogaW9tbXUtYm91bmNlc0BsaXN0cy5s
aW51eC1mb3VuZGF0aW9uLm9yZyBbbWFpbHRvOmlvbW11LQ0KPiA+ID4gYm91bmNlc0BsaXN0cy5s
aW51eC1mb3VuZGF0aW9uLm9yZ10gT24gQmVoYWxmIE9mIEFsZXggV2lsbGlhbXNvbg0KPiA+ID4g
U2VudDogVHVlc2RheSwgT2N0b2JlciAwOCwgMjAxMyA4OjQzIEFNDQo+ID4gPiBUbzogQmh1c2hh
biBCaGFyYXQtUjY1Nzc3DQo+ID4gPiBDYzogYWdyYWZAc3VzZS5kZTsgV29vZCBTY290dC1CMDc0
MjE7IGxpbnV4LXBjaUB2Z2VyLmtlcm5lbC5vcmc7DQo+ID4gPiBnYWxha0BrZXJuZWwuY3Jhc2hp
bmcub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOw0KPiA+ID4gaW9tbXVAbGlzdHMu
bGludXgtZm91bmRhdGlvbi5vcmc7IGJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZzsNCj4gPiA+IGxp
bnV4cHBjLSBkZXZAbGlzdHMub3psYWJzLm9yZw0KPiA+ID4gU3ViamVjdDogUmU6IFtQQVRDSCAy
LzddIGlvbW11OiBhZGQgYXBpIHRvIGdldCBpb21tdV9kb21haW4gb2YgYQ0KPiA+ID4gZGV2aWNl
DQo+ID4gPg0KPiA+ID4gT24gTW9uLCAyMDEzLTEwLTA3IGF0IDA1OjQ2ICswMDAwLCBCaHVzaGFu
IEJoYXJhdC1SNjU3Nzcgd3JvdGU6DQo+ID4gPiA+DQo+ID4gPiA+ID4gLS0tLS1PcmlnaW5hbCBN
ZXNzYWdlLS0tLS0NCj4gPiA+ID4gPiBGcm9tOiBBbGV4IFdpbGxpYW1zb24gW21haWx0bzphbGV4
LndpbGxpYW1zb25AcmVkaGF0LmNvbV0NCj4gPiA+ID4gPiBTZW50OiBGcmlkYXksIE9jdG9iZXIg
MDQsIDIwMTMgMTE6NDIgUE0NCj4gPiA+ID4gPiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3DQo+
ID4gPiA+ID4gQ2M6IGpvcm9AOGJ5dGVzLm9yZzsgYmVuaEBrZXJuZWwuY3Jhc2hpbmcub3JnOw0K
PiA+ID4gPiA+IGdhbGFrQGtlcm5lbC5jcmFzaGluZy5vcmc7IGxpbnV4LSBrZXJuZWxAdmdlci5r
ZXJuZWwub3JnOw0KPiA+ID4gPiA+IGxpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMub3JnOyBsaW51
eC0gcGNpQHZnZXIua2VybmVsLm9yZzsNCj4gPiA+ID4gPiBhZ3JhZkBzdXNlLmRlOyBXb29kIFNj
b3R0LUIwNzQyMTsgaW9tbXVAbGlzdHMubGludXgtDQo+ID4gPiA+ID4gZm91bmRhdGlvbi5vcmcN
Cj4gPiA+ID4gPiBTdWJqZWN0OiBSZTogW1BBVENIIDIvN10gaW9tbXU6IGFkZCBhcGkgdG8gZ2V0
IGlvbW11X2RvbWFpbiBvZiBhDQo+ID4gPiA+ID4gZGV2aWNlDQo+ID4gPiA+ID4NCj4gPiA+ID4g
PiBPbiBGcmksIDIwMTMtMTAtMDQgYXQgMTc6MjMgKzAwMDAsIEJodXNoYW4gQmhhcmF0LVI2NTc3
NyB3cm90ZToNCj4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiA+IC0tLS0tT3JpZ2luYWwgTWVzc2Fn
ZS0tLS0tDQo+ID4gPiA+ID4gPiA+IEZyb206IEFsZXggV2lsbGlhbXNvbiBbbWFpbHRvOmFsZXgu
d2lsbGlhbXNvbkByZWRoYXQuY29tXQ0KPiA+ID4gPiA+ID4gPiBTZW50OiBGcmlkYXksIE9jdG9i
ZXIgMDQsIDIwMTMgMTA6NDMgUE0NCj4gPiA+ID4gPiA+ID4gVG86IEJodXNoYW4gQmhhcmF0LVI2
NTc3Nw0KPiA+ID4gPiA+ID4gPiBDYzogam9yb0A4Ynl0ZXMub3JnOyBiZW5oQGtlcm5lbC5jcmFz
aGluZy5vcmc7DQo+ID4gPiA+ID4gPiA+IGdhbGFrQGtlcm5lbC5jcmFzaGluZy5vcmc7IGxpbnV4
LSBrZXJuZWxAdmdlci5rZXJuZWwub3JnOw0KPiA+ID4gPiA+ID4gPiBsaW51eHBwYy1kZXZAbGlz
dHMub3psYWJzLm9yZzsgbGludXgtIHBjaUB2Z2VyLmtlcm5lbC5vcmc7DQo+ID4gPiA+ID4gPiA+
IGFncmFmQHN1c2UuZGU7IFdvb2QgU2NvdHQtQjA3NDIxOyBpb21tdUBsaXN0cy5saW51eC0NCj4g
PiA+ID4gPiA+ID4gZm91bmRhdGlvbi5vcmcNCj4gPiA+ID4gPiA+ID4gU3ViamVjdDogUmU6IFtQ
QVRDSCAyLzddIGlvbW11OiBhZGQgYXBpIHRvIGdldCBpb21tdV9kb21haW4NCj4gPiA+ID4gPiA+
ID4gb2YgYSBkZXZpY2UNCj4gPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gT24gRnJpLCAyMDEz
LTEwLTA0IGF0IDE2OjQ3ICswMDAwLCBCaHVzaGFuIEJoYXJhdC1SNjU3NzcNCj4gd3JvdGU6DQo+
ID4gPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gPiA+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0t
LS0tDQo+ID4gPiA+ID4gPiA+ID4gPiBGcm9tOiBBbGV4IFdpbGxpYW1zb24NCj4gPiA+ID4gPiA+
ID4gPiA+IFttYWlsdG86YWxleC53aWxsaWFtc29uQHJlZGhhdC5jb21dDQo+ID4gPiA+ID4gPiA+
ID4gPiBTZW50OiBGcmlkYXksIE9jdG9iZXIgMDQsIDIwMTMgOToxNSBQTQ0KPiA+ID4gPiA+ID4g
PiA+ID4gVG86IEJodXNoYW4gQmhhcmF0LVI2NTc3Nw0KPiA+ID4gPiA+ID4gPiA+ID4gQ2M6IGpv
cm9AOGJ5dGVzLm9yZzsgYmVuaEBrZXJuZWwuY3Jhc2hpbmcub3JnOw0KPiA+ID4gPiA+ID4gPiA+
ID4gZ2FsYWtAa2VybmVsLmNyYXNoaW5nLm9yZzsgbGludXgtDQo+ID4gPiA+ID4gPiA+ID4gPiBr
ZXJuZWxAdmdlci5rZXJuZWwub3JnOyBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZzsNCj4g
PiA+ID4gPiA+ID4gPiA+IGxpbnV4LSBwY2lAdmdlci5rZXJuZWwub3JnOyBhZ3JhZkBzdXNlLmRl
OyBXb29kDQo+ID4gPiA+ID4gPiA+ID4gPiBTY290dC1CMDc0MjE7IGlvbW11QGxpc3RzLmxpbnV4
LSBmb3VuZGF0aW9uLm9yZw0KPiA+ID4gPiA+ID4gPiA+ID4gU3ViamVjdDogUmU6IFtQQVRDSCAy
LzddIGlvbW11OiBhZGQgYXBpIHRvIGdldA0KPiA+ID4gPiA+ID4gPiA+ID4gaW9tbXVfZG9tYWlu
IG9mIGEgZGV2aWNlDQo+ID4gPiA+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiA+ID4gT24gRnJp
LCAyMDEzLTEwLTA0IGF0IDA5OjU0ICswMDAwLCBCaHVzaGFuIEJoYXJhdC1SNjU3NzcNCj4gPiA+
IHdyb3RlOg0KPiA+ID4gPiA+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+IC0tLS0t
T3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gRnJvbTogbGludXgt
cGNpLW93bmVyQHZnZXIua2VybmVsLm9yZw0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+IFttYWlsdG86
bGludXgtcGNpLW93bmVyQHZnZXIua2VybmVsLm9yZ10NCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiBP
biBCZWhhbGYgT2YgQWxleCBXaWxsaWFtc29uDQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gU2VudDog
V2VkbmVzZGF5LCBTZXB0ZW1iZXIgMjUsIDIwMTMgMTA6MTYgUE0NCj4gPiA+ID4gPiA+ID4gPiA+
ID4gPiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gQ2M6
IGpvcm9AOGJ5dGVzLm9yZzsgYmVuaEBrZXJuZWwuY3Jhc2hpbmcub3JnOw0KPiA+ID4gPiA+ID4g
PiA+ID4gPiA+IGdhbGFrQGtlcm5lbC5jcmFzaGluZy5vcmc7IGxpbnV4LQ0KPiA+ID4gPiA+ID4g
PiA+ID4gPiA+IGtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4g
bGludXhwcGMtZGV2QGxpc3RzLm96bGFicy5vcmc7DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gbGlu
dXgtIHBjaUB2Z2VyLmtlcm5lbC5vcmc7IGFncmFmQHN1c2UuZGU7IFdvb2QNCj4gPiA+ID4gPiA+
ID4gPiA+ID4gPiBTY290dC1CMDc0MjE7IGlvbW11QGxpc3RzLmxpbnV4LSBmb3VuZGF0aW9uLm9y
ZzsNCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiBCaHVzaGFuDQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4g
QmhhcmF0LVI2NTc3Nw0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+IFN1YmplY3Q6IFJlOiBbUEFUQ0gg
Mi83XSBpb21tdTogYWRkIGFwaSB0byBnZXQNCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiBpb21tdV9k
b21haW4gb2YgYSBkZXZpY2UNCj4gPiA+ID4gPiA+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiA+
ID4gPiA+IE9uIFRodSwgMjAxMy0wOS0xOSBhdCAxMjo1OSArMDUzMCwgQmhhcmF0IEJodXNoYW4N
Cj4gd3JvdGU6DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiBUaGlzIGFwaSByZXR1cm4gdGhlIGlv
bW11IGRvbWFpbiB0byB3aGljaCB0aGUNCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IGRldmljZSBp
cw0KPiA+ID4gYXR0YWNoZWQuDQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiBUaGUgaW9tbXVfZG9t
YWluIGlzIHJlcXVpcmVkIGZvciBtYWtpbmcgQVBJIGNhbGxzDQo+ID4gPiA+ID4gPiA+ID4gPiA+
ID4gPiByZWxhdGVkIHRvDQo+ID4gPiA+ID4gaW9tbXUuDQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4g
PiBGb2xsb3cgdXAgcGF0Y2hlcyB3aGljaCB1c2UgdGhpcyBBUEkgdG8ga25vdyBpb21tdQ0KPiA+
ID4gbWFwaW5nLg0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gPiA+ID4g
PiA+IFNpZ25lZC1vZmYtYnk6IEJoYXJhdCBCaHVzaGFuDQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4g
PiA8YmhhcmF0LmJodXNoYW5AZnJlZXNjYWxlLmNvbT4NCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+
IC0tLQ0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gIGRyaXZlcnMvaW9tbXUvaW9tbXUuYyB8ICAg
MTAgKysrKysrKysrKw0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gIGluY2x1ZGUvbGludXgvaW9t
bXUuaCB8ICAgIDcgKysrKysrKw0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gIDIgZmlsZXMgY2hh
bmdlZCwgMTcgaW5zZXJ0aW9ucygrKSwgMCBkZWxldGlvbnMoLSkNCj4gPiA+ID4gPiA+ID4gPiA+
ID4gPiA+DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9pb21t
dS9pb21tdS5jDQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiBiL2RyaXZlcnMvaW9tbXUvaW9tbXUu
YyBpbmRleA0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gZmJlOWNhNy4uNmFjNWY1MCAxMDA2NDQN
Cj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IC0tLSBhL2RyaXZlcnMvaW9tbXUvaW9tbXUuYw0KPiA+
ID4gPiA+ID4gPiA+ID4gPiA+ID4gKysrIGIvZHJpdmVycy9pb21tdS9pb21tdS5jDQo+ID4gPiA+
ID4gPiA+ID4gPiA+ID4gPiBAQCAtNjk2LDYgKzY5NiwxNiBAQCB2b2lkDQo+ID4gPiA+ID4gPiA+
ID4gPiA+ID4gPiBpb21tdV9kZXRhY2hfZGV2aWNlKHN0cnVjdCBpb21tdV9kb21haW4gKmRvbWFp
biwNCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IHN0cnVjdCBkZXZpY2UgKmRldikgIH0NCj4gPiA+
ID4gPiA+ID4gPiA+ID4gPiA+IEVYUE9SVF9TWU1CT0xfR1BMKGlvbW11X2RldGFjaF9kZXZpY2Up
Ow0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+ICtzdHJ1
Y3QgaW9tbXVfZG9tYWluICppb21tdV9nZXRfZGV2X2RvbWFpbihzdHJ1Y3QNCj4gPiA+IGRldmlj
ZSAqZGV2KSB7DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiArCXN0cnVjdCBpb21tdV9vcHMgKm9w
cyA9IGRldi0+YnVzLT5pb21tdV9vcHM7DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiArDQo+ID4g
PiA+ID4gPiA+ID4gPiA+ID4gPiArCWlmICh1bmxpa2VseShvcHMgPT0gTlVMTCB8fA0KPiA+ID4g
PiA+ID4gPiA+ID4gPiA+ID4gK29wcy0+Z2V0X2Rldl9pb21tdV9kb21haW4gPT0NCj4gPiA+ID4g
PiBOVUxMKSkNCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+ICsJCXJldHVybiBOVUxMOw0KPiA+ID4g
PiA+ID4gPiA+ID4gPiA+ID4gKw0KPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gKwlyZXR1cm4gb3Bz
LT5nZXRfZGV2X2lvbW11X2RvbWFpbihkZXYpOyB9DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiAr
RVhQT1JUX1NZTUJPTF9HUEwoaW9tbXVfZ2V0X2Rldl9kb21haW4pOw0KPiA+ID4gPiA+ID4gPiA+
ID4gPiA+DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gV2hhdCBwcmV2ZW50cyB0aGlzIGZyb20gcmFj
aW5nIGlvbW11X2RvbWFpbl9mcmVlKCk/DQo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gVGhlcmUncyBu
byByZWZlcmVuY2VzIGFjcXVpcmVkLCBzbyB0aGVyZSdzIG5vIHJlYXNvbg0KPiA+ID4gPiA+ID4g
PiA+ID4gPiA+IGZvciB0aGUgY2FsbGVyIHRvIGFzc3VtZSB0aGUNCj4gPiA+ID4gPiA+ID4gPiA+
IHBvaW50ZXIgaXMgdmFsaWQuDQo+ID4gPiA+ID4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiA+ID4g
PiA+IFNvcnJ5IGZvciBsYXRlIHF1ZXJ5LCBzb21laG93IHRoaXMgZW1haWwgd2VudCBpbnRvIGEN
Cj4gPiA+ID4gPiA+ID4gPiA+ID4gZm9sZGVyIGFuZCBlc2NhcGVkOw0KPiA+ID4gPiA+ID4gPiA+
ID4gPg0KPiA+ID4gPiA+ID4gPiA+ID4gPiBKdXN0IHRvIGJlIHN1cmUsIHRoZXJlIGlzIG5vdCBs
b2NrIGF0IGdlbmVyaWMgInN0cnVjdA0KPiA+ID4gPiA+ID4gPiA+ID4gPiBpb21tdV9kb21haW4i
LCBidXQgSVANCj4gPiA+ID4gPiA+ID4gPiA+IHNwZWNpZmljIHN0cnVjdHVyZSAobGluayBGU0wg
ZG9tYWluKSBsaW5rZWQgaW4NCj4gPiA+ID4gPiA+ID4gPiA+IGlvbW11X2RvbWFpbi0+cHJpdiBo
YXZlIGEgbG9jaywgc28gd2UgbmVlZCB0byBlbnN1cmUNCj4gPiA+ID4gPiA+ID4gPiA+IHRoaXMg
cmFjZSBpbiBGU0wgaW9tbXUgY29kZSAoc2F5DQo+ID4gPiA+ID4gPiA+ID4gPiBkcml2ZXJzL2lv
bW11L2ZzbF9wYW11X2RvbWFpbi5jKSwNCj4gPiA+IHJpZ2h0Pw0KPiA+ID4gPiA+ID4gPiA+ID4N
Cj4gPiA+ID4gPiA+ID4gPiA+IE5vLCBpdCdzIG5vdCBzdWZmaWNpZW50IHRvIG1ha2Ugc3VyZSB0
aGF0IHlvdXIgdXNlIG9mDQo+ID4gPiA+ID4gPiA+ID4gPiB0aGUgaW50ZXJmYWNlIGlzIHJhY2Ug
ZnJlZS4gIFRoZSBpbnRlcmZhY2UgaXRzZWxmIG5lZWRzDQo+ID4gPiA+ID4gPiA+ID4gPiB0byBi
ZSBkZXNpZ25lZCBzbyB0aGF0IGl0J3MgZGlmZmljdWx0IHRvIHVzZSBpbmNvcnJlY3RseS4NCj4g
PiA+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiA+IFNvIHdlIGNhbiBkZWZpbmUNCj4gPiA+ID4g
PiA+ID4gPiBpb21tdV9nZXRfZGV2X2RvbWFpbigpL2lvbW11X3B1dF9kZXZfZG9tYWluKCk7DQo+
ID4gPiA+ID4gPiA+ID4gaW9tbXVfZ2V0X2Rldl9kb21haW4oKSB3aWxsIHJldHVybiBkb21haW4g
d2l0aCB0aGUgbG9jaw0KPiA+ID4gPiA+ID4gPiA+IGhlbGQsIGFuZA0KPiA+ID4gPiA+ID4gPiA+
IGlvbW11X3B1dF9kZXZfZG9tYWluKCkgd2lsbCByZWxlYXNlIHRoZSBsb2NrPyBBbmQNCj4gPiA+
ID4gPiA+ID4gPiBpb21tdV9nZXRfZGV2X2RvbWFpbigpIG11c3QgYWx3YXlzIGJlIGZvbGxvd2Vk
IGJ5DQo+ID4gPiA+ID4gPiA+ID4gaW9tbXVfZ2V0X2Rldl9kb21haW4oKS4NCj4gPiA+ID4gPiA+
ID4NCj4gPiA+ID4gPiA+ID4gV2hhdCBsb2NrPyAgZ2V0L3B1dCBhcmUgZ2VuZXJhbGx5IHVzZWQg
Zm9yIHJlZmVyZW5jZQ0KPiA+ID4gPiA+ID4gPiBjb3VudGluZywgbm90IGxvY2tpbmcgaW4gdGhl
IGtlcm5lbC4NCj4gPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gPiA+IFRoYXQncyBub3QgdGhl
IGNhc2UgaGVyZS4gIFRoaXMgaXMgYSBiYWNrZG9vciB0byBnZXQgdGhlDQo+ID4gPiA+ID4gPiA+
ID4gPiBpb21tdSBkb21haW4gZnJvbSB0aGUgaW9tbXUgZHJpdmVyIHJlZ2FyZGxlc3Mgb2Ygd2hv
IGlzDQo+ID4gPiA+ID4gPiA+ID4gPiB1c2luZw0KPiA+ID4gaXQgb3IgaG93Lg0KPiA+ID4gPiA+
ID4gPiA+ID4gVGhlIGlvbW11IGRvbWFpbiBpcyBjcmVhdGVkIGFuZCBtYW5hZ2VkIGJ5IHZmaW8s
IHNvDQo+ID4gPiA+ID4gPiA+ID4gPiBzaG91bGRuJ3Qgd2UgYmUgbG9va2luZyBhdCBob3cgdG8g
ZG8gdGhpcyB0aHJvdWdoIHZmaW8/DQo+ID4gPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gPiBM
ZXQgbWUgZmlyc3QgZGVzY3JpYmUgd2hhdCB3ZSBhcmUgZG9pbmcgaGVyZToNCj4gPiA+ID4gPiA+
ID4gPiBEdXJpbmcgaW5pdGlhbGl6YXRpb246LQ0KPiA+ID4gPiA+ID4gPiA+ICAtIHZmaW8gdGFs
a3MgdG8gTVNJIHN5c3RlbSB0byBrbm93IHRoZSBNU0ktcGFnZSBhbmQgc2l6ZQ0KPiA+ID4gPiA+
ID4gPiA+ICAtIHZmaW8gdGhlbiBpbnRlcmFjdHMgd2l0aCBpb21tdSB0byBtYXAgdGhlIE1TSS1w
YWdlIGluDQo+ID4gPiA+ID4gPiA+ID4gaW9tbXUgKElPVkEgaXMgZGVjaWRlZCBieSB1c2Vyc3Bh
Y2UgYW5kIHBoeXNpY2FsIGFkZHJlc3MNCj4gPiA+ID4gPiA+ID4gPiBpcyB0aGUNCj4gPiA+ID4g
PiA+ID4gPiBNU0ktcGFnZSkNCj4gPiA+ID4gPiA+ID4gPiAgLSBTbyB0aGUgSU9WQSBzdWJ3aW5k
b3cgbWFwcGluZyBpcyBjcmVhdGVkIGluIGlvbW11IGFuZA0KPiA+ID4gPiA+ID4gPiA+IHllcyBW
RklPIGtub3cgYWJvdXQNCj4gPiA+ID4gPiA+ID4gdGhpcyBtYXBwaW5nLg0KPiA+ID4gPiA+ID4g
PiA+DQo+ID4gPiA+ID4gPiA+ID4gTm93IGRvIFNFVF9JUlEoTVNJL01TSVgpIGlvY3RsOg0KPiA+
ID4gPiA+ID4gPiA+ICAtIGNhbGxzIHBjaV9lbmFibGVfbXNpeCgpL3BjaV9lbmFibGVfbXNpX2Js
b2NrKCk6IHdoaWNoDQo+ID4gPiA+ID4gPiA+ID4gaXMgc3VwcG9zZWQgdG8gc2V0DQo+ID4gPiA+
ID4gPiA+IE1TSSBhZGRyZXNzL2RhdGEgaW4gZGV2aWNlLg0KPiA+ID4gPiA+ID4gPiA+ICAtIFNv
IGluIGN1cnJlbnQgaW1wbGVtZW50YXRpb24gKHRoaXMgcGF0Y2hzZXQpDQo+ID4gPiA+ID4gPiA+
ID4gbXNpLXN1YnN5c3RlbSBnZXRzIHRoZSBJT1ZBDQo+ID4gPiA+ID4gPiA+IGZyb20gaW9tbXUg
dmlhIHRoaXMgZGVmaW5lZCBpbnRlcmZhY2UuDQo+ID4gPiA+ID4gPiA+ID4gIC0gQXJlIHlvdSBz
YXlpbmcgdGhhdCByYXRoZXIgdGhhbiBnZXR0aW5nIHRoaXMgZnJvbQ0KPiA+ID4gPiA+ID4gPiA+
IGlvbW11LCB3ZSBzaG91bGQgZ2V0IHRoaXMNCj4gPiA+ID4gPiA+ID4gZnJvbSB2ZmlvPyBXaGF0
IGRpZmZlcmVuY2UgZG9lcyB0aGlzIG1ha2U/DQo+ID4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiA+
IFllcywgeW91IGp1c3Qgc2FpZCBhYm92ZSB0aGF0IHZmaW8ga25vd3MgdGhlIG1zaSB0byBpb3Zh
DQo+ID4gPiA+ID4gPiA+IG1hcHBpbmcsIHNvIHdoeSBnbyBvdXRzaWRlIG9mIHZmaW8gdG8gZmlu
ZCBpdCBsYXRlcj8gIFRoZQ0KPiA+ID4gPiA+ID4gPiBkaWZmZXJlbmNlIGlzIG9uZSBjYXNlIHlv
dSBjYW4gaGF2ZSBhIHByb3BlciByZWZlcmVuY2UgdG8NCj4gPiA+ID4gPiA+ID4gZGF0YSBzdHJ1
Y3R1cmVzIHRvIG1ha2Ugc3VyZSB0aGUgcG9pbnRlciB5b3UgZ2V0IGJhY2sNCj4gPiA+ID4gPiA+
ID4gYWN0dWFsbHkgaGFzIG1lYW5pbmcgYXQgdGhlIHRpbWUgeW91J3JlIHVzaW5nIGl0IHZzIHRo
ZSBjb2RlDQo+ID4gPiA+ID4gPiA+IGhlcmUgd2hlcmUgeW91J3JlIGRlZmluaW5nIGFuIEFQSSB0
aGF0IHJldHVybnMgYSBtZWFuaW5nbGVzcw0KPiA+ID4gPiA+ID4gPiB2YWx1ZQ0KPiA+ID4gPiA+
ID4NCj4gPiA+ID4gPiA+IFdpdGggRlNMLVBBTVUgd2Ugd2lsbCBhbHdheXMgZ2V0IGNvbnNpc3Rh
bnQgZGF0YSBmcm9tIGlvbW11IG9yDQo+ID4gPiA+ID4gPiB2ZmlvLWRhdGENCj4gPiA+ID4gPiBz
dHJ1Y3R1cmUuDQo+ID4gPiA+ID4NCj4gPiA+ID4gPiBHcmVhdCwgYnV0IHlvdSdyZSB0cnlpbmcg
dG8gYWRkIGEgZ2VuZXJpYyBBUEkgdG8gdGhlIElPTU1VDQo+ID4gPiA+ID4gc3Vic3lzdGVtIHRo
YXQncyBkaWZmaWN1bHQgdG8gdXNlIGNvcnJlY3RseS4gIFRoZSBmYWN0IHRoYXQgeW91DQo+ID4g
PiA+ID4gdXNlIGl0IGNvcnJlY3RseSBkb2VzIG5vdCBqdXN0aWZ5IHRoZSBBUEkuDQo+ID4gPiA+
ID4NCj4gPiA+ID4gPiA+ID4gYmVjYXVzZSB5b3UgY2FuJ3QgY2hlY2sgb3INCj4gPiA+ID4gPiA+
ID4gZW5mb3JjZSB0aGF0IGFuIGFyYml0cmFyeSBjYWxsZXIgaXMgdXNpbmcgaXQgY29ycmVjdGx5
Lg0KPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+IEkgYW0gbm90IHN1cmUgd2hhdCBpcyBhcmJpdHJh
cnkgY2FsbGVyPyBwZGV2IGlzIGtub3duIHRvIHZmaW8sDQo+ID4gPiA+ID4gPiBzbyB2ZmlvIHdp
bGwgb25seSBtYWtlDQo+ID4gPiA+ID4gPiBwY2lfZW5hYmxlX21zaXgoKS9wY2lfZW5hYmxlX21z
aV9ibG9jaygpIGZvcg0KPiA+ID4gdGhpcyBwZGV2Lg0KPiA+ID4gPiA+ID4gSWYgYW55b3RoZXIg
Y29kZSBtYWtlcyB0aGVuIGl0IGlzIHNvbWUgb3RoZXIgdW5leHBlY3RlZGx5DQo+ID4gPiA+ID4g
PiB0aGluZyBoYXBwZW5pbmcgaW4gc3lzdGVtLCBubz8NCj4gPiA+ID4gPg0KPiA+ID4gPiA+IFdo
YXQncyBwcm9wb3NlZCBoZXJlIGlzIGEgZ2VuZXJpYyBJT01NVSBBUEkuICBBbnlib2R5IGNhbiBj
YWxsDQo+IHRoaXMuDQo+ID4gPiA+ID4gV2hhdCBpZiB0aGUgaG9zdCBTQ1NJIGRyaXZlciBkZWNp
ZGVzIHRvIGdvIGdldCB0aGUgaW9tbXUgZG9tYWluDQo+ID4gPiA+ID4gZm9yIGl0J3MgZGV2aWNl
IChvciBhbnkgb3RoZXIgZGV2aWNlKT8gIERvZXMgdGhhdCBmaXQgeW91ciB1c2FnZQ0KPiBtb2Rl
bD8NCj4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiAgSXQncyBub3QgbWFpbnRhaW5hYmxlLg0KPiA+
ID4gPiA+ID4gPiBUaGFua3MsDQo+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gSSBkbyBub3QgaGF2
ZSBhbnkgaXNzdWUgd2l0aCB0aGlzIGFzIHdlbGwsIGNhbiB5b3UgYWxzbw0KPiA+ID4gPiA+ID4g
ZGVzY3JpYmUgdGhlIHR5cGUgb2YgQVBJIHlvdSBhcmUgZW52aXNpb25pbmc7IEkgY2FuIHRoaW5r
IG9mDQo+ID4gPiA+ID4gPiBkZWZpbmluZyBzb21lIGZ1bmN0aW9uIGluIHZmaW8uYy92ZmlvX2lv
bW11Ki5jLCBtYWtlIHRoZW0NCj4gPiA+ID4gPiA+IGdsb2JhbCBhbmQgZGVjbGFyZSB0aGVuIGlu
IGluY2x1ZGUvTGludXgvdmZpby5oIEFuZCBpbmNsdWRlDQo+ID4gPiA+ID4gPiA8TGludXgvdmZp
by5oPiBpbiBjYWxsZXIgZmlsZQ0KPiA+ID4gPiA+ID4gKGFyY2gvcG93ZXJwYy9rZXJuZWwvbXNp
LmMpDQo+ID4gPiA+ID4NCj4gPiA+ID4gPiBEbyB5b3UgcmVhbGx5IHdhbnQgbW9kdWxlIGRlcGVu
ZGVuY2llcyBiZXR3ZWVuIHZmaW8gYW5kIHlvdXINCj4gPiA+ID4gPiBjb3JlIGtlcm5lbCBNU0kg
c2V0dXA/ICBMb29rIGF0IHRoZSB2ZmlvIGV4dGVybmFsIHVzZXIgaW50ZXJmYWNlDQo+ID4gPiA+
ID4gdGhhdA0KPiA+ID4gd2UndmUgYWxyZWFkeSBkZWZpbmVkLg0KPiA+ID4gPiA+IFRoYXQgYWxs
b3dzIG90aGVyIGNvbXBvbmVudHMgb2YgdGhlIGtlcm5lbCB0byBnZXQgYSBwcm9wZXINCj4gPiA+
ID4gPiByZWZlcmVuY2UgdG8gYSB2ZmlvIGdyb3VwLiAgRnJvbSB0aGVyZSB5b3UgY2FuIHdvcmsg
b3V0IGhvdyB0bw0KPiA+ID4gPiA+IGdldCB3aGF0IHlvdSB3YW50LiAgQW5vdGhlciBhbHRlcm5h
dGl2ZSBpcyB0aGF0IHZmaW8gY291bGQNCj4gPiA+ID4gPiByZWdpc3RlciBhbiBNU0kgdG8gSU9W
QSBtYXBwaW5nIHdpdGggYXJjaGl0ZWN0dXJlIGNvZGUgd2hlbiB0aGUNCj4gbWFwcGluZyBpcyBj
cmVhdGVkLg0KPiA+ID4gPiA+IFRoZSBNU0kgc2V0dXAgcGF0aCBjb3VsZCB0aGVuIGRvIGEgbG9v
a3VwIGluIGFyY2hpdGVjdHVyZSBjb2RlDQo+ID4gPiA+ID4gZm9yIHRoZSBtYXBwaW5nLiAgWW91
IGNvdWxkIGV2ZW4gc3RvcmUgdGhlIE1TSSB0byBJT1ZBIG1hcHBpbmcNCj4gPiA+ID4gPiBpbiBW
RklPIGFuZCBjcmVhdGUgYW4gaW50ZXJmYWNlIHdoZXJlIFNFVF9JUlEgcGFzc2VzIHRoYXQNCj4g
PiA+ID4gPiBtYXBwaW5nIGludG8gc2V0dXANCj4gPiA+IGNvZGUuDQo+ID4gW1NldGhpIFZhcnVu
LUIxNjM5NV0gV2hhdCB5b3UgYXJlIHN1Z2dlc3RpbmcgaXMgdGhhdCB0aGUgTVNJIHNldHVwDQo+
ID4gcGF0aCBxdWVyaWVzIHRoZSB2ZmlvIHN1YnN5c3RlbSBmb3IgdGhlIG1hcHBpbmcsIHJhdGhl
ciB0aGFuIGRpcmVjdGx5DQo+ID4gcXVlcnlpbmcgdGhlIGlvbW11IHN1YnN5c3RlbS4gU28sIHNh
eSBpZiB3ZSBhZGQgYW4gaW50ZXJmYWNlIGZvcg0KPiA+IGdldHRpbmcgTVNJIHRvIElPVkEgbWFw
cGluZyBpbiB0aGUgbXNpIHNldHVwIHBhdGgsIHdvdWxkbid0IHRoaXMgYWdhaW4NCj4gPiBiZSBz
cGVjaWZpYyB0byB2ZmlvPyBJIHJlY2tvbiB0aGF0IHRoaXMgaW50ZXJmYWNlIHdvdWxkIGFnYWlu
IHBwYw0KPiA+IG1hY2hpbmUgc3BlY2lmaWMgaW50ZXJmYWNlLg0KPiANCj4gU3VyZSwgaWYgdGhp
cyBpcyBhIGdlbmVyaWMgTVNJIHNldHVwIHBhdGggdGhlbiBjbGVhcmx5IHZmaW8gc2hvdWxkIG5v
dCBiZQ0KPiBpbnZvbHZlZC4gIEJ1dCBieSB0aGF0IHNhbWUgbm90aW9uLCBpZiB0aGlzIGlzIGEg
Z2VuZXJpYyBNU0kgc2V0dXAgcGF0aCwNCj4gaG93IGNhbiB0aGUgcHJvcG9zZWQgc29sdXRpb25z
IGd1YXJhbnRlZSB0aGF0IHRoZSBpb21tdV9kb21haW4gb3IgaW92YQ0KPiByZXR1cm5lZCBpcyBz
dGlsbCB2YWxpZCBpbiBhbGwgY2FzZXM/ICBJdCdzIHJhY3kuICBJZiB0aGUgY2FsbGVyIHRyeWlu
Zw0KPiB0byBzZXR1cCBNU0kgaGFzIHRoZSBpbmZvcm1hdGlvbiBuZWVkZWQsIHdoeSBkb2Vzbid0
IGl0IHBhc3MgaXQgaW4gYXMNCj4gcGFydCBvZiB0aGUgc2V0dXA/ICBUaGFua3MsDQpbU2V0aGkg
VmFydW4tQjE2Mzk1XSBBZ3JlZWQsIHRoZSBwcm9wb3NlZCBpbnRlcmZhY2UgaXMgbm90IGNsZWFu
LiBCdXQsIHdlIHN0aWxsIG5lZWQgYW4gaW50ZXJmYWNlIHRocm91Z2ggd2hpY2ggTVNJIGRyaXZl
ciBjYW4gY2FsbCBpbiB0byB0aGUgdmZpbyBsYXllci4NCg0KLVZhcnVuDQoNCg0K

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

Alex Williamson Oct. 14, 2013, 2:20 p.m. UTC | #15

On Mon, 2013-10-14 at 12:58 +0000, Sethi Varun-B16395 wrote:
> 
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Friday, October 11, 2013 2:12 AM
> > To: Sethi Varun-B16395
> > Cc: Bhushan Bharat-R65777; agraf@suse.de; Wood Scott-B07421; linux-
> > pci@vger.kernel.org; galak@kernel.crashing.org; linux-
> > kernel@vger.kernel.org; iommu@lists.linux-foundation.org;
> > benh@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> > 
> > On Thu, 2013-10-10 at 20:09 +0000, Sethi Varun-B16395 wrote:
> > >
> > > > -----Original Message-----
> > > > From: iommu-bounces@lists.linux-foundation.org [mailto:iommu-
> > > > bounces@lists.linux-foundation.org] On Behalf Of Alex Williamson
> > > > Sent: Tuesday, October 08, 2013 8:43 AM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: agraf@suse.de; Wood Scott-B07421; linux-pci@vger.kernel.org;
> > > > galak@kernel.crashing.org; linux-kernel@vger.kernel.org;
> > > > iommu@lists.linux-foundation.org; benh@kernel.crashing.org;
> > > > linuxppc- dev@lists.ozlabs.org
> > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > device
> > > >
> > > > On Mon, 2013-10-07 at 05:46 +0000, Bhushan Bharat-R65777 wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > > Sent: Friday, October 04, 2013 11:42 PM
> > > > > > To: Bhushan Bharat-R65777
> > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > foundation.org
> > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > > > device
> > > > > >
> > > > > > On Fri, 2013-10-04 at 17:23 +0000, Bhushan Bharat-R65777 wrote:
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > > > > Sent: Friday, October 04, 2013 10:43 PM
> > > > > > > > To: Bhushan Bharat-R65777
> > > > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > > > galak@kernel.crashing.org; linux- kernel@vger.kernel.org;
> > > > > > > > linuxppc-dev@lists.ozlabs.org; linux- pci@vger.kernel.org;
> > > > > > > > agraf@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > > > foundation.org
> > > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain
> > > > > > > > of a device
> > > > > > > >
> > > > > > > > On Fri, 2013-10-04 at 16:47 +0000, Bhushan Bharat-R65777
> > wrote:
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Alex Williamson
> > > > > > > > > > [mailto:alex.williamson@redhat.com]
> > > > > > > > > > Sent: Friday, October 04, 2013 9:15 PM
> > > > > > > > > > To: Bhushan Bharat-R65777
> > > > > > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > > > > > galak@kernel.crashing.org; linux-
> > > > > > > > > > kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > > > > > > > linux- pci@vger.kernel.org; agraf@suse.de; Wood
> > > > > > > > > > Scott-B07421; iommu@lists.linux- foundation.org
> > > > > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get
> > > > > > > > > > iommu_domain of a device
> > > > > > > > > >
> > > > > > > > > > On Fri, 2013-10-04 at 09:54 +0000, Bhushan Bharat-R65777
> > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: linux-pci-owner@vger.kernel.org
> > > > > > > > > > > > [mailto:linux-pci-owner@vger.kernel.org]
> > > > > > > > > > > > On Behalf Of Alex Williamson
> > > > > > > > > > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > > > > > > > > > To: Bhushan Bharat-R65777
> > > > > > > > > > > > Cc: joro@8bytes.org; benh@kernel.crashing.org;
> > > > > > > > > > > > galak@kernel.crashing.org; linux-
> > > > > > > > > > > > kernel@vger.kernel.org;
> > > > > > > > > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > > > > > > > linux- pci@vger.kernel.org; agraf@suse.de; Wood
> > > > > > > > > > > > Scott-B07421; iommu@lists.linux- foundation.org;
> > > > > > > > > > > > Bhushan
> > > > > > > > > > > > Bharat-R65777
> > > > > > > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get
> > > > > > > > > > > > iommu_domain of a device
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan
> > wrote:
> > > > > > > > > > > > > This api return the iommu domain to which the
> > > > > > > > > > > > > device is
> > > > attached.
> > > > > > > > > > > > > The iommu_domain is required for making API calls
> > > > > > > > > > > > > related to
> > > > > > iommu.
> > > > > > > > > > > > > Follow up patches which use this API to know iommu
> > > > maping.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Bharat Bhushan
> > > > > > > > > > > > > <bharat.bhushan@freescale.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  drivers/iommu/iommu.c |   10 ++++++++++
> > > > > > > > > > > > >  include/linux/iommu.h |    7 +++++++
> > > > > > > > > > > > >  2 files changed, 17 insertions(+), 0 deletions(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/drivers/iommu/iommu.c
> > > > > > > > > > > > > b/drivers/iommu/iommu.c index
> > > > > > > > > > > > > fbe9ca7..6ac5f50 100644
> > > > > > > > > > > > > --- a/drivers/iommu/iommu.c
> > > > > > > > > > > > > +++ b/drivers/iommu/iommu.c
> > > > > > > > > > > > > @@ -696,6 +696,16 @@ void
> > > > > > > > > > > > > iommu_detach_device(struct iommu_domain *domain,
> > > > > > > > > > > > > struct device *dev)  }
> > > > > > > > > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > > > > > > > > > >
> > > > > > > > > > > > > +struct iommu_domain *iommu_get_dev_domain(struct
> > > > device *dev) {
> > > > > > > > > > > > > +	struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +	if (unlikely(ops == NULL ||
> > > > > > > > > > > > > +ops->get_dev_iommu_domain ==
> > > > > > NULL))
> > > > > > > > > > > > > +		return NULL;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +	return ops->get_dev_iommu_domain(dev); }
> > > > > > > > > > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > > > > > > > > > > >
> > > > > > > > > > > > What prevents this from racing iommu_domain_free()?
> > > > > > > > > > > > There's no references acquired, so there's no reason
> > > > > > > > > > > > for the caller to assume the
> > > > > > > > > > pointer is valid.
> > > > > > > > > > >
> > > > > > > > > > > Sorry for late query, somehow this email went into a
> > > > > > > > > > > folder and escaped;
> > > > > > > > > > >
> > > > > > > > > > > Just to be sure, there is not lock at generic "struct
> > > > > > > > > > > iommu_domain", but IP
> > > > > > > > > > specific structure (link FSL domain) linked in
> > > > > > > > > > iommu_domain->priv have a lock, so we need to ensure
> > > > > > > > > > this race in FSL iommu code (say
> > > > > > > > > > drivers/iommu/fsl_pamu_domain.c),
> > > > right?
> > > > > > > > > >
> > > > > > > > > > No, it's not sufficient to make sure that your use of
> > > > > > > > > > the interface is race free.  The interface itself needs
> > > > > > > > > > to be designed so that it's difficult to use incorrectly.
> > > > > > > > >
> > > > > > > > > So we can define
> > > > > > > > > iommu_get_dev_domain()/iommu_put_dev_domain();
> > > > > > > > > iommu_get_dev_domain() will return domain with the lock
> > > > > > > > > held, and
> > > > > > > > > iommu_put_dev_domain() will release the lock? And
> > > > > > > > > iommu_get_dev_domain() must always be followed by
> > > > > > > > > iommu_get_dev_domain().
> > > > > > > >
> > > > > > > > What lock?  get/put are generally used for reference
> > > > > > > > counting, not locking in the kernel.
> > > > > > > >
> > > > > > > > > > That's not the case here.  This is a backdoor to get the
> > > > > > > > > > iommu domain from the iommu driver regardless of who is
> > > > > > > > > > using
> > > > it or how.
> > > > > > > > > > The iommu domain is created and managed by vfio, so
> > > > > > > > > > shouldn't we be looking at how to do this through vfio?
> > > > > > > > >
> > > > > > > > > Let me first describe what we are doing here:
> > > > > > > > > During initialization:-
> > > > > > > > >  - vfio talks to MSI system to know the MSI-page and size
> > > > > > > > >  - vfio then interacts with iommu to map the MSI-page in
> > > > > > > > > iommu (IOVA is decided by userspace and physical address
> > > > > > > > > is the
> > > > > > > > > MSI-page)
> > > > > > > > >  - So the IOVA subwindow mapping is created in iommu and
> > > > > > > > > yes VFIO know about
> > > > > > > > this mapping.
> > > > > > > > >
> > > > > > > > > Now do SET_IRQ(MSI/MSIX) ioctl:
> > > > > > > > >  - calls pci_enable_msix()/pci_enable_msi_block(): which
> > > > > > > > > is supposed to set
> > > > > > > > MSI address/data in device.
> > > > > > > > >  - So in current implementation (this patchset)
> > > > > > > > > msi-subsystem gets the IOVA
> > > > > > > > from iommu via this defined interface.
> > > > > > > > >  - Are you saying that rather than getting this from
> > > > > > > > > iommu, we should get this
> > > > > > > > from vfio? What difference does this make?
> > > > > > > >
> > > > > > > > Yes, you just said above that vfio knows the msi to iova
> > > > > > > > mapping, so why go outside of vfio to find it later?  The
> > > > > > > > difference is one case you can have a proper reference to
> > > > > > > > data structures to make sure the pointer you get back
> > > > > > > > actually has meaning at the time you're using it vs the code
> > > > > > > > here where you're defining an API that returns a meaningless
> > > > > > > > value
> > > > > > >
> > > > > > > With FSL-PAMU we will always get consistant data from iommu or
> > > > > > > vfio-data
> > > > > > structure.
> > > > > >
> > > > > > Great, but you're trying to add a generic API to the IOMMU
> > > > > > subsystem that's difficult to use correctly.  The fact that you
> > > > > > use it correctly does not justify the API.
> > > > > >
> > > > > > > > because you can't check or
> > > > > > > > enforce that an arbitrary caller is using it correctly.
> > > > > > >
> > > > > > > I am not sure what is arbitrary caller? pdev is known to vfio,
> > > > > > > so vfio will only make
> > > > > > > pci_enable_msix()/pci_enable_msi_block() for
> > > > this pdev.
> > > > > > > If anyother code makes then it is some other unexpectedly
> > > > > > > thing happening in system, no?
> > > > > >
> > > > > > What's proposed here is a generic IOMMU API.  Anybody can call
> > this.
> > > > > > What if the host SCSI driver decides to go get the iommu domain
> > > > > > for it's device (or any other device)?  Does that fit your usage
> > model?
> > > > > >
> > > > > > > >  It's not maintainable.
> > > > > > > > Thanks,
> > > > > > >
> > > > > > > I do not have any issue with this as well, can you also
> > > > > > > describe the type of API you are envisioning; I can think of
> > > > > > > defining some function in vfio.c/vfio_iommu*.c, make them
> > > > > > > global and declare then in include/Linux/vfio.h And include
> > > > > > > <Linux/vfio.h> in caller file
> > > > > > > (arch/powerpc/kernel/msi.c)
> > > > > >
> > > > > > Do you really want module dependencies between vfio and your
> > > > > > core kernel MSI setup?  Look at the vfio external user interface
> > > > > > that
> > > > we've already defined.
> > > > > > That allows other components of the kernel to get a proper
> > > > > > reference to a vfio group.  From there you can work out how to
> > > > > > get what you want.  Another alternative is that vfio could
> > > > > > register an MSI to IOVA mapping with architecture code when the
> > mapping is created.
> > > > > > The MSI setup path could then do a lookup in architecture code
> > > > > > for the mapping.  You could even store the MSI to IOVA mapping
> > > > > > in VFIO and create an interface where SET_IRQ passes that
> > > > > > mapping into setup
> > > > code.
> > > [Sethi Varun-B16395] What you are suggesting is that the MSI setup
> > > path queries the vfio subsystem for the mapping, rather than directly
> > > querying the iommu subsystem. So, say if we add an interface for
> > > getting MSI to IOVA mapping in the msi setup path, wouldn't this again
> > > be specific to vfio? I reckon that this interface would again ppc
> > > machine specific interface.
> > 
> > Sure, if this is a generic MSI setup path then clearly vfio should not be
> > involved.  But by that same notion, if this is a generic MSI setup path,
> > how can the proposed solutions guarantee that the iommu_domain or iova
> > returned is still valid in all cases?  It's racy.  If the caller trying
> > to setup MSI has the information needed, why doesn't it pass it in as
> > part of the setup?  Thanks,
> [Sethi Varun-B16395] Agreed, the proposed interface is not clean. But, we still need an interface through which MSI driver can call in to the vfio layer.

Or one that allows vfio to pass in the iova when the interrupt is being
setup.  I'm afraid any kind of reverse interface where MSI calls back
into vfio is going to be racy.  Thanks,

Alex

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

[2/7] iommu: add api to get iommu_domain of a device

Commit Message

Comments

Patch