Re: [Query] ACS enablement in the DT based boot flow

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Nov 06, 2024 at 11:43:30AM +0530, Pavan Kondeti wrote:
> 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.

Please send the patch to the list with the appropriate commit log and
signed-off-by; see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.11

Be sure to cc anybody who reported previous issues.  It will also help
if you can explain why any issues that were previously reported are no
longer a problem.

> [1]
> https://lore.kernel.org/all/1621566204-37456-1-git-send-email-wangxingang5@huawei.com/