Jason,
Thanks for replies, clarifications...
Couple questions below.
On 7/18/25 2:09 PM, Jason Gunthorpe wrote:
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.
Didn't notice that... will re-look.
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)
{
sure...
+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))
my Linux tree says its this:
static inline bool pci_is_root_bus(struct pci_bus *pbus)
{
return !(pbus->parent);
}
is there a change to pci_is_root_bus() in a -next branch?
+ */
+ 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.
ah, so !group == keep looking for for non-isolated conditions.. got it.
Could that lead to two iommu-groups being created that could/should be one larger one?
Thanks,
Jason
