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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, May 16, 2023 at 04:00:47PM +0100, Robin Murphy wrote:
> 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.

I think that was true before..

Today we block out VFIO based on iommu_device_use_default_domain()
which is called prior to probe. That function is a NOP if the iommu
group hasn't already been setup.

Thus the platform_device probed by fsl_pci_probe() does not block VFIO
any more, even if it is in the same group.

But, more broadly, we exclude things like PCI bridges from the VFIO
check by using driver_managed_dma:

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)

Even though it is a NOP in this case because of ordering, it still
would be good for documentation purposes.

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

er.. Hum, yes... How did I miss that :\

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

Are you Ok with the above?

Jason