[PATCH 0/5 v2] iommu: Support non-paging IOMMUs

[PATCH 0/5 v2] iommu: Support non-paging IOMMUs

[Joerg Roedel]

Hi Varuns, Stuart,

here is the reworked patch-set with the IOMMU-API changes to support the
PAMU IOMMU. It should include the results from our discussion last week.
Please have a look at it and tell me if this interface works for you. I
will merge it then into the IOMMU tree so that you have a branch to base
the PAMU patches on.

Thanks,

	Joerg

[PATCH 1/5] iommu: Make sure DOMAIN_ATTR_MAX is really the maximum

[Joerg Roedel]

Move it to the end of the list.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 include/linux/iommu.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f3b99e1..7e6ce72 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -58,8 +58,8 @@ struct iommu_domain {
 #define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
 
 enum iommu_attr {
-	DOMAIN_ATTR_MAX,
 	DOMAIN_ATTR_GEOMETRY,
+	DOMAIN_ATTR_MAX,
 };
 
 #ifdef CONFIG_IOMMU_API
-- 
1.7.9.5

[PATCH 2/5] iommu: Check for valid pgsize_bitmap in iommu_map/unmap

[Joerg Roedel]

In case the page-size bitmap is zero the code path in
iommu_map and iommu_unmap is undefined. Make it defined and
return -ENODEV in this case.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 drivers/iommu/iommu.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ddbdaca..622360f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -734,7 +734,8 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
 	size_t orig_size = size;
 	int ret = 0;
 
-	if (unlikely(domain->ops->map == NULL))
+	if (unlikely(domain->ops->unmap == NULL ||
+		     domain->ops->pgsize_bitmap == 0UL))
 		return -ENODEV;
 
 	/* find out the minimum page size supported */
@@ -808,7 +809,8 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 	size_t unmapped_page, unmapped = 0;
 	unsigned int min_pagesz;
 
-	if (unlikely(domain->ops->unmap == NULL))
+	if (unlikely(domain->ops->unmap == NULL ||
+		     domain->ops->pgsize_bitmap == 0UL))
 		return -ENODEV;
 
 	/* find out the minimum page size supported */
-- 
1.7.9.5

[PATCH 3/5] iommu: Implement DOMAIN_ATTR_PAGING attribute

[Joerg Roedel]

This attribute of a domain can be queried to find out if the
domain supports setting up page-tables using the iommu_map()
and iommu_unmap() functions.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 drivers/iommu/iommu.c |    5 +++++
 include/linux/iommu.h |    1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 622360f..ab9dafd 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -869,6 +869,7 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
 			  enum iommu_attr attr, void *data)
 {
 	struct iommu_domain_geometry *geometry;
+	bool *paging;
 	int ret = 0;
 
 	switch (attr) {
@@ -877,6 +878,10 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
 		*geometry = domain->geometry;
 
 		break;
+	case DOMAIN_ATTR_PAGING:
+		paging  = data;
+		*paging = (domain->ops->pgsize_bitmap != 0UL);
+		break;
 	default:
 		if (!domain->ops->domain_get_attr)
 			return -EINVAL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7e6ce72..26066f5 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -59,6 +59,7 @@ struct iommu_domain {
 
 enum iommu_attr {
 	DOMAIN_ATTR_GEOMETRY,
+	DOMAIN_ATTR_PAGING,
 	DOMAIN_ATTR_MAX,
 };
 
-- 
1.7.9.5

[PATCH 4/5] iommu: Add domain window handling functions

[Joerg Roedel]

Add the iommu_domain_window_enable() and iommu_domain_window_disable()
functions to the IOMMU-API. These functions will be used to setup
domains that are based on subwindows and not on paging.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 drivers/iommu/iommu.c |   20 ++++++++++++++++++++
 include/linux/iommu.h |   22 ++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ab9dafd..0fdb7db 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -852,6 +852,26 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 }
 EXPORT_SYMBOL_GPL(iommu_unmap);
 
+
+int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr,
+			       phys_addr_t paddr, size_t size)
+{
+	if (unlikely(domain->ops->domain_window_enable == NULL))
+		return -ENODEV;
+
+	return domain->ops->domain_window_enable(domain, wnd_nr, paddr, size);
+}
+EXPORT_SYMBOL_GPL(iommu_domain_window_enable);
+
+void iommu_domain_window_disable(struct iommu_domain *domain, u32 wnd_nr)
+{
+	if (unlikely(domain->ops->domain_window_disable == NULL))
+		return;
+
+	return domain->ops->domain_window_disable(domain, wnd_nr);
+}
+EXPORT_SYMBOL_GPL(iommu_domain_window_disable);
+
 static int __init iommu_init(void)
 {
 	iommu_group_kset = kset_create_and_add("iommu_groups",
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 26066f5..0cba2d8 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -101,6 +101,12 @@ struct iommu_ops {
 			       enum iommu_attr attr, void *data);
 	int (*domain_set_attr)(struct iommu_domain *domain,
 			       enum iommu_attr attr, void *data);
+
+	/* Window handling functions */
+	int (*domain_window_enable)(struct iommu_domain *domain, u32 wnd_nr,
+				    phys_addr_t paddr, size_t size);
+	void (*domain_window_disable)(struct iommu_domain *domain, u32 wnd_nr);
+
 	unsigned long pgsize_bitmap;
 };
 
@@ -158,6 +164,10 @@ extern int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr,
 extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
 				 void *data);
 
+/* Window handling function prototypes */
+extern int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr,
+				      phys_addr_t offset, size_t size);
+extern void iommu_domain_window_disable(struct iommu_domain *domain, u32 wnd_nr);
 /**
  * report_iommu_fault() - report about an IOMMU fault to the IOMMU framework
  * @domain: the iommu domain where the fault has happened
@@ -240,6 +250,18 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	return -ENODEV;
 }
 
+static inline int iommu_domain_window_enable(struct iommu_domain *domain,
+					     u32 wnd_nr, phys_addr_t paddr,
+					     size_t size)
+{
+	return -ENODEV;
+}
+
+static inline void iommu_domain_window_disable(struct iommu_domain *domain,
+					       u32 wnd_nr)
+{
+}
+
 static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
 					     unsigned long iova)
 {
-- 
1.7.9.5

[PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute

[Joerg Roedel]

This attribute can be used to set and get the number of
subwindows on IOMMUs that are window-based.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 drivers/iommu/iommu.c |   33 ++++++++++++++++++++++++++++++---
 include/linux/iommu.h |    5 +++++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0fdb7db..66402f7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -891,6 +891,7 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
 	struct iommu_domain_geometry *geometry;
 	bool *paging;
 	int ret = 0;
+	u32 *count;
 
 	switch (attr) {
 	case DOMAIN_ATTR_GEOMETRY:
@@ -902,6 +903,15 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
 		paging  = data;
 		*paging = (domain->ops->pgsize_bitmap != 0UL);
 		break;
+	case DOMAIN_ATTR_WINDOWS:
+		count = data;
+
+		if (domain->ops->domain_get_windows != NULL)
+			*count = domain->ops->domain_get_windows(domain);
+		else
+			ret = -ENODEV;
+
+		break;
 	default:
 		if (!domain->ops->domain_get_attr)
 			return -EINVAL;
@@ -916,9 +926,26 @@ EXPORT_SYMBOL_GPL(iommu_domain_get_attr);
 int iommu_domain_set_attr(struct iommu_domain *domain,
 			  enum iommu_attr attr, void *data)
 {
-	if (!domain->ops->domain_set_attr)
-		return -EINVAL;
+	int ret = 0;
+	u32 *count;
+
+	switch (attr) {
+	case DOMAIN_ATTR_WINDOWS:
+		count = data;
+
+		if (domain->ops->domain_set_windows != NULL)
+			ret = domain->ops->domain_set_windows(domain, *count);
+		else
+			ret = -ENODEV;
 
-	return domain->ops->domain_set_attr(domain, attr, data);
+		break;
+	default:
+		if (domain->ops->domain_set_attr == NULL)
+			return -EINVAL;
+
+		ret = domain->ops->domain_set_attr(domain, attr, data);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_domain_set_attr);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0cba2d8..8330df1 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -60,6 +60,7 @@ struct iommu_domain {
 enum iommu_attr {
 	DOMAIN_ATTR_GEOMETRY,
 	DOMAIN_ATTR_PAGING,
+	DOMAIN_ATTR_WINDOWS,
 	DOMAIN_ATTR_MAX,
 };
 
@@ -106,6 +107,10 @@ struct iommu_ops {
 	int (*domain_window_enable)(struct iommu_domain *domain, u32 wnd_nr,
 				    phys_addr_t paddr, size_t size);
 	void (*domain_window_disable)(struct iommu_domain *domain, u32 wnd_nr);
+	/* Set the numer of window per domain */
+	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);
 
 	unsigned long pgsize_bitmap;
 };
-- 
1.7.9.5

Re: [PATCH 4/5] iommu: Add domain window handling functions

[Stuart Yoder]

On Mon, Feb 4, 2013 at 7:18 AM, Joerg Roedel <joro@8bytes.org> wrote:
> Add the iommu_domain_window_enable() and iommu_domain_window_disable()
> functions to the IOMMU-API. These functions will be used to setup
> domains that are based on subwindows and not on paging.
>
> Signed-off-by: Joerg Roedel <joro@8bytes.org>
> ---
>  drivers/iommu/iommu.c |   20 ++++++++++++++++++++
>  include/linux/iommu.h |   22 ++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ab9dafd..0fdb7db 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -852,6 +852,26 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
>  }
>  EXPORT_SYMBOL_GPL(iommu_unmap);
>
> +
> +int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr,
> +                              phys_addr_t paddr, size_t size)
> +{
> +       if (unlikely(domain->ops->domain_window_enable == NULL))
> +               return -ENODEV;
> +
> +       return domain->ops->domain_window_enable(domain, wnd_nr, paddr, size);
> +}
> +EXPORT_SYMBOL_GPL(iommu_domain_window_enable);
> +
> +void iommu_domain_window_disable(struct iommu_domain *domain, u32 wnd_nr)
> +{
> +       if (unlikely(domain->ops->domain_window_disable == NULL))
> +               return;
> +
> +       return domain->ops->domain_window_disable(domain, wnd_nr);
> +}
> +EXPORT_SYMBOL_GPL(iommu_domain_window_disable);
> +
>  static int __init iommu_init(void)
>  {
>         iommu_group_kset = kset_create_and_add("iommu_groups",
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 26066f5..0cba2d8 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -101,6 +101,12 @@ struct iommu_ops {
>                                enum iommu_attr attr, void *data);
>         int (*domain_set_attr)(struct iommu_domain *domain,
>                                enum iommu_attr attr, void *data);
> +
> +       /* Window handling functions */
> +       int (*domain_window_enable)(struct iommu_domain *domain, u32 wnd_nr,
> +                                   phys_addr_t paddr, size_t size);
> +       void (*domain_window_disable)(struct iommu_domain *domain, u32 wnd_nr);
> +
>         unsigned long pgsize_bitmap;
>  };
>
> @@ -158,6 +164,10 @@ extern int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr,
>  extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
>                                  void *data);
>
> +/* Window handling function prototypes */
> +extern int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr,
> +                                     phys_addr_t offset, size_t size);
> +extern void iommu_domain_window_disable(struct iommu_domain *domain, u32 wnd_nr);
>  /**
>   * report_iommu_fault() - report about an IOMMU fault to the IOMMU framework
>   * @domain: the iommu domain where the fault has happened
> @@ -240,6 +250,18 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova,
>         return -ENODEV;
>  }
>
> +static inline int iommu_domain_window_enable(struct iommu_domain *domain,
> +                                            u32 wnd_nr, phys_addr_t paddr,
> +                                            size_t size)
> +{
> +       return -ENODEV;
> +}
> +
> +static inline void iommu_domain_window_disable(struct iommu_domain *domain,
> +                                              u32 wnd_nr)
> +{
> +}
> +
>  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
>                                              unsigned long iova)
>  {

This API looks workable.   The one change we need is that the size argument in
the enable API needs to be 64 bits.   Our window sizes can exceed 4GB.

Stuart

Re: [PATCH 4/5] iommu: Add domain window handling functions

[Joerg Roedel]

On Mon, Feb 04, 2013 at 12:10:51PM -0600, Stuart Yoder wrote:
> On Mon, Feb 4, 2013 at 7:18 AM, Joerg Roedel <joro@8bytes.org> wrote:
> > +static inline int iommu_domain_window_enable(struct iommu_domain *domain,
> > +                                            u32 wnd_nr, phys_addr_t paddr,
> > +                                            size_t size)
> > +{
> > +       return -ENODEV;
> > +}
> > +
> > +static inline void iommu_domain_window_disable(struct iommu_domain *domain,
> > +                                              u32 wnd_nr)
> > +{
> > +}
> > +
> >  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
> >                                              unsigned long iova)
> >  {
> 
> This API looks workable.   The one change we need is that the size argument in
> the enable API needs to be 64 bits.   Our window sizes can exceed 4GB.

Okay. So if your architecture supports sizes over 2^32 then size_t
probably is already 64bits, right?


	Joerg

Re: [PATCH 4/5] iommu: Add domain window handling functions

[Stuart Yoder]

On Mon, Feb 4, 2013 at 12:56 PM, Joerg Roedel <joro@8bytes.org> wrote:
> On Mon, Feb 04, 2013 at 12:10:51PM -0600, Stuart Yoder wrote:
>> On Mon, Feb 4, 2013 at 7:18 AM, Joerg Roedel <joro@8bytes.org> wrote:
>> > +static inline int iommu_domain_window_enable(struct iommu_domain *domain,
>> > +                                            u32 wnd_nr, phys_addr_t paddr,
>> > +                                            size_t size)
>> > +{
>> > +       return -ENODEV;
>> > +}
>> > +
>> > +static inline void iommu_domain_window_disable(struct iommu_domain *domain,
>> > +                                              u32 wnd_nr)
>> > +{
>> > +}
>> > +
>> >  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
>> >                                              unsigned long iova)
>> >  {
>>
>> This API looks workable.   The one change we need is that the size argument in
>> the enable API needs to be 64 bits.   Our window sizes can exceed 4GB.
>
> Okay. So if your architecture supports sizes over 2^32 then size_t
> probably is already 64bits, right?

No, on a 32-bit platform size_t would generally be 32-bits.  But the PAMU
is independent of that.   I think we should just make it a u64.

Stuart

RE: [PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute

[Sethi Varun-B16395]

Hi Joerg,
In case of PAMU, geometry would be meaningless without specifying the number of subwindows. If the API user specifies a geometry but fails to specify the number of required subwindows, we would assume a default (max supported) value, which may be incorrect. It's going to be really ugly. I think we should add the number of sub windows to the domain geometry. This parameter would be used in case of a window based iommu. So, a separate set_window API wouldn't be required.

We can still implement the get_windows API for getting the maximum number of subwindows supported by the window based IOMMU.

Regards
Varun

> -----Original Message-----
> From: Joerg Roedel [mailto:joro@8bytes.org]
> Sent: Monday, February 04, 2013 6:49 PM
> To: Sethi Varun-B16395; Stuart Yoder
> Cc: iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org; Joerg
> Roedel
> Subject: [PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute
> 
> This attribute can be used to set and get the number of subwindows on
> IOMMUs that are window-based.
> 
> Signed-off-by: Joerg Roedel <joro@8bytes.org>
> ---
>  drivers/iommu/iommu.c |   33 ++++++++++++++++++++++++++++++---
>  include/linux/iommu.h |    5 +++++
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> 0fdb7db..66402f7 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -891,6 +891,7 @@ int iommu_domain_get_attr(struct iommu_domain
> *domain,
>  	struct iommu_domain_geometry *geometry;
>  	bool *paging;
>  	int ret = 0;
> +	u32 *count;
> 
>  	switch (attr) {
>  	case DOMAIN_ATTR_GEOMETRY:
> @@ -902,6 +903,15 @@ int iommu_domain_get_attr(struct iommu_domain
> *domain,
>  		paging  = data;
>  		*paging = (domain->ops->pgsize_bitmap != 0UL);
>  		break;
> +	case DOMAIN_ATTR_WINDOWS:
> +		count = data;
> +
> +		if (domain->ops->domain_get_windows != NULL)
> +			*count = domain->ops->domain_get_windows(domain);
> +		else
> +			ret = -ENODEV;
> +
> +		break;
>  	default:
>  		if (!domain->ops->domain_get_attr)
>  			return -EINVAL;
> @@ -916,9 +926,26 @@ EXPORT_SYMBOL_GPL(iommu_domain_get_attr);
>  int iommu_domain_set_attr(struct iommu_domain *domain,
>  			  enum iommu_attr attr, void *data)
>  {
> -	if (!domain->ops->domain_set_attr)
> -		return -EINVAL;
> +	int ret = 0;
> +	u32 *count;
> +
> +	switch (attr) {
> +	case DOMAIN_ATTR_WINDOWS:
> +		count = data;
> +
> +		if (domain->ops->domain_set_windows != NULL)
> +			ret = domain->ops->domain_set_windows(domain, *count);
> +		else
> +			ret = -ENODEV;
> 
> -	return domain->ops->domain_set_attr(domain, attr, data);
> +		break;
> +	default:
> +		if (domain->ops->domain_set_attr == NULL)
> +			return -EINVAL;
> +
> +		ret = domain->ops->domain_set_attr(domain, attr, data);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(iommu_domain_set_attr);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> 0cba2d8..8330df1 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -60,6 +60,7 @@ struct iommu_domain {
>  enum iommu_attr {
>  	DOMAIN_ATTR_GEOMETRY,
>  	DOMAIN_ATTR_PAGING,
> +	DOMAIN_ATTR_WINDOWS,
>  	DOMAIN_ATTR_MAX,
>  };
> 
> @@ -106,6 +107,10 @@ struct iommu_ops {
>  	int (*domain_window_enable)(struct iommu_domain *domain, u32
> wnd_nr,
>  				    phys_addr_t paddr, size_t size);
>  	void (*domain_window_disable)(struct iommu_domain *domain, u32
> wnd_nr);
> +	/* Set the numer of window per domain */
> +	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);
> 
>  	unsigned long pgsize_bitmap;
>  };
> --
> 1.7.9.5
> 
> 

Re: [PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute

[Joerg Roedel]

Hi,

On Tue, Feb 05, 2013 at 09:14:40AM +0000, Sethi Varun-B16395 wrote:
> In case of PAMU, geometry would be meaningless without specifying the
> number of subwindows. If the API user specifies a geometry but fails
> to specify the number of required subwindows, we would assume a
> default (max supported) value, which may be incorrect. It's going to
> be really ugly. I think we should add the number of sub windows to the
> domain geometry. This parameter would be used in case of a window
> based iommu. So, a separate set_window API wouldn't be required.

I think this can all be implemented with the interface proposed here
together with a PAMU-specific DOMAIN_ATTR_PAMU_ENABLE attribute. Stuart
outlined in his RFC mail how this can be done. I don't consider this
ugly, it is rather a clean tradeoff between putting functionality into
the generic part of the IOMMU-API and putting it into the PAMU specific
part.

Regards,

	Joerg

RE: [PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute

[Sethi Varun-B16395]

DOMAIN_ATTR_PAMU_ENABLE is required to enable a particular DMA window. My point is about the domain geometry, which is incomplete in case of PAMU without the number of subwindows. Geometry parameters are used for initializing the PAMU window settings. Individual subwindows can only be enabled, once the PAMU window has been initialized using the geometry settings.

Regards
Varun


> -----Original Message-----
> From: Joerg Roedel [mailto:joro@8bytes.org]
> Sent: Tuesday, February 05, 2013 4:12 PM
> To: Sethi Varun-B16395
> Cc: Stuart Yoder; iommu@lists.linux-foundation.org; linux-
> kernel@vger.kernel.org; Wood Scott-B07421
> Subject: Re: [PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute
> 
> Hi,
> 
> On Tue, Feb 05, 2013 at 09:14:40AM +0000, Sethi Varun-B16395 wrote:
> > In case of PAMU, geometry would be meaningless without specifying the
> > number of subwindows. If the API user specifies a geometry but fails
> > to specify the number of required subwindows, we would assume a
> > default (max supported) value, which may be incorrect. It's going to
> > be really ugly. I think we should add the number of sub windows to the
> > domain geometry. This parameter would be used in case of a window
> > based iommu. So, a separate set_window API wouldn't be required.
> 
> I think this can all be implemented with the interface proposed here
> together with a PAMU-specific DOMAIN_ATTR_PAMU_ENABLE attribute. Stuart
> outlined in his RFC mail how this can be done. I don't consider this
> ugly, it is rather a clean tradeoff between putting functionality into
> the generic part of the IOMMU-API and putting it into the PAMU specific
> part.
> 
> Regards,
> 
> 	Joerg
> 
> 

Re: [PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute

[Joerg Roedel]

On Tue, Feb 05, 2013 at 10:52:03AM +0000, Sethi Varun-B16395 wrote:
> DOMAIN_ATTR_PAMU_ENABLE is required to enable a particular DMA window.
> My point is about the domain geometry, which is incomplete in case of
> PAMU without the number of subwindows. Geometry parameters are used
> for initializing the PAMU window settings. Individual subwindows can
> only be enabled, once the PAMU window has been initialized using the
> geometry settings.

I understand that. What you do is:

	/* Set geometry */
	set_attr(domain, DOMAIN_ATTR_GEOMETRY);

	/* Set number of PAMU subwindows */
	set_attr(domain, DOMAIN_ATTR_WINDOWS);

	/* Commit changes to hardware and enable the window */
	set_attr(domain, DOMAIN_ATTR_PAMU_ENABLE);

And I don't see any problem with that. The domain_attr interface was
introduced to cope with device specifics, we don't change global
interface data structures for that.


	Joerg

Re: [PATCH 4/5] iommu: Add domain window handling functions

[Joerg Roedel]

On Mon, Feb 04, 2013 at 05:31:51PM -0600, Stuart Yoder wrote:
> On Mon, Feb 4, 2013 at 12:56 PM, Joerg Roedel <joro@8bytes.org> wrote:
> > On Mon, Feb 04, 2013 at 12:10:51PM -0600, Stuart Yoder wrote:
> >> On Mon, Feb 4, 2013 at 7:18 AM, Joerg Roedel <joro@8bytes.org> wrote:
> >> > +static inline int iommu_domain_window_enable(struct iommu_domain *domain,
> >> > +                                            u32 wnd_nr, phys_addr_t paddr,
> >> > +                                            size_t size)
> >> > +{
> >> > +       return -ENODEV;
> >> > +}
> >> > +
> >> > +static inline void iommu_domain_window_disable(struct iommu_domain *domain,
> >> > +                                              u32 wnd_nr)
> >> > +{
> >> > +}
> >> > +
> >> >  static inline phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
> >> >                                              unsigned long iova)
> >> >  {
> >>
> >> This API looks workable.   The one change we need is that the size argument in
> >> the enable API needs to be 64 bits.   Our window sizes can exceed 4GB.
> >
> > Okay. So if your architecture supports sizes over 2^32 then size_t
> > probably is already 64bits, right?
> 
> No, on a 32-bit platform size_t would generally be 32-bits.  But the PAMU
> is independent of that.   I think we should just make it a u64.

Okay, I will change that. Will push the updated branch once Sethi agrees
on the interface changes.


	Joerg

RE: [PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute

[Sethi Varun-B16395]

> -----Original Message-----
> From: Joerg Roedel [mailto:joro@8bytes.org]
> Sent: Tuesday, February 05, 2013 6:36 PM
> To: Sethi Varun-B16395
> Cc: Stuart Yoder; iommu@lists.linux-foundation.org; linux-
> kernel@vger.kernel.org; Wood Scott-B07421
> Subject: Re: [PATCH 5/5] iommu: Add DOMAIN_ATTR_WINDOWS domain attribute
> 
> On Tue, Feb 05, 2013 at 10:52:03AM +0000, Sethi Varun-B16395 wrote:
> > DOMAIN_ATTR_PAMU_ENABLE is required to enable a particular DMA window.
> > My point is about the domain geometry, which is incomplete in case of
> > PAMU without the number of subwindows. Geometry parameters are used
> > for initializing the PAMU window settings. Individual subwindows can
> > only be enabled, once the PAMU window has been initialized using the
> > geometry settings.
> 
> I understand that. What you do is:
> 
> 	/* Set geometry */
> 	set_attr(domain, DOMAIN_ATTR_GEOMETRY);
> 
> 	/* Set number of PAMU subwindows */
> 	set_attr(domain, DOMAIN_ATTR_WINDOWS);
> 
> 	/* Commit changes to hardware and enable the window */
> 	set_attr(domain, DOMAIN_ATTR_PAMU_ENABLE);
> 
> And I don't see any problem with that. The domain_attr interface was
> introduced to cope with device specifics, we don't change global
> interface data structures for that.
> 
> 
[Sethi Varun-B16395] But, then it would be mandatory to set the DOMAIN_ATTR_WINDOWS attribute in case of PAMU. I guess that it would be implementation specific. I am fine with the API.

-Varun

Re: [PATCH 4/5] iommu: Add domain window handling functions

[Joerg Roedel]

On Mon, Feb 04, 2013 at 05:31:51PM -0600, Stuart Yoder wrote:
> On Mon, Feb 4, 2013 at 12:56 PM, Joerg Roedel <joro@8bytes.org> wrote:
> > Okay. So if your architecture supports sizes over 2^32 then size_t
> > probably is already 64bits, right?
> 
> No, on a 32-bit platform size_t would generally be 32-bits.  But the PAMU
> is independent of that.   I think we should just make it a u64.

Okay, you can find the patches in the 'core' branch of the iommu-tree.
They are also included in my next branch. You can use them now to base
the PAMU patches on it.


	Joerg

RE: [PATCH 4/5] iommu: Add domain window handling functions

[Sethi Varun-B16395]

Thanks Joerg.

> -----Original Message-----
> From: Joerg Roedel [mailto:joro@8bytes.org]
> Sent: Wednesday, February 06, 2013 3:56 PM
> To: Stuart Yoder
> Cc: Sethi Varun-B16395; iommu@lists.linux-foundation.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 4/5] iommu: Add domain window handling functions
> 
> On Mon, Feb 04, 2013 at 05:31:51PM -0600, Stuart Yoder wrote:
> > On Mon, Feb 4, 2013 at 12:56 PM, Joerg Roedel <joro@8bytes.org> wrote:
> > > Okay. So if your architecture supports sizes over 2^32 then size_t
> > > probably is already 64bits, right?
> >
> > No, on a 32-bit platform size_t would generally be 32-bits.  But the
> PAMU
> > is independent of that.   I think we should just make it a u64.
> 
> Okay, you can find the patches in the 'core' branch of the iommu-tree.
> They are also included in my next branch. You can use them now to base
> the PAMU patches on it.
> 
> 
> 	Joerg
> 
>