Hi Will,
On Thu, Jul 18, 2024 at 03:43:18PM +0530, Pavan Kondeti wrote:
> > > The pci_request_acs() in of_iommu_configure(), which happens too late
> > > to affect pci_enable_acs(), was added by 6bf6c24720d3 ("iommu/of:
> > > Request ACS from the PCI core when configuring IOMMU linkage"), so I
> > > cc'd Will and Joerg. I don't know if that *used* to work and got
> > > broken somehow, or if it never worked as intended.
> >
> > I don't have any way to test this, but I'm supportive of having the same
> > flow for DT and ACPI-based flows. Vidya, are you able to cook a patch?
> >
>
> I ran into a similar observation while testing a PCI device assignment
> to a VM. In my configuration, the virtio-iommu is enumerated over the
> PCI transport. So, I am thinking we can't hook pci_request_acs() to an
> IOMMU driver. Does the below patch makes sense?
>
> The patch is tested with a VM and I could see ACS getting enabled and
> separate IOMMU groups are created for the devices attached under
> PCIe root port(s).
>
> The RC/devices with ACS quirks are not suffering from this problem as we
> short circuit ACS capability detection checking in
> pci_acs_enabled()->pci_dev_specific_acs_enabled() . May be this is one
> of the reason why this was not reported/observed by some platforms with
> DT.
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index b908fe1ae951..0eeb7abfbcfa 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -123,6 +123,13 @@ bool pci_host_of_has_msi_map(struct device *dev)
> return false;
> }
>
> +bool pci_host_of_has_iommu_map(struct device *dev)
> +{
> + if (dev && dev->of_node)
> + return of_get_property(dev->of_node, "iommu-map", NULL);
> + return false;
> +}
> +
> static inline int __of_pci_pci_compare(struct device_node *node,
> unsigned int data)
> {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4c367f13acdc..ea6fcdaf63e2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -889,6 +889,7 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
> dev_set_msi_domain(&bus->dev, d);
> }
>
> +bool pci_host_of_has_iommu(struct device *dev);
> static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> {
> struct device *parent = bridge->dev.parent;
> @@ -951,6 +952,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> !pci_host_of_has_msi_map(parent))
> bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
>
> + if (pci_host_of_has_iommu_map(parent))
> + pci_request_acs();
> +
> if (!parent)
> set_dev_node(bus->bridge, pcibus_to_node(bus));
>
I see that this problem is reproducible with the kernel tip. While preparing
patch submission, I found there was an attempt to fix [1] this problem
earlier but later reverted due to issues reported on linux-next. I did
not see any follow up on the issues. I would like to resend this patch
again as it was acked by people.
Thanks,
Pavan
[1]
https://lore.kernel.org/all/1621566204-37456-1-git-send-email-wangxingang5@xxxxxxxxxx/
