[PATCH v2 0/3] Remove iommu_group_remove_device() from fsl

With POWER SPAPR now having a real iommu driver and using the normal group
lifecycle stuff fixing FSL will leave only VFIO's no-iommu support as a
user for the iommu_group_add/remove_device() calls. This will help
simplify the understanding of what the core code should be doing for these
functions.

Fix FSL to not need to call iommu_group_remove_device() at all.

v2:
 - Change the approach to use driver_managed_dma
 - Really simplify fsl_pamu_device_group() and just put everything in one
   function
 - New patch to make missing OF properties a probe failure
v1: https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_jgg@nvidia.com

Jason Gunthorpe (3):
  iommu/fsl: Always allocate a group for non-pci devices
  iommu/fsl: Move ENODEV to fsl_pamu_probe_device()
  iommu/fsl: Use driver_managed_dma to allow VFIO to work

 arch/powerpc/sysdev/fsl_pci.c   |   1 +
 drivers/iommu/fsl_pamu_domain.c | 123 +++++++++-----------------------
 2 files changed, 36 insertions(+), 88 deletions(-)


base-commit: 1421774b874bfd5fd1b2b05b59b67c0c5e0d513e
-- 
2.40.1

fsl_pamu_device_group() is only called if dev->iommu_group is NULL, so
iommu_group_get() always returns NULL. Remove this test and just allocate
a group. Call generic_device_group() for this like the other drivers.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/fsl_pamu_domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index bce37229709965..cd0c60b402154f 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c

@@ -437,7 +437,7 @@ static struct iommu_group *fsl_pamu_device_group(struct device *dev)
 	if (dev_is_pci(dev))
 		group = get_pci_device_group(to_pci_dev(dev));
 	else if (of_get_property(dev->of_node, "fsl,liodn", &len))
-		group = get_device_iommu_group(dev);
+		return generic_device_group(dev);
 
 	return group;
 }

-- 
2.40.1

The expectation is for the probe op to return ENODEV if the iommu is not
able to support the device. Move the check for fsl,liodn to
fsl_pamu_probe_device() simplify fsl_pamu_device_group()

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/fsl_pamu_domain.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index cd0c60b402154f..d0683daa900fa5 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c

@@ -427,23 +427,28 @@ static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
 
 static struct iommu_group *fsl_pamu_device_group(struct device *dev)
 {
-	struct iommu_group *group = ERR_PTR(-ENODEV);
-	int len;
-
 	/*
 	 * For platform devices we allocate a separate group for
 	 * each of the devices.
 	 */
-	if (dev_is_pci(dev))
-		group = get_pci_device_group(to_pci_dev(dev));
-	else if (of_get_property(dev->of_node, "fsl,liodn", &len))
+	if (!dev_is_pci(dev))
 		return generic_device_group(dev);
 
-	return group;
+	return get_pci_device_group(to_pci_dev(dev));
 }
 
 static struct iommu_device *fsl_pamu_probe_device(struct device *dev)
 {
+	int len;
+
+	/*
+	 * uboot must fill the fsl,liodn for platform devices to be supported by
+	 * the iommu.
+	 */
+	if (!dev_is_pci(dev) &&
+	    !of_get_property(dev->of_node, "fsl,liodn", &len))
+		return ERR_PTR(-ENODEV);
+
 	return &pamu_iommu;
 }
 

-- 
2.40.1

The FSL driver is mangling the iommu_groups to not have a group for its
PCI bridge/controller (eg the thing passed to fsl_add_bridge()). Robin
says this is so FSL could work with VFIO which would be blocked by having
a probed driver on the platform_device in the same group. This is
supported by comments from FSL:

 https://lore.kernel.org/all/C5ECD7A89D1DC44195F34B25E172658D459471@039-SN2MPN1-013.039d.mgd.msft.net

 ..  PCIe devices share the same device group as the PCI controller. This
  becomes a problem while assigning the devices to the guest, as you are
  required to unbind all the PCIe devices including the controller from the
  host. PCIe controller can't be unbound from the host, so we simply delete
  the controller iommu_group.

However, today, we use driver_managed_dma to allow PCI infrastructure
devices that are 'security safe' to co-exist in groups and still allow
VFIO to work. Set this flag for the fsl_pci_driver.

Change fsl_pamu_device_group() so that it no longer removes the controller
from any groups. For check_pci_ctl_endpt_part() mode this creates an extra
group that contains only the controller.

Otherwise force the controller's single group to be the group of all the
PCI devices on the controller's hose. VFIO continues to work because of
driver_managed_dma.

Remove the iommu_group_remove_device() calls from fsl_pamu and lightly
restructure its fsl_pamu_device_group() function.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 arch/powerpc/sysdev/fsl_pci.c   |   1 +
 drivers/iommu/fsl_pamu_domain.c | 108 ++++++++------------------------
 2 files changed, 26 insertions(+), 83 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index b7232c46b24481..6daf620b63a4d5 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c

@@ -1353,6 +1353,7 @@ static struct platform_driver fsl_pci_driver = {
 		.of_match_table = pci_ids,
 	},
 	.probe = fsl_pci_probe,
+	.driver_managed_dma = true,
 };
 
 static int __init fsl_pci_init(void)

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index d0683daa900fa5..4ac0e247ec2b51 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c

@@ -334,17 +334,6 @@ int fsl_pamu_configure_l1_stash(struct iommu_domain *domain, u32 cpu)
 	return ret;
 }
 
-static struct iommu_group *get_device_iommu_group(struct device *dev)
-{
-	struct iommu_group *group;
-
-	group = iommu_group_get(dev);
-	if (!group)
-		group = iommu_group_alloc();
-
-	return group;
-}
-
 static  bool check_pci_ctl_endpt_part(struct pci_controller *pci_ctl)
 {
 	u32 version;

@@ -356,85 +345,38 @@ static  bool check_pci_ctl_endpt_part(struct pci_controller *pci_ctl)
 	return version >= 0x204;
 }
 
-/* Get iommu group information from peer devices or devices on the parent bus */
-static struct iommu_group *get_shared_pci_device_group(struct pci_dev *pdev)
-{
-	struct pci_dev *tmp;
-	struct iommu_group *group;
-	struct pci_bus *bus = pdev->bus;
-
-	/*
-	 * Traverese the pci bus device list to get
-	 * the shared iommu group.
-	 */
-	while (bus) {
-		list_for_each_entry(tmp, &bus->devices, bus_list) {
-			if (tmp == pdev)
-				continue;
-			group = iommu_group_get(&tmp->dev);
-			if (group)
-				return group;
-		}
-
-		bus = bus->parent;
-	}
-
-	return NULL;
-}
-
-static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
-{
-	struct pci_controller *pci_ctl;
-	bool pci_endpt_partitioning;
-	struct iommu_group *group = NULL;
-
-	pci_ctl = pci_bus_to_host(pdev->bus);
-	pci_endpt_partitioning = check_pci_ctl_endpt_part(pci_ctl);
-	/* We can partition PCIe devices so assign device group to the device */
-	if (pci_endpt_partitioning) {
-		group = pci_device_group(&pdev->dev);
-
-		/*
-		 * PCIe controller is not a paritionable entity
-		 * free the controller device iommu_group.
-		 */
-		if (pci_ctl->parent->iommu_group)
-			iommu_group_remove_device(pci_ctl->parent);
-	} else {
-		/*
-		 * All devices connected to the controller will share the
-		 * PCI controllers device group. If this is the first
-		 * device to be probed for the pci controller, copy the
-		 * device group information from the PCI controller device
-		 * node and remove the PCI controller iommu group.
-		 * For subsequent devices, the iommu group information can
-		 * be obtained from sibling devices (i.e. from the bus_devices
-		 * link list).
-		 */
-		if (pci_ctl->parent->iommu_group) {
-			group = get_device_iommu_group(pci_ctl->parent);
-			iommu_group_remove_device(pci_ctl->parent);
-		} else {
-			group = get_shared_pci_device_group(pdev);
-		}
-	}
-
-	if (!group)
-		group = ERR_PTR(-ENODEV);
-
-	return group;
-}
-
 static struct iommu_group *fsl_pamu_device_group(struct device *dev)
 {
+	struct iommu_group *group;
+	struct pci_dev *pdev;
+
 	/*
-	 * For platform devices we allocate a separate group for
-	 * each of the devices.
+	 * For platform devices we allocate a separate group for each of the
+	 * devices.
 	 */
 	if (!dev_is_pci(dev))
 		return generic_device_group(dev);
 
-	return get_pci_device_group(to_pci_dev(dev));
+	/*
+	 * We can partition PCIe devices so assign device group to the device
+	 */
+	pdev = to_pci_dev(dev);
+	if (check_pci_ctl_endpt_part(pci_bus_to_host(pdev->bus)))
+		return pci_device_group(&pdev->dev);
+
+	/*
+	 * All devices connected to the controller will share the same device
+	 * group.
+	 *
+	 * Due to ordering between fsl_pamu_init() and fsl_pci_init() it is
+	 * guaranteed that the pci_ctl->parent platform_device will have the
+	 * iommu driver bound and will already have a group set. So we just
+	 * re-use this group as the group for every device in the hose.
+	 */
+	group = iommu_group_get(pci_bus_to_host(pdev->bus)->parent);
+	if (WARN_ON(!group))
+		return ERR_PTR(-EINVAL);
+	return group;
 }
 
 static struct iommu_device *fsl_pamu_probe_device(struct device *dev)

-- 
2.40.1

On Tue, May 16, 2023 at 09:35:25PM -0300, Jason Gunthorpe wrote:

With POWER SPAPR now having a real iommu driver and using the normal group
lifecycle stuff fixing FSL will leave only VFIO's no-iommu support as a
user for the iommu_group_add/remove_device() calls. This will help
simplify the understanding of what the core code should be doing for these
functions.

Fix FSL to not need to call iommu_group_remove_device() at all.

v2:
 - Change the approach to use driver_managed_dma
 - Really simplify fsl_pamu_device_group() and just put everything in one
   function
 - New patch to make missing OF properties a probe failure
v1: https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_jgg@nvidia.com

Jason Gunthorpe (3):
  iommu/fsl: Always allocate a group for non-pci devices
  iommu/fsl: Move ENODEV to fsl_pamu_probe_device()
  iommu/fsl: Use driver_managed_dma to allow VFIO to work

 arch/powerpc/sysdev/fsl_pci.c   |   1 +
 drivers/iommu/fsl_pamu_domain.c | 123 +++++++++-----------------------
 2 files changed, 36 insertions(+), 88 deletions(-)

Any chance someone can test this on real hardware?

Regards,

	Joerg

On Tue, May 23, 2023 at 08:26:32AM +0200, Joerg Roedel wrote:

On Tue, May 16, 2023 at 09:35:25PM -0300, Jason Gunthorpe wrote:

quoted

With POWER SPAPR now having a real iommu driver and using the normal group
lifecycle stuff fixing FSL will leave only VFIO's no-iommu support as a
user for the iommu_group_add/remove_device() calls. This will help
simplify the understanding of what the core code should be doing for these
functions.

Fix FSL to not need to call iommu_group_remove_device() at all.

v2:
 - Change the approach to use driver_managed_dma
 - Really simplify fsl_pamu_device_group() and just put everything in one
   function
 - New patch to make missing OF properties a probe failure
v1: https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_jgg@nvidia.com

Jason Gunthorpe (3):
  iommu/fsl: Always allocate a group for non-pci devices
  iommu/fsl: Move ENODEV to fsl_pamu_probe_device()
  iommu/fsl: Use driver_managed_dma to allow VFIO to work

 arch/powerpc/sysdev/fsl_pci.c   |   1 +
 drivers/iommu/fsl_pamu_domain.c | 123 +++++++++-----------------------
 2 files changed, 36 insertions(+), 88 deletions(-)

Any chance someone can test this on real hardware?

There isn't even a MAINTAINERS entry for this, and the git log looks
pretty dead for a long time. I tried to cc people who might care,
but I'm not so optimistic - unless Li says something.

I do feel good that if there is a problem and someone does come
forward it can be fixed up without a big trouble. Certainly without
going back to mis-using iommu_grou_add/remove_device..

Thanks,
Jason

Jason Gunthorpe [off-list ref] writes:

On Tue, May 23, 2023 at 08:26:32AM +0200, Joerg Roedel wrote:

quoted

On Tue, May 16, 2023 at 09:35:25PM -0300, Jason Gunthorpe wrote:

quoted

With POWER SPAPR now having a real iommu driver and using the normal group
lifecycle stuff fixing FSL will leave only VFIO's no-iommu support as a
user for the iommu_group_add/remove_device() calls. This will help
simplify the understanding of what the core code should be doing for these
functions.

Fix FSL to not need to call iommu_group_remove_device() at all.

v2:
 - Change the approach to use driver_managed_dma
 - Really simplify fsl_pamu_device_group() and just put everything in one
   function
 - New patch to make missing OF properties a probe failure
v1: https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_jgg@nvidia.com

Jason Gunthorpe (3):
  iommu/fsl: Always allocate a group for non-pci devices
  iommu/fsl: Move ENODEV to fsl_pamu_probe_device()
  iommu/fsl: Use driver_managed_dma to allow VFIO to work

 arch/powerpc/sysdev/fsl_pci.c   |   1 +
 drivers/iommu/fsl_pamu_domain.c | 123 +++++++++-----------------------
 2 files changed, 36 insertions(+), 88 deletions(-)

Any chance someone can test this on real hardware?

There isn't even a MAINTAINERS entry for this, and the git log looks
pretty dead for a long time. I tried to cc people who might care,
but I'm not so optimistic - unless Li says something.

I guess it falls under LINUX FOR POWERPC EMBEDDED PPC83XX AND PPC85XX,
but that's basically orphaned these days. Basically all the FSL/NXP
powerpc code is orphaned, although there are still some users.

And things are somewhat complicated because some of the drivers are also
used on their ARM SOCs, so those still get maintained from the ARM side.

But looks like this driver is powerpc only.

Turns out I do have a machine that will probe this driver. AFAICS this
series doesn't regress it, but that's just booting. I don't have it
setup to test KVM/VFIO etc.

I do see some changes in dmesg, eg:

-fsl-pci ffe270000.pcie: Removing from iommu group 61
-pci 0003:00:00.0: Adding to iommu group 60
+pci 0003:00:00.0: Adding to iommu group 64

And lots more like that.

Anything else I can check easily?

cheers

On Tue, May 30, 2023 at 10:03:53PM +1000, Michael Ellerman wrote:

Jason Gunthorpe [off-list ref] writes:

quoted

On Tue, May 23, 2023 at 08:26:32AM +0200, Joerg Roedel wrote:

quoted

On Tue, May 16, 2023 at 09:35:25PM -0300, Jason Gunthorpe wrote:

quoted

With POWER SPAPR now having a real iommu driver and using the normal group
lifecycle stuff fixing FSL will leave only VFIO's no-iommu support as a
user for the iommu_group_add/remove_device() calls. This will help
simplify the understanding of what the core code should be doing for these
functions.

Fix FSL to not need to call iommu_group_remove_device() at all.

v2:
 - Change the approach to use driver_managed_dma
 - Really simplify fsl_pamu_device_group() and just put everything in one
   function
 - New patch to make missing OF properties a probe failure
v1: https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_jgg@nvidia.com

Jason Gunthorpe (3):
  iommu/fsl: Always allocate a group for non-pci devices
  iommu/fsl: Move ENODEV to fsl_pamu_probe_device()
  iommu/fsl: Use driver_managed_dma to allow VFIO to work

 arch/powerpc/sysdev/fsl_pci.c   |   1 +
 drivers/iommu/fsl_pamu_domain.c | 123 +++++++++-----------------------
 2 files changed, 36 insertions(+), 88 deletions(-)

Any chance someone can test this on real hardware?

There isn't even a MAINTAINERS entry for this, and the git log looks
pretty dead for a long time. I tried to cc people who might care,
but I'm not so optimistic - unless Li says something.

I guess it falls under LINUX FOR POWERPC EMBEDDED PPC83XX AND PPC85XX,
but that's basically orphaned these days. Basically all the FSL/NXP
powerpc code is orphaned, although there are still some users.

:\

But looks like this driver is powerpc only.

Yes
 

I do see some changes in dmesg, eg:

-fsl-pci ffe270000.pcie: Removing from iommu group 61
-pci 0003:00:00.0: Adding to iommu group 60
+pci 0003:00:00.0: Adding to iommu group 64

And lots more like that.

Yes, we expected that the groups would renumber.

Anything else I can check easily?

Wow Great, I think that is a Tested-by. :) Honestly booting at all is
99% of the battle..

This system looks like it has "partitionable end points" so I expect
it to all work, all this does is create a group for the controller
itself, which you saw in the boot with this diff:

  -fsl-pci ffe270000.pcie: Removing from iommu group 61

Which is harmless as long as its group is singleton.

So, I don't think there is more you can do with this system.

Joerg, this seems like enough, lets go ahead please :)

Thanks,
Jason

Jason Gunthorpe [off-list ref] writes:

On Tue, May 30, 2023 at 10:03:53PM +1000, Michael Ellerman wrote:

quoted

Jason Gunthorpe [off-list ref] writes:

quoted

On Tue, May 23, 2023 at 08:26:32AM +0200, Joerg Roedel wrote:

quoted

On Tue, May 16, 2023 at 09:35:25PM -0300, Jason Gunthorpe wrote:

quoted

With POWER SPAPR now having a real iommu driver and using the normal group
lifecycle stuff fixing FSL will leave only VFIO's no-iommu support as a
user for the iommu_group_add/remove_device() calls. This will help
simplify the understanding of what the core code should be doing for these
functions.

Fix FSL to not need to call iommu_group_remove_device() at all.

v2:
 - Change the approach to use driver_managed_dma
 - Really simplify fsl_pamu_device_group() and just put everything in one
   function
 - New patch to make missing OF properties a probe failure
v1: https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_jgg@nvidia.com

Jason Gunthorpe (3):
  iommu/fsl: Always allocate a group for non-pci devices
  iommu/fsl: Move ENODEV to fsl_pamu_probe_device()
  iommu/fsl: Use driver_managed_dma to allow VFIO to work

 arch/powerpc/sysdev/fsl_pci.c   |   1 +
 drivers/iommu/fsl_pamu_domain.c | 123 +++++++++-----------------------
 2 files changed, 36 insertions(+), 88 deletions(-)

Any chance someone can test this on real hardware?

There isn't even a MAINTAINERS entry for this, and the git log looks
pretty dead for a long time. I tried to cc people who might care,
but I'm not so optimistic - unless Li says something.

...

quoted

Anything else I can check easily?

Wow Great, I think that is a Tested-by. :) Honestly booting at all is
99% of the battle..

Great, yep consider it:

Tested-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

On Wed, May 31, 2023 at 05:04:04PM +1000, Michael Ellerman wrote:

Great, yep consider it:

Tested-by: Michael Ellerman <mpe@ellerman.id.au>

Alright, applied them for 6.5.