Message-ID: <b8a871d8-d84b-4c86-8f63-f4e1b2a5fccf@arm.com>
Date: Wed, 24 Sep 2025 20:49:26 +0100
From: Robin Murphy <robin.murphy@....com>
To: Bjorn Helgaas <helgaas@...nel.org>,
 Manivannan Sadhasivam <mani@...nel.org>
Cc: manivannan.sadhasivam@....qualcomm.com,
 Bjorn Helgaas <bhelgaas@...gle.com>, Joerg Roedel <joro@...tes.org>,
 Will Deacon <will@...nel.org>,
 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 2025-09-24 7:57 pm, Bjorn Helgaas wrote:
> On Wed, Sep 24, 2025 at 02:20:52PM +0530, Manivannan Sadhasivam wrote:
>> 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.
> 
> I don't doubt that the current code doesn't detect presence or use of
> IOMMU until later.  But that feels like an implementation defect
> because logically the IOMMU is upstream of any PCI device that uses
> it, so architecturally I would expect it to be *possible* to detect it
> before PCI enumeration.
> 
> More to the point, it's not at all obvious how to infer that
> 'iommu-map' in the devicetree means the IOMMU will be used.

Indeed, I would say the way to go down that route would be to echo what 
we do for "iommus", and defer probing the entire host controller driver 
until the targets of an "iommu-map" are either present or assumed to 
never be appearing. (And of course an ACPI equivalent for that would be 
tricky...)

However, even that isn't necessarily the full solution, as just as it's 
not really appropriate for PCI to force ACS without knowing whether an 
IOMMU is actually present to make it meaningful, it's also not strictly 
appropriate for an IOMMU driver to request ACS globally without knowing 
that it actually serves any relevant PCIe devices. Even in the ideal 
scenario, I think the point of actually knowing is still a bit too late 
for the current API design:

   pci_device_add
     pci_init_capabilities
       pci_acs_init
         pci_enable_acs
     device_add
       iommu_bus_notifier
         iommu_probe_device
           //logically, request ACS for dev here

(at the moment, iommu_probe_device() will actually end up calling into 
the same of_iommu_configure()->pci_request_acs() path, but the plan is 
still to eventually shorten and streamline that.)

I guess we might want to separate the actual ACS enablement from the 
basic capability init, or at least perhaps just call pci_enable_acs() 
again after the IOMMU notifier may have changed the policy?

Thanks,
Robin.

Powered by blists - more mailing lists