Re: [PATCH v2 03/16] iommu: Compute iommu_groups properly for PCIe switches

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jul 17, 2025 at 06:03:42PM -0400, Donald Dutile wrote:
> > +static struct iommu_group *pci_get_alias_group(struct pci_dev *pdev)
> So, the former pci_device_group() is completely re-written below,
> and what it use to do is renamed pci_get_alias_group(), which shouldn't be
> (easily) confused with ...
> 
> > +{
> > +	struct iommu_group *group;
> > +	DECLARE_BITMAP(devfns, 256) = {};
> >   	/*
> >   	 * Look for existing groups on device aliases.  If we alias another
> >   	 * device or another device aliases us, use the same group.
> >   	 */
> > -	group = get_pci_alias_group(pdev, (unsigned long *)devfns);
> > +	group = get_pci_alias_group(pdev, devfns);
> ... get_pci_alias_group() ?
> 
> ... and it's only used for PCIe case below (in pci_device_group), so
> should it be named 'pcie_get_alias_group()' ?

Well, the naming is alot better after this is reworked with the
reachable set patch and these two functions are removed.

But even then I guess it is not a great name.

How about:

/*
 * Return a group if the function has isolation restrictions related to
 * aliases or MFD ACS.
 */
static struct iommu_group *pci_get_function_group(struct pci_dev *pdev)
{

> > +static struct iommu_group *pci_hierarchy_group(struct pci_dev *pdev)
> although static, could you provide a function description for its purpose ?

/* Return a group if the upstream hierarchy has isolation restrictions. */

> > +	/*
> > +	 * !self is only for SRIOV virtual busses which should have been
> > +	 * excluded above.
> by pci_is_root_bus() ?? -- that checks if bus->parent exists...
> not sure how that excludes the case of !bus->self ...

Should be this:

	/*
	 * !self is only for SRIOV virtual busses which should have been
	 * excluded by pci_physfn()
	 */
	if (WARN_ON(!bus->self))

> > +	 */
> > +	if (WARN_ON(!bus->self))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	group = iommu_group_get(&bus->self->dev);
> > +	if (!group) {
> > +		/*
> > +		 * If the upstream bridge needs the same group as pdev then
> > +		 * there is no way for it's pci_device_group() to discover it.
> > +		 */
> > +		dev_err(&pdev->dev,
> > +			"PCI device is probing out of order, upstream bridge device of %s is not probed yet\n",
> > +			pci_name(bus->self));
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +	}
> > +	if (group->bus_data & BUS_DATA_PCI_NON_ISOLATED)
> > +		return group;
> > +	iommu_group_put(group);
> > +	return NULL;
> ... and w/o the function description, I don't follow:
> -- rtn an iommu-group if it has NON_ISOLATED property ... but rtn null if all devices below it are isolated?

Yes. For all these internal functions non null means we found a group
to join, NULL means to keep checking isolation rules.

Thanks,
Jason