Message-ID: <e4fjukl3xkfcxcm5p6jo4davplqlrx4xrpbjhdsduqjlv7oz7l@zilbaf3h6py2>
Date: Wed, 24 Sep 2025 14:20:52 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: manivannan.sadhasivam@....qualcomm.com, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, 
	Robin Murphy <robin.murphy@....com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, Joerg Roedel <jroedel@...e.de>, 
	iommu@...ts.linux.dev, Anders Roxell <anders.roxell@...aro.org>, 
	Naresh Kamboju <naresh.kamboju@...aro.org>, Pavankumar Kondeti <quic_pkondeti@...cinc.com>, 
	Xingang Wang <wangxingang5@...wei.com>, Marek Szyprowski <m.szyprowski@...sung.com>, 
	stable@...r.kernel.org
Subject: Re: [PATCH 2/2] iommu/of: Call pci_request_acs() before enumerating
 the Root Port device

On Tue, Sep 23, 2025 at 03:27:01PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 10, 2025 at 11:09:21PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Xingang Wang <wangxingang5@...wei.com>
> > 
> > When booting with devicetree, ACS is enabled for all ACS capable PCI
> > devices except the first Root Port enumerated in the system. This is due to
> > calling pci_request_acs() after the enumeration and initialization of the
> > Root Port device. 
> 
> I suppose you're referring to a path like below, where we *check*
> pci_acs_enable during PCI enumeration, but we don't *set* it until we
> add the device and look for a driver for it?
> 
>   pci_host_common_init
>     devm_pci_alloc_host_bridge
>       devm_of_pci_bridge_init
>         pci_request_acs
>           pci_acs_enable = 1                    # ++ new set here
>     pci_host_probe
>       pci_scan_root_bus_bridge
>         pci_scan_device
>           pci_init_capabilities
>             pci_enable_acs
>               if (pci_acs_enable)               # test here
>                 ...
>       pci_bus_add_devices
>         driver_probe_device
>           pci_dma_configure
>             of_dma_configure
>               of_dma_configure_id
>                 of_iommu_configure
>                   pci_request_acs
>                     pci_acs_enable = 1          # -- previously set here
> 

Yes!

> > But afterwards, ACS is getting enabled for the rest of the PCI
> > devices, since pci_request_acs() sets the 'pci_acs_enable' flag and
> > the PCI core uses this flag to enable ACS for the rest of the ACS
> > capable devices.
> 
> I don't quite understand why ACS would be enabled for *any* of the
> devices because we generally enumerate all of them, which includes the
> pci_init_capabilities() and pci_enable_acs(), before adding and
> attaching drivers to them.
> 
> But it does seem kind of dumb that we set the system-wide "enable ACS"
> property in a per-device place like an individual device probe.
> 

I had the same opinion when I saw the 'pci_acs_enable' flag. But I think the
intention was to enable ACS only if the controller is capable of assigning
different IOMMU groups per device. Otherwise, ACS is more or less of no use.

> > Ideally, pci_request_acs() should only be called if the 'iommu-map' DT
> > property is set for the host bridge device. Hence, call pci_request_acs()
> > from devm_of_pci_bridge_init() if the 'iommu-map' property is present in
> > the host bridge DT node. This aligns with the implementation of the ARM64
> > ACPI driver (drivers/acpi/arm64/iort.c) as well.
> > 
> > With this change, ACS will be enabled for all the PCI devices including the
> > first Root Port device of the DT platforms.
> > 
> > Cc: stable@...r.kernel.org # 5.6
> > Fixes: 6bf6c24720d33 ("iommu/of: Request ACS from the PCI core when configuring IOMMU linkage")
> > Signed-off-by: Xingang Wang <wangxingang5@...wei.com>
> > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@...cinc.com>
> > [mani: reworded subject, description and comment]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > ---
> >  drivers/iommu/of_iommu.c | 1 -
> >  drivers/pci/of.c         | 8 +++++++-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 6b989a62def20ecafd833f00a3a92ce8dca192e0..c31369924944d36a3afd3d4ff08c86fc6daf55de 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -141,7 +141,6 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
> >  			.np = master_np,
> >  		};
> >  
> > -		pci_request_acs();
> >  		err = pci_for_each_dma_alias(to_pci_dev(dev),
> >  					     of_pci_iommu_init, &info);
> >  		of_pci_check_device_ats(dev, master_np);
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index 3579265f119845637e163d9051437c89662762f8..98c2523f898667b1618c37451d1759959d523da1 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -638,9 +638,15 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
> >  
> >  int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge)
> >  {
> > -	if (!dev->of_node)
> > +	struct device_node *node = dev->of_node;
> > +
> > +	if (!node)
> >  		return 0;
> >  
> > +	/* Enable ACS if IOMMU mapping is detected for the host bridge */
> > +	if (of_property_read_bool(node, "iommu-map"))
> > +		pci_request_acs();
> 
> I'm not really convinced that the existence of 'iommu-map' in
> devicetree is a clear signal that ACS should be enabled, so I'm a
> little hesitant about this part.
> 
> Is it possible to boot using a devicetree with 'iommu-map', but with
> the IOMMU disabled or the IOMMU driver not present?  Or other
> situations where we don't need ACS?
> 

Certainly possible. But the issue is, we cannot reliably detect the presence of
IOMMU until the first pci_dev is created, which will be too late as
pci_acs_init() is called during pci_device_add().

This seems to be the case for ACPI platforms also.

Maybe IOMMU folks Robin/Joerg/Will could comment more.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists