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

[Pavan Kondeti]

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/