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
