linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2 v2] iommu/s390: Improve iommu-groups and add sysfs support
@ 2017-06-15 13:11 Joerg Roedel
  2017-06-15 13:11 ` [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device() Joerg Roedel
  2017-06-15 13:11 ` [PATCH 2/2] iommu/s390: Add support for iommu_device handling Joerg Roedel
  0 siblings, 2 replies; 10+ messages in thread
From: Joerg Roedel @ 2017-06-15 13:11 UTC (permalink / raw)
  To: Sebastian Ott, Gerald Schaefer; +Cc: iommu, linux-kernel, joro, jroedel

Hey,

here are two patches for the s390 iommu driver. The first
one optimizes iommu-group creation by making use of
iommu_group_get_for_dev().

The second one adds support for the iommu_device_register
interface and with that also sysfs support.

Any comments and testing appreciated.

Thanks,

	Joerg

Joerg Roedel (2):
  iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device()
  iommu/s390: Add support for iommu_device handling

 arch/s390/include/asm/pci.h |  7 +++++++
 arch/s390/pci/pci.c         |  5 +++++
 drivers/iommu/s390-iommu.c  | 50 ++++++++++++++++++++++++++++++++++++---------
 3 files changed, 52 insertions(+), 10 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device()
  2017-06-15 13:11 [PATCH 0/2 v2] iommu/s390: Improve iommu-groups and add sysfs support Joerg Roedel
@ 2017-06-15 13:11 ` Joerg Roedel
  2017-06-16 17:33   ` Gerald Schaefer
  2017-06-15 13:11 ` [PATCH 2/2] iommu/s390: Add support for iommu_device handling Joerg Roedel
  1 sibling, 1 reply; 10+ messages in thread
From: Joerg Roedel @ 2017-06-15 13:11 UTC (permalink / raw)
  To: Sebastian Ott, Gerald Schaefer; +Cc: iommu, linux-kernel, joro, jroedel

From: Joerg Roedel <jroedel@suse.de>

The iommu_group_get_for_dev() function also attaches the
device to its group, so this code doesn't need to be in the
iommu driver.

Further by using this function the driver can make use of
default domains in the future.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/s390-iommu.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 179e636..8788640 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -165,20 +165,14 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
 
 static int s390_iommu_add_device(struct device *dev)
 {
-	struct iommu_group *group;
-	int rc;
+	struct iommu_group *group = iommu_group_get_for_dev(dev);
 
-	group = iommu_group_get(dev);
-	if (!group) {
-		group = iommu_group_alloc();
-		if (IS_ERR(group))
-			return PTR_ERR(group);
-	}
+	if (IS_ERR(group))
+		return PTR_ERR(group);
 
-	rc = iommu_group_add_device(group, dev);
 	iommu_group_put(group);
 
-	return rc;
+	return 0;
 }
 
 static void s390_iommu_remove_device(struct device *dev)
@@ -344,6 +338,7 @@ static struct iommu_ops s390_iommu_ops = {
 	.iova_to_phys = s390_iommu_iova_to_phys,
 	.add_device = s390_iommu_add_device,
 	.remove_device = s390_iommu_remove_device,
+	.device_group = generic_device_group,
 	.pgsize_bitmap = S390_IOMMU_PGSIZES,
 };
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] iommu/s390: Add support for iommu_device handling
  2017-06-15 13:11 [PATCH 0/2 v2] iommu/s390: Improve iommu-groups and add sysfs support Joerg Roedel
  2017-06-15 13:11 ` [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device() Joerg Roedel
@ 2017-06-15 13:11 ` Joerg Roedel
  2017-06-19 15:02   ` Gerald Schaefer
  1 sibling, 1 reply; 10+ messages in thread
From: Joerg Roedel @ 2017-06-15 13:11 UTC (permalink / raw)
  To: Sebastian Ott, Gerald Schaefer; +Cc: iommu, linux-kernel, joro, jroedel

From: Joerg Roedel <jroedel@suse.de>

Add support for the iommu_device_register interface to make
the s390 hardware iommus visible to the iommu core and in
sysfs.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/s390/include/asm/pci.h |  7 +++++++
 arch/s390/pci/pci.c         |  5 +++++
 drivers/iommu/s390-iommu.c  | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 4e31866..a3f697e 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -8,6 +8,7 @@
 
 #include <linux/pci.h>
 #include <linux/mutex.h>
+#include <linux/iommu.h>
 #include <asm-generic/pci.h>
 #include <asm/pci_clp.h>
 #include <asm/pci_debug.h>
@@ -123,6 +124,8 @@ struct zpci_dev {
 	unsigned long	iommu_pages;
 	unsigned int	next_bit;
 
+	struct iommu_device iommu_dev;  /* IOMMU core handle */
+
 	char res_name[16];
 	struct zpci_bar_struct bars[PCI_BAR_COUNT];
 
@@ -173,6 +176,10 @@ int clp_add_pci_device(u32, u32, int);
 int clp_enable_fh(struct zpci_dev *, u8);
 int clp_disable_fh(struct zpci_dev *);
 
+/* IOMMU Interface */
+int zpci_init_iommu(struct zpci_dev *zdev);
+void zpci_destroy_iommu(struct zpci_dev *zdev);
+
 #ifdef CONFIG_PCI
 /* Error handling and recovery */
 void zpci_event_error(void *);
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 8051df1..4c13da6 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -749,6 +749,7 @@ void pcibios_remove_bus(struct pci_bus *bus)
 
 	zpci_exit_slot(zdev);
 	zpci_cleanup_bus_resources(zdev);
+	zpci_destroy_iommu(zdev);
 	zpci_free_domain(zdev);
 
 	spin_lock(&zpci_list_lock);
@@ -820,6 +821,10 @@ int zpci_create_device(struct zpci_dev *zdev)
 	if (rc)
 		goto out;
 
+	rc = zpci_init_iommu(zdev);
+	if (rc)
+		goto out_free;
+
 	mutex_init(&zdev->lock);
 	if (zdev->state == ZPCI_FN_STATE_CONFIGURED) {
 		rc = zpci_enable_device(zdev);
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 8788640..85f3bc5 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -18,6 +18,8 @@
  */
 #define S390_IOMMU_PGSIZES	(~0xFFFUL)
 
+static struct iommu_ops s390_iommu_ops;
+
 struct s390_domain {
 	struct iommu_domain	domain;
 	struct list_head	devices;
@@ -166,11 +168,13 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
 static int s390_iommu_add_device(struct device *dev)
 {
 	struct iommu_group *group = iommu_group_get_for_dev(dev);
+	struct zpci_dev *zdev = to_pci_dev(dev)->sysdata;
 
 	if (IS_ERR(group))
 		return PTR_ERR(group);
 
 	iommu_group_put(group);
+	iommu_device_link(&zdev->iommu_dev, dev);
 
 	return 0;
 }
@@ -197,6 +201,7 @@ static void s390_iommu_remove_device(struct device *dev)
 			s390_iommu_detach_device(domain, dev);
 	}
 
+	iommu_device_unlink(&zdev->iommu_dev, dev);
 	iommu_group_remove_device(dev);
 }
 
@@ -327,6 +332,36 @@ static size_t s390_iommu_unmap(struct iommu_domain *domain,
 	return size;
 }
 
+int zpci_init_iommu(struct zpci_dev *zdev)
+{
+	int rc = 0;
+
+	rc = iommu_device_sysfs_add(&zdev->iommu_dev, NULL, NULL,
+				    "s390-iommu.%08x", zdev->fid);
+	if (rc)
+		goto out_err;
+
+	iommu_device_set_ops(&zdev->iommu_dev, &s390_iommu_ops);
+
+	rc = iommu_device_register(&zdev->iommu_dev);
+	if (rc)
+		goto out_sysfs;
+
+	return 0;
+
+out_sysfs:
+	iommu_device_sysfs_remove(&zdev->iommu_dev);
+
+out_err:
+	return rc;
+}
+
+void zpci_destroy_iommu(struct zpci_dev *zdev)
+{
+	iommu_device_unregister(&zdev->iommu_dev);
+	iommu_device_sysfs_remove(&zdev->iommu_dev);
+}
+
 static struct iommu_ops s390_iommu_ops = {
 	.capable = s390_iommu_capable,
 	.domain_alloc = s390_domain_alloc,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device()
  2017-06-15 13:11 ` [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device() Joerg Roedel
@ 2017-06-16 17:33   ` Gerald Schaefer
  2017-06-27 15:16     ` Joerg Roedel
  0 siblings, 1 reply; 10+ messages in thread
From: Gerald Schaefer @ 2017-06-16 17:33 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: Sebastian Ott, iommu, linux-kernel, jroedel

On Thu, 15 Jun 2017 15:11:51 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> From: Joerg Roedel <jroedel@suse.de>
> 
> The iommu_group_get_for_dev() function also attaches the
> device to its group, so this code doesn't need to be in the
> iommu driver.
> 
> Further by using this function the driver can make use of
> default domains in the future.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

Seems pretty straightforward, so
Reviewed-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>

However, looking at iommu_group_get_for_dev(), I wonder if the
generic_device_group() always returns the right thing, but that
would be independent from this patch.

With generic_device_group() returning NULL in case the allocation failed,
this part of iommu_group_get_for_dev() would then happily dereference the
NULL pointer, because IS_ERR(group) would be false:

        if (ops && ops->device_group)
                group = ops->device_group(dev);

        if (IS_ERR(group))
                return group;

        /*
         * Try to allocate a default domain - needs support from the
         * IOMMU driver.
         */
        if (!group->default_domain) {

The same is true for pci_device_group(), which also returns NULL in case
of allocation failure. I guess both functions should just return the
group pointer from iommu_group_alloc() directly, which already would
contain an appropriate ERR_PTR, but never NULL.

What do you think?

> ---
>  drivers/iommu/s390-iommu.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index 179e636..8788640 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -165,20 +165,14 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
> 
>  static int s390_iommu_add_device(struct device *dev)
>  {
> -	struct iommu_group *group;
> -	int rc;
> +	struct iommu_group *group = iommu_group_get_for_dev(dev);
> 
> -	group = iommu_group_get(dev);
> -	if (!group) {
> -		group = iommu_group_alloc();
> -		if (IS_ERR(group))
> -			return PTR_ERR(group);
> -	}
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
> 
> -	rc = iommu_group_add_device(group, dev);
>  	iommu_group_put(group);
> 
> -	return rc;
> +	return 0;
>  }
> 
>  static void s390_iommu_remove_device(struct device *dev)
> @@ -344,6 +338,7 @@ static struct iommu_ops s390_iommu_ops = {
>  	.iova_to_phys = s390_iommu_iova_to_phys,
>  	.add_device = s390_iommu_add_device,
>  	.remove_device = s390_iommu_remove_device,
> +	.device_group = generic_device_group,
>  	.pgsize_bitmap = S390_IOMMU_PGSIZES,
>  };
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] iommu/s390: Add support for iommu_device handling
  2017-06-15 13:11 ` [PATCH 2/2] iommu/s390: Add support for iommu_device handling Joerg Roedel
@ 2017-06-19 15:02   ` Gerald Schaefer
  2017-06-27 15:28     ` Joerg Roedel
  0 siblings, 1 reply; 10+ messages in thread
From: Gerald Schaefer @ 2017-06-19 15:02 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: Sebastian Ott, iommu, linux-kernel, jroedel, gerald.schaefer

On Thu, 15 Jun 2017 15:11:52 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> From: Joerg Roedel <jroedel@suse.de>
> 
> Add support for the iommu_device_register interface to make
> the s390 hardware iommus visible to the iommu core and in
> sysfs.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/s390/include/asm/pci.h |  7 +++++++
>  arch/s390/pci/pci.c         |  5 +++++
>  drivers/iommu/s390-iommu.c  | 35 +++++++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 4e31866..a3f697e 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -8,6 +8,7 @@
> 
>  #include <linux/pci.h>
>  #include <linux/mutex.h>
> +#include <linux/iommu.h>
>  #include <asm-generic/pci.h>
>  #include <asm/pci_clp.h>
>  #include <asm/pci_debug.h>
> @@ -123,6 +124,8 @@ struct zpci_dev {
>  	unsigned long	iommu_pages;
>  	unsigned int	next_bit;
> 
> +	struct iommu_device iommu_dev;  /* IOMMU core handle */
> +
>  	char res_name[16];
>  	struct zpci_bar_struct bars[PCI_BAR_COUNT];
> 
> @@ -173,6 +176,10 @@ int clp_add_pci_device(u32, u32, int);
>  int clp_enable_fh(struct zpci_dev *, u8);
>  int clp_disable_fh(struct zpci_dev *);
> 
> +/* IOMMU Interface */
> +int zpci_init_iommu(struct zpci_dev *zdev);
> +void zpci_destroy_iommu(struct zpci_dev *zdev);
> +
>  #ifdef CONFIG_PCI
>  /* Error handling and recovery */
>  void zpci_event_error(void *);
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 8051df1..4c13da6 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -749,6 +749,7 @@ void pcibios_remove_bus(struct pci_bus *bus)
> 
>  	zpci_exit_slot(zdev);
>  	zpci_cleanup_bus_resources(zdev);
> +	zpci_destroy_iommu(zdev);
>  	zpci_free_domain(zdev);
> 
>  	spin_lock(&zpci_list_lock);
> @@ -820,6 +821,10 @@ int zpci_create_device(struct zpci_dev *zdev)
>  	if (rc)
>  		goto out;
> 
> +	rc = zpci_init_iommu(zdev);
> +	if (rc)
> +		goto out_free;
> +

After this point, there are two options for failure (zpci_enable_device +
zpci_scan_bus), but missing error handling with an appropriate call to
zpci_destroy_iommu().

I must admit that I do not understand what these sysfs attributes are
used for, and by whom and when. Is it really necessary/correct to register
them (including the s390_iommu_ops) _before_ the zpci_dev is registered
to the bus (zpci_scan_bus -> pci_scan_root_bus/pci_bus_add_devices)?

What is the expected life cycle for the attributes, and are they already
needed when a device is still under DMA API access, or even before it is
added to the PCI bus?

>  	mutex_init(&zdev->lock);
>  	if (zdev->state == ZPCI_FN_STATE_CONFIGURED) {
>  		rc = zpci_enable_device(zdev);
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index 8788640..85f3bc5 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -18,6 +18,8 @@
>   */
>  #define S390_IOMMU_PGSIZES	(~0xFFFUL)
> 
> +static struct iommu_ops s390_iommu_ops;
> +
>  struct s390_domain {
>  	struct iommu_domain	domain;
>  	struct list_head	devices;
> @@ -166,11 +168,13 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
>  static int s390_iommu_add_device(struct device *dev)
>  {
>  	struct iommu_group *group = iommu_group_get_for_dev(dev);
> +	struct zpci_dev *zdev = to_pci_dev(dev)->sysdata;
> 
>  	if (IS_ERR(group))
>  		return PTR_ERR(group);
> 
>  	iommu_group_put(group);
> +	iommu_device_link(&zdev->iommu_dev, dev);
> 
>  	return 0;
>  }
> @@ -197,6 +201,7 @@ static void s390_iommu_remove_device(struct device *dev)
>  			s390_iommu_detach_device(domain, dev);
>  	}
> 
> +	iommu_device_unlink(&zdev->iommu_dev, dev);
>  	iommu_group_remove_device(dev);
>  }
> 
> @@ -327,6 +332,36 @@ static size_t s390_iommu_unmap(struct iommu_domain *domain,
>  	return size;
>  }
> 
> +int zpci_init_iommu(struct zpci_dev *zdev)
> +{
> +	int rc = 0;
> +
> +	rc = iommu_device_sysfs_add(&zdev->iommu_dev, NULL, NULL,
> +				    "s390-iommu.%08x", zdev->fid);
> +	if (rc)
> +		goto out_err;
> +
> +	iommu_device_set_ops(&zdev->iommu_dev, &s390_iommu_ops);
> +
> +	rc = iommu_device_register(&zdev->iommu_dev);
> +	if (rc)
> +		goto out_sysfs;
> +
> +	return 0;
> +
> +out_sysfs:
> +	iommu_device_sysfs_remove(&zdev->iommu_dev);
> +
> +out_err:
> +	return rc;
> +}
> +
> +void zpci_destroy_iommu(struct zpci_dev *zdev)
> +{
> +	iommu_device_unregister(&zdev->iommu_dev);
> +	iommu_device_sysfs_remove(&zdev->iommu_dev);
> +}
> +
>  static struct iommu_ops s390_iommu_ops = {
>  	.capable = s390_iommu_capable,
>  	.domain_alloc = s390_domain_alloc,

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device()
  2017-06-16 17:33   ` Gerald Schaefer
@ 2017-06-27 15:16     ` Joerg Roedel
  0 siblings, 0 replies; 10+ messages in thread
From: Joerg Roedel @ 2017-06-27 15:16 UTC (permalink / raw)
  To: Gerald Schaefer; +Cc: Sebastian Ott, iommu, linux-kernel, jroedel

Hi Gerald,

sorry for the delay. Answers inline.

On Fri, Jun 16, 2017 at 07:33:01PM +0200, Gerald Schaefer wrote:
> Seems pretty straightforward, so
> Reviewed-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>

Thanks, I add it to the patch.

> With generic_device_group() returning NULL in case the allocation failed,
> this part of iommu_group_get_for_dev() would then happily dereference the
> NULL pointer, because IS_ERR(group) would be false:

Yeah, you are right, this is a bug. I'll send a patch to fix this
shortly. I don't remember whether there was a good reason to drop the
error values in the device_group call-backs. Probably there is none and
I can just pass the error value up instead of NULLing them out.



	Joerg

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] iommu/s390: Add support for iommu_device handling
  2017-06-19 15:02   ` Gerald Schaefer
@ 2017-06-27 15:28     ` Joerg Roedel
  2017-06-29 13:27       ` Gerald Schaefer
  0 siblings, 1 reply; 10+ messages in thread
From: Joerg Roedel @ 2017-06-27 15:28 UTC (permalink / raw)
  To: Gerald Schaefer; +Cc: Sebastian Ott, iommu, linux-kernel, jroedel

On Mon, Jun 19, 2017 at 05:02:19PM +0200, Gerald Schaefer wrote:
> On Thu, 15 Jun 2017 15:11:52 +0200
> Joerg Roedel <joro@8bytes.org> wrote:
> > +	rc = zpci_init_iommu(zdev);
> > +	if (rc)
> > +		goto out_free;
> > +
> 
> After this point, there are two options for failure (zpci_enable_device +
> zpci_scan_bus), but missing error handling with an appropriate call to
> zpci_destroy_iommu().

Right, I'll fix that in the next version.

> I must admit that I do not understand what these sysfs attributes are
> used for, and by whom and when. Is it really necessary/correct to register
> them (including the s390_iommu_ops) _before_ the zpci_dev is registered
> to the bus (zpci_scan_bus -> pci_scan_root_bus/pci_bus_add_devices)?
> 
> What is the expected life cycle for the attributes, and are they already
> needed when a device is still under DMA API access, or even before it is
> added to the PCI bus?

The sysfs attributes are used by user-space for topology detection. A
user-space program using VFIO needs to know which PCI-devices it needs
to assign to the VFIO driver in order to gain access.

On s390 this probably doesn't matter much, as the real PCI topology is
not exposed, but it matters on other architectures. The purpose of this
patch is not so much the sysfs attributes. Its a step to convert all
IOMMU drivers to use the iommu_device_register() interface. Goal is that
the iommu core code knows about individual hardware iommus and the
devices behind it.

When this is implemented in all iommu drivers, we can start moving more
code out of the drivers into the iommu core. This also probably doesn't
matter much for s390, but all iommu drivers need to use this interface
before we can move on. The sysfs-stuff is just a by-product of that.

So to move on with that, it would be good to get an Acked-by on this
patch from you guys once you think I fixed everything :)


Regards,

	Joerg

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] iommu/s390: Add support for iommu_device handling
  2017-06-27 15:28     ` Joerg Roedel
@ 2017-06-29 13:27       ` Gerald Schaefer
  0 siblings, 0 replies; 10+ messages in thread
From: Gerald Schaefer @ 2017-06-29 13:27 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: Sebastian Ott, iommu, linux-kernel, jroedel

On Tue, 27 Jun 2017 17:28:06 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> On Mon, Jun 19, 2017 at 05:02:19PM +0200, Gerald Schaefer wrote:
> > On Thu, 15 Jun 2017 15:11:52 +0200
> > Joerg Roedel <joro@8bytes.org> wrote:
> > > +	rc = zpci_init_iommu(zdev);
> > > +	if (rc)
> > > +		goto out_free;
> > > +
> > 
> > After this point, there are two options for failure (zpci_enable_device +
> > zpci_scan_bus), but missing error handling with an appropriate call to
> > zpci_destroy_iommu().
> 
> Right, I'll fix that in the next version.
> 
> > I must admit that I do not understand what these sysfs attributes are
> > used for, and by whom and when. Is it really necessary/correct to register
> > them (including the s390_iommu_ops) _before_ the zpci_dev is registered
> > to the bus (zpci_scan_bus -> pci_scan_root_bus/pci_bus_add_devices)?
> > 
> > What is the expected life cycle for the attributes, and are they already
> > needed when a device is still under DMA API access, or even before it is
> > added to the PCI bus?
> 
> The sysfs attributes are used by user-space for topology detection. A
> user-space program using VFIO needs to know which PCI-devices it needs
> to assign to the VFIO driver in order to gain access.
> 
> On s390 this probably doesn't matter much, as the real PCI topology is
> not exposed, but it matters on other architectures. The purpose of this
> patch is not so much the sysfs attributes. Its a step to convert all
> IOMMU drivers to use the iommu_device_register() interface. Goal is that
> the iommu core code knows about individual hardware iommus and the
> devices behind it.
> 
> When this is implemented in all iommu drivers, we can start moving more
> code out of the drivers into the iommu core. This also probably doesn't
> matter much for s390, but all iommu drivers need to use this interface
> before we can move on. The sysfs-stuff is just a by-product of that.
> 
> So to move on with that, it would be good to get an Acked-by on this
> patch from you guys once you think I fixed everything :)

Ok, thanks for the explanation. So it should not be a problem if the
attributes are registered in zpci_create_device() before the device is
registered to the bus, especially since they do not provide any manipulation
function but only topology information.

BTW, I get the following new attributes with this patch:
/sys/devices/pci0000:00/0000:00:00.0/iommu
/sys/devices/virtual/iommu
/sys/devices/virtual/iommu/s390-iommu.00000012
/sys/class/iommu/s390-iommu.00000012

Regards,
Gerald

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] iommu/s390: Add support for iommu_device handling
  2017-04-27 15:28 ` [PATCH 2/2] iommu/s390: Add support for iommu_device handling Joerg Roedel
@ 2017-04-28 23:02   ` kbuild test robot
  0 siblings, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2017-04-28 23:02 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: kbuild-all, Sebastian Ott, Gerald Schaefer, iommu, linux-kernel,
	Joerg Roedel

[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]

Hi Joerg,

[auto build test ERROR on s390/features]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Joerg-Roedel/iommu-s390-Fix-iommu-groups-and-add-sysfs-support/20170429-003440
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All errors (new ones prefixed by >>):

   In file included from include/linux/pci.h:1627:0,
                    from include/trace/events/iommu.h:14,
                    from include/linux/iommu.h:27,
                    from drivers/s390/cio/vfio_ccw_cp.c:12:
>> arch/s390/include/asm/pci.h:127:22: error: field 'iommu_dev' has incomplete type
     struct iommu_device iommu_dev;  /* IOMMU core handle */
                         ^~~~~~~~~

vim +/iommu_dev +127 arch/s390/include/asm/pci.h

   121		unsigned long	*iommu_bitmap;
   122		unsigned long	*lazy_bitmap;
   123		unsigned long	iommu_size;
   124		unsigned long	iommu_pages;
   125		unsigned int	next_bit;
   126	
 > 127		struct iommu_device iommu_dev;  /* IOMMU core handle */
   128		struct iommu_group *group;	/* IOMMU group for all devices behind this zdev */
   129	
   130		char res_name[16];

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45141 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] iommu/s390: Add support for iommu_device handling
  2017-04-27 15:28 [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support Joerg Roedel
@ 2017-04-27 15:28 ` Joerg Roedel
  2017-04-28 23:02   ` kbuild test robot
  0 siblings, 1 reply; 10+ messages in thread
From: Joerg Roedel @ 2017-04-27 15:28 UTC (permalink / raw)
  To: Sebastian Ott, Gerald Schaefer; +Cc: iommu, linux-kernel, Joerg Roedel

From: Joerg Roedel <jroedel@suse.de>

Add support for the iommu_device_register interface to make
the s390 hardware iommus visible to the iommu core and in
sysfs.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/s390/include/asm/pci.h |  1 +
 drivers/iommu/s390-iommu.c  | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 045665d..8c071af 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -124,6 +124,7 @@ struct zpci_dev {
 	unsigned long	iommu_pages;
 	unsigned int	next_bit;
 
+	struct iommu_device iommu_dev;  /* IOMMU core handle */
 	struct iommu_group *group;	/* IOMMU group for all devices behind this zdev */
 
 	char res_name[16];
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index cad3ad0..ec7f5e4 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -18,6 +18,8 @@
  */
 #define S390_IOMMU_PGSIZES	(~0xFFFUL)
 
+static struct iommu_ops s390_iommu_ops;
+
 struct s390_domain {
 	struct iommu_domain	domain;
 	struct list_head	devices;
@@ -173,10 +175,13 @@ static struct iommu_group *s390_iommu_device_group(struct device *dev)
 static int s390_iommu_add_device(struct device *dev)
 {
 	struct iommu_group *group = iommu_group_get_for_dev(dev);
+	struct zpci_dev *zdev = to_pci_dev(dev)->sysdata;
+
 	if (IS_ERR(group))
 		return PTR_ERR(group);
 
 	iommu_group_put(group);
+	iommu_device_link(&zdev->iommu_dev, dev);
 
 	return 0;
 }
@@ -203,6 +208,7 @@ static void s390_iommu_remove_device(struct device *dev)
 			s390_iommu_detach_device(domain, dev);
 	}
 
+	iommu_device_unlink(&zdev->iommu_dev, dev);
 	iommu_group_remove_device(dev);
 }
 
@@ -342,13 +348,37 @@ int zpci_init_iommu(struct zpci_dev *zdev)
 	if (IS_ERR(zdev->group)) {
 		rc = PTR_ERR(zdev->group);
 		zdev->group = NULL;
+		goto out_err;
 	}
 
+	rc = iommu_device_sysfs_add(&zdev->iommu_dev, NULL, NULL,
+				    "s390-iommu.%08x", zdev->fid);
+	if (rc)
+		goto out_group;
+
+	iommu_device_set_ops(&zdev->iommu_dev, &s390_iommu_ops);
+
+	rc = iommu_device_register(&zdev->iommu_dev);
+	if (rc)
+		goto out_sysfs;
+
+	return 0;
+
+out_sysfs:
+	iommu_device_sysfs_remove(&zdev->iommu_dev);
+
+out_group:
+	iommu_group_put(zdev->group);
+	zdev->group = NULL;
+
+out_err:
 	return rc;
 }
 
 void zpci_destroy_iommu(struct zpci_dev *zdev)
 {
+	iommu_device_unregister(&zdev->iommu_dev);
+	iommu_device_sysfs_remove(&zdev->iommu_dev);
 	iommu_group_put(zdev->group);
 	zdev->group = NULL;
 }
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-06-29 13:27 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-15 13:11 [PATCH 0/2 v2] iommu/s390: Improve iommu-groups and add sysfs support Joerg Roedel
2017-06-15 13:11 ` [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device() Joerg Roedel
2017-06-16 17:33   ` Gerald Schaefer
2017-06-27 15:16     ` Joerg Roedel
2017-06-15 13:11 ` [PATCH 2/2] iommu/s390: Add support for iommu_device handling Joerg Roedel
2017-06-19 15:02   ` Gerald Schaefer
2017-06-27 15:28     ` Joerg Roedel
2017-06-29 13:27       ` Gerald Schaefer
  -- strict thread matches above, loose matches on Subject: below --
2017-04-27 15:28 [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support Joerg Roedel
2017-04-27 15:28 ` [PATCH 2/2] iommu/s390: Add support for iommu_device handling Joerg Roedel
2017-04-28 23:02   ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).