iommu/s390: Improve iommu-groups and add sysfs support

From: Joerg Roedel <[email protected]>

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 <[email protected]>

---
 drivers/iommu/s390-iommu.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

drivers/iommu/s390-iommu.c +5 -10

index 179e636..8788640 100644
@@ -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,
 };
 

Seems pretty straightforward, so

Reviewed-by Gerald Schaefer <[email protected]>

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:

From: Joerg Roedel <[email protected]>

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 <[email protected]>

---
 arch/s390/include/asm/pci.h |  7 +++++++
 arch/s390/pci/pci.c         |  5 +++++
 drivers/iommu/s390-iommu.c  | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)

arch/s390/include/asm/pci.h +7 -0

index 4e31866..a3f697e 100644
@@ -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 *);

arch/s390/pci/pci.c +1 -0

index 8051df1..4c13da6 100644
@@ -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);

arch/s390/pci/pci.c +0 -0

 	mutex_init(&zdev->lock);
 	if (zdev->state == ZPCI_FN_STATE_CONFIGURED) {
 		rc = zpci_enable_device(zdev);

drivers/iommu/s390-iommu.c +35 -0

index 8788640..85f3bc5 100644
@@ -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,

arch/s390/pci/pci.c int zpci_create_device(struct zpci_dev *zdev) · line 827

Quoted 4 lines · On Thu, 15 Jun 2017 15:11:52 +0200 Joerg Roedel…

arch/s390/pci/pci.c +4 -0

+	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?

Quoted 10 lines · Gerald Schaefer

arch/s390/pci/pci.c int zpci_create_device(struct zpci_dev *zdev) · line 827

Quoted 4 lines · On Thu, 15 Jun 2017 15:11:52 +0200 Joerg Roedel…

arch/s390/pci/pci.c +4 -0

+	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.

Quoted 8 lines

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 :)