Re: [PATCH 1/2] iommu/fsl: Do not use iommu_group_remove_device() under ops->device_group()

[Robin Murphy <robin.murphy@arm.com>]

On 2023-05-16 01:27, Jason Gunthorpe wrote:
> This API is expected to be used only by POWER and VFIO no-iommu that
> manually manage the group lifecycle. It should not be called under
> ops->device_group().
> 
> This is already buggy as is since the core code does not expect a probed
> driver to loose it's iommu_group without also releasing the device.
> 
> FSL seems to be trying to block the platform_device that represents the
> pci_controller, eg the thing passed to fsl_add_bridge(), from having an
> iommu_group.
> 
> Instead of creating an iommu_group that we don't want and then later
> removing it, just don't create it at all in the first place.
> 
> For the 'pci_endpt_partitioning' case every PCI device already gets its
> own iommu_group through the standard code, so it is unclear why having a
> dedicated group for the controller could be problematic.
> 
> For the other case, the controller group was being used to bizarrely
> de-duplicate the group in it's hose. Instead just directly create a group
> for the hose the first time we encounter it. The code already searches the
> entire hose to find any iommu_group. Again, it is unclear why having the
> pci_controller inside the same iommu_group as the PCI devices would be
> harmful.

It's harmful in that case because it prevents VFIO from working at all - 
even now that VFIO no longer rejects cross-bus groups on principle, the 
fsl-pci driver being bound to the platform device would then deny VFIO 
from taking ownership.

> In any case, this is a cleaner way to not enable iommu support for the
> platform_device.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/fsl_pamu_domain.c | 42 ++++++++++++++++-----------------
>   1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index bce37229709965..bf045f58cd50ae 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -11,6 +11,7 @@
>   
>   #include <linux/platform_device.h>
>   #include <sysdev/fsl_pci.h>
> +#include <linux/pci.h>
>   
>   /*
>    * Global spinlock that needs to be held while
> @@ -379,7 +380,21 @@ static struct iommu_group *get_shared_pci_device_group(struct pci_dev *pdev)
>   		bus = bus->parent;
>   	}
>   
> -	return NULL;
> +	return iommu_group_alloc();
> +}
> +
> +static int __is_pci_controller_parent(struct device *dev, void *data)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct pci_controller *pci_ctl = pci_bus_to_host(pdev->bus);
> +
> +	return dev == pci_ctl->parent;

If I'm not mistaken, this is testing every *PCI* device to see any of 
them are their own host bridge's platform parent...

> +}
> +
> +static bool is_pci_controller_parent(struct device *dev)
> +{
> +	return bus_for_each_dev(&pci_bus_type, NULL, NULL,
> +				__is_pci_controller_parent);

for_each_pci_dev() might be easier to handle, however for what I think 
this is ultimately trying to do, device_find_child() would seem even better.

>   }
>   
>   static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
> @@ -393,30 +408,12 @@ static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
>   	/* 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).
> +		 * same device group.
>   		 */
> -		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);
> -		}
> +		group = get_shared_pci_device_group(pdev);

It would be nice if we could still associate the "PCI group" directly 
with the pci_controller some other way, and avoid all the 
slightly-confusing bus walks altogether, but I don't see a sufficiently 
clean way to achieve that :(

Thanks,
Robin.

>   	}
>   
>   	if (!group)
> @@ -436,7 +433,8 @@ 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))
> +	else if (of_get_property(dev->of_node, "fsl,liodn", &len) &&
> +		 !is_pci_controller_parent(dev))
>   		group = get_device_iommu_group(dev);
>   
>   	return group;