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] [day] [month] [year] [list]
Message-ID: <Zx+96SEUUN57SPTU@lizhi-Precision-Tower-5810>
Date: Mon, 28 Oct 2024 12:38:01 -0400
From: Frank Li <Frank.li@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Abraham I <kishon@...nel.org>,
	Saravana Kannan <saravanak@...gle.com>,
	Jingoo Han <jingoohan1@...il.com>,
	Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
	Jesper Nilsson <jesper.nilsson@...s.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>, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...s.com, linux-arm-kernel@...ts.infradead.org,
	imx@...ts.linux.dev,
	Krzysztof Wilczyński <kwilczynski@...nel.org>
Subject: Re: [PATCH v4 1/4] PCI: dwc: ep: Add bus_addr_base for outbound
 window

On Fri, Oct 25, 2024 at 05:31:02PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 24, 2024 at 04:41:43PM -0400, Frank Li wrote:
> >                                Endpoint          Root complex
> >                              ┌───────┐        ┌─────────┐
> >                ┌─────┐       │ EP    │        │         │      ┌─────┐
> >                │     │       │ Ctrl  │        │         │      │ CPU │
> >                │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
> >                │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
> >                │     │       │       │        │ └────┘  │ Outbound Transfer
> >                └─────┘       │       │        │         │
> >                              │       │        │         │
> >                              │       │        │         │
> >                              │       │        │         │ Inbound Transfer
> >                              │       │        │         │      ┌──▼──┐
> >               ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
> >               │       │ outbound Transfer*    │ │       │      └─────┘
> >    ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
> >    │     │    │ Fabric│Bus   │       │ PCI Addr         │
> >    │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
> >    │     │CPU │       │0x8000_0000   │        │         │
> >    └─────┘Addr└───────┘      │       │        │         │
> >           0x7000_0000        └───────┘        └─────────┘
> >
> > Add `bus_addr_base` to configure the outbound window address for CPU write.
> > The bus fabric generally passes the same address to the PCIe EP controller,
> > but some bus fabrics convert the address before sending it to the PCIe EP
> > controller.
> >
> > Above diagram, CPU write data to outbound windows address 0x7000_0000,
> > Bus fabric convert it to 0x8000_0000. ATU should use bus address
> > 0x8000_0000 as input address and convert to PCI address 0xA000_0000.
>
> Thanks for the diagram and description.  I don't think the top half is
> relevant to *this* patch, is it?  I think this patch is only concerned
> with the address translations between the CPU in the endpoint and the
> PCI bus address.  In this case it happens in two steps: the bus fabric
> applies one offset, and the ATU applies a second offset.
>
> Unless the top half is relevant, I would omit it and simply use
> something like this:
>
>                    Endpoint
>   ┌───────────────────────────────────────────────┐
>   │                             pcie-ep@...10000  │
>   │                             ┌────────────────┐│
>   │                             │   Endpoint     ││
>   │                             │   PCIe         ││
>   │                             │   Controller   ││
>   │           bus@...00000      │                ││
>   │           ┌──────────┐      │                ││
>   │           │          │ Outbound Transfer     ││
>   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
>   ││     │    │  Fabric  │Bus   │                ││PCI Addr
>   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
>   ││     │CPU │          │0x8000_0000            ││
>   │└─────┘Addr└──────────┘      │                ││
>   │       0x7000_0000           └────────────────┘│
>   └───────────────────────────────────────────────┘
>
> If you don't want a big "Endpoint" box including the CPU and bus
> fabric, that's OK with me, too.  I added it because everything on the
> PCI side only sees TLPs that contain PCI bus addresses, and can't tell
> anything about the internal implementation of the Endpoint.
>
> > Previously, `cpu_addr_fixup()` was used to handle address conversion. Now,
> > the device tree provides this information, preferring a common method.
> >
> > bus@...00000 {
> > 	compatible = "simple-bus";
> > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> >
> > 	pcie-ep@...10000 {
> > 		reg = <0x5f010000 0x00010000>,
> > 		      <0x80000000 0x10000000>;
> > 		reg-names = "dbi", "addr_space";
> > 		...
> > 	};
> > 	...
> > };
>
> I guess bus@...00000 includes a "ranges" property because that
> translation from 0x7000_0000 -> 0x8000_0000 is fixed or at least
> not touched by Linux?

Yes, it is fixed by hardware.

>
> And the pcie-ep@...10000 address translation from 0x8000_0000 to
> 0xA000_0000 *is* programmed by Linux and therefore can't be described
> by a DT?  But I guess Linux only programs the *PCI* side, and the
> parent side (0x8000_0000) is fixed?

0x8000_0000 -> 0xA000_0000 is programmed by Linux RC side host driver tell
Linux EP side driver how to map it. 0x8000_0000 is fixed MDIO space.

>
> AFAICT, the "reg = <0x5f010000 0x00010000>" part is not relevant here.

Yes.

>
> I guess this implementation assumes there's only a single aperture
> through the Bus Fabric, right?
>
> And also a single ATU aperture through the endpoint PCIe controller?
>

There are 8 ATU provide by DWC PCI controller. Bus only provide a big
PCI map windows for example, 0x8000_0000..0x9000_0000.

we can slip it up to 8 small region, such as each 64K,  each region can
map to any PCI address by ATU.

0x8001_0000 -> 0xA000_00000
0x8002_0000 -> 0xB100_00000
0x8003_0000 -> 0xD000_00000
...

This part EPC driver already handle it. EPC driver only need know
"addr_space" information by above sample dts.

> And also that there's only one layer of Bus Fabric address
> translation?

Doesn't metter, but PCI controller only care about closest one. Others
translation is tranparent to drivers. As above example, PCI EP controller
only care about input address is 0x8000_0000,  and cpu send out address is
0x7000_0000. Even there are more translation,
0x7000_0000-> 0x9000_0000->0x8000_0000,
  ^^^^^^                   ^^^^

PCI EP controller only care input (0x7000_0000) and output (0x9000_0000),
don't care any internal translation (0x9000_0000).


> The fact that only a few DWC controllers have this
> translation suggests that this part of the picture might be external
> to the DWC IP and there could be more variation.  But I guess there's
> no point in adding code for topologies that don't exist; we can deal
> with that if the need ever arises.

Some cdns also have the same situation.

>
> > 'ranges' in bus@...00000 descript how address convert from CPU address
> > to bus address.
> >
> > Use `of_property_read_reg()` to obtain the bus address and set it to the
> > ATU correctly, eliminating the need for vendor-specific cpu_addr_fixup().
> >
> > Add 'using_dtbus_info' to indicate device tree reflect correctly bus
> > address translation in case break compatibility.
> >
> > Signed-off-by: Frank Li <Frank.Li@....com>
> > ---
> > Change from v3 to v4
> > - change bus_addr_base to u64 to fix 32bit build error
> > | Reported-by: kernel test robot <lkp@...el.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202410230328.BTHareG1-lkp@intel.com/
> >
> > Change from v2 to v3
> > - Add using_dtbus_info to control if use device tree bus ranges
> > information.
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 14 +++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h    |  9 +++++++++
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 43ba5c6738df1..81b4057befa62 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/align.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/of.h>
> > +#include <linux/of_address.h>
> >  #include <linux/platform_device.h>
> >
> >  #include "pcie-designware.h"
> > @@ -294,7 +295,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >
> >  	atu.func_no = func_no;
> >  	atu.type = PCIE_ATU_TYPE_MEM;
> > -	atu.cpu_addr = addr;
> > +	atu.cpu_addr = addr - ep->phys_base + ep->bus_addr_base;
>
> Tangent: Maybe dw_pcie_ob_atu_cfg.cpu_addr isn't exactly the right
> name, since it now contains an address that is not a CPU physical
> address.  Not a question for *this* patch though.

yes, cpu_addr is not good name altough it is correct for most system.

>
> >  	atu.pci_addr = pci_addr;
> >  	atu.size = size;
> >  	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > @@ -861,6 +862,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  	struct device *dev = pci->dev;
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct device_node *np = dev->of_node;
> > +	int index;
> >
> >  	INIT_LIST_HEAD(&ep->func_list);
> >
> > @@ -873,6 +875,16 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		return -EINVAL;
> >
> >  	ep->phys_base = res->start;
> > +	ep->bus_addr_base = ep->phys_base;
> > +
> > +	if (pci->using_dtbus_info) {
> > +		index = of_property_match_string(np, "reg-names", "addr_space");
> > +		if (index < 0)
> > +			return -EINVAL;
> > +
> > +		of_property_read_reg(np, index, &ep->bus_addr_base, NULL);
> > +	}
>
> If this translation were fixed, I suppose we'd extract something from
> a "ranges" property that contains (child-bus-address,
> parent-bus-address) information.

Yes, see below
	ranges = <0x80000000 0x0 0x70000000 0x10000000>;

> So I suppose "addr_space" contains a
> fixed parent-bus-address, and is setting the child (PCI) bus address,
> right?

"addr_space" hold PCI EP outbound MDIO space, which is parent-bus-address.
it is confused if called as PCI bus address, which most likely the address
after ATU covert.

bus@...00000 {
     compatible = "simple-bus";
     ranges = <0x80000000 0x0 0x70000000 0x10000000>;

     pcie-ep@...10000 {
             reg = <0x5f010000 0x00010000>,
                   <0x80000000 0x10000000>;
             reg-names = "dbi", "addr_space";
             ...
     };

History reasion, PCI EP use reg-names "addr_space" indicate outbound
windows informaiton.

>
> If so, I might add a comment here for other readers who come this way.
> (And me, because I won't remember the next time I read it :)
>
> >  	ep->addr_size = resource_size(res);
> >
> >  	if (ep->ops->pre_init)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 347ab74ac35aa..f10b533b04f77 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -410,6 +410,7 @@ struct dw_pcie_ep {
> >  	struct list_head	func_list;
> >  	const struct dw_pcie_ep_ops *ops;
> >  	phys_addr_t		phys_base;
> > +	u64			bus_addr_base;
> >  	size_t			addr_size;
> >  	size_t			page_size;
> >  	u8			bar_to_atu[PCI_STD_NUM_BARS];
> > @@ -463,6 +464,14 @@ struct dw_pcie {
> >  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
> >  	struct gpio_desc		*pe_rst;
> >  	bool			suspended;
> > +	/*
> > +	 * Use device tree 'ranges' property of bus node instead using
> > +	 * cpu_addr_fixup(). Some old platform dts 'ranges' in bus node may not
> > +	 * reflect real hardware's behavior. In case break these platform back
> > +	 * compatibility, add below flags. Set it true if dts already correct
> > +	 * indicate bus fabric address convert.
> > +	 */
> > +	bool			using_dtbus_info;
> >  };
> >
> >  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> >
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ