linux-kernel - Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f80e1d58-cae1-4f3b-8b66-fc920ad4c5ba@oss.qualcomm.com>
Date: Tue, 21 Oct 2025 17:42:39 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Jingoo Han <jingoohan1@...il.com>,
        Manivannan Sadhasivam
 <mani@...nel.org>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kwilczynski@...nel.org>,
        Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, Ron Economos <re@...z.net>
Subject: Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor
 drivers



On 10/18/2025 12:40 AM, Bjorn Helgaas wrote:
> On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya Chundru wrote:
>> When the vendor configuration space is 256MB aligned, the DesignWare
>> PCIe host driver enables ECAM access and sets the DBI base to the start
>> of the config space. This causes vendor drivers to incorrectly program
>> iATU regions, as they rely on the DBI address for internal accesses.
>>
>> To fix this, avoid overwriting the DBI base when ECAM is enabled.
>> Instead, introduce a custom ECAM PCI ops implementation that accesses
>> the DBI region directly for bus 0 and uses ECAM for other buses.
>>
>> Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")
>> Reported-by: Ron Economos <re@...z.net>
>> Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
>> Suggested-by: Manivannan Sadhasivam <mani@...nel.org>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-designware-host.c | 28 +++++++++++++++++++----
>>   1 file changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -23,6 +23,7 @@
>>   #include "pcie-designware.h"
>>   
>>   static struct pci_ops dw_pcie_ops;
>> +static struct pci_ops dw_pcie_ecam_ops;
>>   static struct pci_ops dw_child_pcie_ops;
>>   
>>   #define DW_PCIE_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS		| \
>> @@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *re
>>   	if (IS_ERR(pp->cfg))
>>   		return PTR_ERR(pp->cfg);
>>   
>> -	pci->dbi_base = pp->cfg->win;
>> -	pci->dbi_phys_addr = res->start;
>> -
>>   	return 0;
>>   }
>>   
>> @@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
>>   		if (ret)
>>   			return ret;
>>   
>> -		pp->bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
>> +		pp->bridge->ops = &dw_pcie_ecam_ops;
>>   		pp->bridge->sysdata = pp->cfg;
>>   		pp->cfg->priv = pp;
>>   	} else {
>> @@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
>>   }
>>   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>>   
>> +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
>> +{
>> +	struct pci_config_window *cfg = bus->sysdata;
>> +	struct dw_pcie_rp *pp = cfg->priv;
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	unsigned int busn = bus->number;
>> +
>> +	if (busn > 0)
>> +		return pci_ecam_map_bus(bus, devfn, where);
> 
> Is there a way to avoid the "root bus is bus 00" assumption here?  It
> looks like something like this might work (it inverts the condition
> to take care of the root bus special case first):
> 
>    if (bus == pp->bridge->bus) {
>      if (PCI_SLOT(devfn) > 0)
>        return NULL;
> 
>      return pci->dbi_base + where;
>    }
> 
>    return pci_ecam_map_bus(bus, devfn, where);
> This is working fine Bjorn, shall I send v2 with this change.

- Krishna Chaitanya.
>> +	if (PCI_SLOT(devfn) > 0)
>> +		return NULL;
> 
> This essentially says only one function (00.0) can be on the root bus.
> I assume that someday that will be relaxed and there may be multiple
> Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
> didn't have to have this check.
> 
> What happens without it?  Does the IP return the ~0 data that the PCI
> core would interpret as "there's no device here"?
> 
> Regardless, I love this series because it removes quite a bit of code
> and seems so much cleaner.
> 
>> +	return pci->dbi_base + where;
>> +}
>> +
>>   static struct pci_ops dw_pcie_ops = {
>>   	.map_bus = dw_pcie_own_conf_map_bus,
>>   	.read = pci_generic_config_read,
>>   	.write = pci_generic_config_write,
>>   };
>>   
>> +static struct pci_ops dw_pcie_ecam_ops = {
>> +	.map_bus = dw_pcie_ecam_conf_map_bus,
>> +	.read = pci_generic_config_read,
>> +	.write = pci_generic_config_write,
>> +};
>> +
>>   static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>>   {
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>
>> -- 
>> 2.34.1
>>