| 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
| ||
|
Message-ID: <20250313220450.GA755590@bhelgaas>
Date: Thu, 13 Mar 2025 17:04:50 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Frank Li <Frank.Li@....com>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
Jingoo Han <jingoohan1@...il.com>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Richard Zhu <hongxing.zhu@....com>,
Lucas Stach <l.stach@...gutronix.de>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev,
Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v11 06/11] PCI: dwc: Use devicetree 'ranges' property to
get rid of cpu_addr_fixup() callback
On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote:
> The 'ranges' property at PCI controller parent bus can indicate address
> translation information. Most system's bus fabric use 1:1 map between input
> and output address. but some hardware like i.MX8QXP doesn't use 1:1 map.
I think you've used reg["addr_space"] to get the offset for Endpoints
forever.
I just noticed that through v9, you used 'ranges' to get the offset
for the Root Complex (with "Add parent_bus_offset to resource_entry"),
and I think v10 switched to use reg["config"] instead.
I think I originally proposed the idea of "Add parent_bus_offset to
resource_entry" patch, but I think it turned out to be kind of an ugly
approach.
Anyway, IIUC this v11 patch actually uses reg["config"] to compute the
offset, not 'ranges', so we should probably update the subject and
commit log to reflect that, and maybe remove the now-unused bits of
the devicetree example.
I do worry a little bit about the assumption that the offset of
reg["config"] is the same as the offset of the other pieces. The main
place we use the offset on RCs is for the ATU, and isn't the ATU in
the MemSpace area at 0x8000_0000 below?
It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config"
offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset,
but I don't know that this is guaranteed for all designs.
v9:
[PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry
https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-3-3c4bb506f665@nxp.com
[PATCH v9 5/7] PCI: dwc: ep: Add parent_bus_addr for outbound window
https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-5-3c4bb506f665@nxp.com
v10:
[PATCH v10 05/10] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback
https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-5-409dafc950d1@nxp.com
[PATCH v10 06/10] PCI: dwc: ep: Add parent_bus_addr for outbound window
https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-6-409dafc950d1@nxp.com
> See below diagram:
>
> ┌─────────┐ ┌────────────┐
> ┌─────┐ │ │ IA: 0x8ff8_0000 │ │
> │ CPU ├───►│ ┌────►├─────────────────┐ │ PCI │
> └─────┘ │ │ │ IA: 0x8ff0_0000 │ │ │
> CPU Addr │ │ ┌─►├─────────────┐ │ │ Controller │
> 0x7ff8_0000─┼───┘ │ │ │ │ │ │
> │ │ │ │ │ │ │ PCI Addr
> 0x7ff0_0000─┼──────┘ │ │ └──► IOSpace ─┼────────────►
> │ │ │ │ │ 0
> 0x7000_0000─┼────────►├─────────┐ │ │ │
> └─────────┘ │ └──────► CfgSpace ─┼────────────►
> BUS Fabric │ │ │ 0
> │ │ │
> └──────────► MemSpace ─┼────────────►
> IA: 0x8000_0000 │ │ 0x8000_0000
> └────────────┘
>
> bus@...00000 {
> compatible = "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0x80000000 0x0 0x70000000 0x10000000>;
>
> pcie@...10000 {
> compatible = "fsl,imx8q-pcie";
> reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> reg-names = "dbi", "config";
> #address-cells = <3>;
> #size-cells = <2>;
> device_type = "pci";
> bus-range = <0x00 0xff>;
> ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> ...
> };
> };
>
> Term Intermediate address (IA) here means the address just before PCIe
> controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> be removed.
>
> Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset
> is the same for all kinds of address translation.
>
> Just set parent_bus_offset, but doesn't use it, so no functional change
> intended yet.
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> pp->io_base = pci_pio_to_address(win->res->start);
> }
>
> + /*
> + * visconti_pcie_cpu_addr_fixup() use pp->io_base,
> + * so have to call dw_pcie_init_parent_bus_offset() after init
> + * pp->io_base.
> + */
> + ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base);
> + if (ret)
> + return ret;
> +
> /* Set default bus ops */
> bridge->ops = &dw_pcie_ops;
> bridge->child_ops = &dw_child_pcie_ops;
>
> --
> 2.34.1
>
Powered by blists - more mailing lists