[PATCH 0/2 v2] iommu/s390: Improve iommu-groups and add sysfs support

[PATCH 0/2 v2] iommu/s390: Improve iommu-groups and add sysfs support

[Joerg Roedel]

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

[PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device()

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

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-l3A5Bk7waGM@public.gmane.org>
---
 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

Re: [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device()

[Gerald Schaefer]

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,
>  };
> 

Re: [PATCH 1/2] iommu/s390: Use iommu_group_get_for_dev() in s390_iommu_add_device()

[Joerg Roedel]

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-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>

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

[PATCH 2/2] iommu/s390: Add support for iommu_device handling

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

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-l3A5Bk7waGM@public.gmane.org>
---
 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

Re: [PATCH 2/2] iommu/s390: Add support for iommu_device handling

[Gerald Schaefer]

On Thu, 15 Jun 2017 15:11:52 +0200
Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:

> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> 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-l3A5Bk7waGM@public.gmane.org>
> ---
>  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,

Re: [PATCH 2/2] iommu/s390: Add support for iommu_device handling

[Joerg Roedel]

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-zLv9SwRftAIdnm+yROfE0A@public.gmane.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

Re: [PATCH 2/2] iommu/s390: Add support for iommu_device handling

[Gerald Schaefer]

On Tue, 27 Jun 2017 17:28:06 +0200
Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.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-zLv9SwRftAIdnm+yROfE0A@public.gmane.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