| 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: <e04f8a63-3dc8-4a58-a9a4-4c70debd2b93@quicinc.com>
Date: Thu, 18 Jul 2024 14:52:20 -0700
From: Mayank Rana <quic_mrana@...cinc.com>
To: Prudhvi Yarlagadda <quic_pyarlaga@...cinc.com>, <jingoohan1@...il.com>,
<manivannan.sadhasivam@...aro.org>, <lpieralisi@...nel.org>,
<kw@...ux.com>, <robh@...nel.org>, <bhelgaas@...gle.com>
CC: <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] PCI: qcom: Avoid DBI and ATU register space mirror
to BAR/MMIO region
Hi Prudhvi
On 7/17/2024 10:12 PM, Prudhvi Yarlagadda wrote:
> PARF hardware block which is a wrapper on top of DWC PCIe controller
> mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
> register to get the size of the memory block to be mirrored and uses
> PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
> address of DBI and ATU space inside the memory block that is being
> mirrored.
>
> When a memory region which is located above the SLV_ADDR_SPACE_SIZE
> boundary is used for BAR region then there could be an overlap of DBI and
> ATU address space that is getting mirrored and the BAR region. This
> results in DBI and ATU address space contents getting updated when a PCIe
> function driver tries updating the BAR/MMIO memory region. Reference
> memory map of the PCIe memory region with DBI and ATU address space
> overlapping BAR region is as below.
>
> |---------------|
> | |
> | |
> ------- --------|---------------|
> | | |---------------|
> | | | DBI |
> | | |---------------|---->DBI_BASE_ADDR
> | | | |
> | | | |
> | PCIe | |---->2*SLV_ADDR_SPACE_SIZE
> | BAR/MMIO|---------------|
> | Region | ATU |
> | | |---------------|---->ATU_BASE_ADDR
> | | | |
> PCIe | |---------------|
> Memory | | DBI |
> Region | |---------------|---->DBI_BASE_ADDR
> | | | |
> | --------| |
> | | |---->SLV_ADDR_SPACE_SIZE
> | |---------------|
> | | ATU |
> | |---------------|---->ATU_BASE_ADDR
> | | |
> | |---------------|
> | | DBI |
> | |---------------|---->DBI_BASE_ADDR
> | | |
> | | |
> ----------------|---------------|
> | |
> | |
> | |
> |---------------|
>
> Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is not
> used for BAR region which is why the above mentioned issue is not
> encountered. This issue is discovered as part of internal testing when we
> tried moving the BAR region beyond the SLV_ADDR_SPACE_SIZE boundary. Hence
> we are trying to fix this.
>
> As PARF hardware block mirrors DBI and ATU register space after every
> PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary multiple, write
> U32_MAX (all 0xFF's) to PARF_SLV_ADDR_SPACE_SIZE register to avoid
> mirroring DBI and ATU to BAR/MMIO region. Write the physical base address
> of DBI and ATU register blocks to PARF_DBI_BASE_ADDR (default 0x0) and
> PARF_ATU_BASE_ADDR (default 0x1000) respectively to make sure DBI and ATU
> blocks are at expected memory locations.
>
> The register offsets PARF_DBI_BASE_ADDR_V2, PARF_SLV_ADDR_SPACE_SIZE_V2
> and PARF_ATU_BASE_ADDR are applicable for platforms that use PARF
> Qcom IP rev 1.9.0, 2.7.0 and 2.9.0. PARF_DBI_BASE_ADDR_V2 and
> PARF_SLV_ADDR_SPACE_SIZE_V2 are applicable for PARF Qcom IP rev 2.3.3.
> PARF_DBI_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE are applicable for PARF
> Qcom IP rev 1.0.0, 2.3.2 and 2.4.0. Updating the init()/post_init()
> functions of the respective PARF versions to program applicable
> PARF_DBI_BASE_ADDR, PARF_SLV_ADDR_SPACE_SIZE and PARF_ATU_BASE_ADDR
> register offsets. And remove the unused SLV_ADDR_SPACE_SZ macro.
>
> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@...cinc.com>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 62 +++++++++++++++++++-------
> 1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0180edf3310e..845c7641431f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -45,6 +45,7 @@
> #define PARF_PHY_REFCLK 0x4c
> #define PARF_CONFIG_BITS 0x50
> #define PARF_DBI_BASE_ADDR 0x168
> +#define PARF_SLV_ADDR_SPACE_SIZE 0x16C
> #define PARF_MHI_CLOCK_RESET_CTRL 0x174
> #define PARF_AXI_MSTR_WR_ADDR_HALT 0x178
> #define PARF_AXI_MSTR_WR_ADDR_HALT_V2 0x1a8
[...]> +static void qcom_pcie_configure_dbi_base(struct qcom_pcie *pcie)
> +{
> + struct dw_pcie *pci = pcie->pci;
> +
> + if (pci->dbi_phys_addr)
> + writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
> + PARF_DBI_BASE_ADDR);
> +
> + writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
We can't update PARF_SLV_ADDR_SPACE_SIZE without updating
PARF_DBI_BASE_ADDR.
Please make dbi_phys_addr mandatory to update PARF_SLV_ADDR_SPACE_SIZE.
> +}
> +
> +static void qcom_pcie_configure_dbi_atu_base(struct qcom_pcie *pcie)
> +{
> + struct dw_pcie *pci = pcie->pci;
> +
> + if (pci->dbi_phys_addr) {
> + writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
> + PARF_DBI_BASE_ADDR_V2);
> + writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf +
> + PARF_DBI_BASE_ADDR_V2_HI);
> + }
> +
> + if (pci->atu_phys_addr) {
> + writel(lower_32_bits(pci->atu_phys_addr), pcie->parf +
> + PARF_ATU_BASE_ADDR);
> + writel(upper_32_bits(pci->atu_phys_addr), pcie->parf +
> + PARF_ATU_BASE_ADDR_HI);
> + }
> +
> + writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2);
> + writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2_HI);
Same as above. atu_phys_addr shall be optional here but not
dbi_phys_addr to update PARF_SLV_ADDR_SPACE_SIZE.
> +}
> +
> static void qcom_pcie_2_1_0_ltssm_enable(struct qcom_pcie *pcie)
> {
> u32 val;
> @@ -540,8 +576,7 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
>
> static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> {
> - /* change DBI base address */
> - writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
> + qcom_pcie_configure_dbi_base(pcie);
>
> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> u32 val = readl(pcie->parf + PARF_AXI_MSTR_WR_ADDR_HALT);
> @@ -628,8 +663,7 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
> val &= ~PHY_TEST_PWR_DOWN;
> writel(val, pcie->parf + PARF_PHY_CTRL);
>
> - /* change DBI base address */
> - writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
> + qcom_pcie_configure_dbi_base(pcie);
>
> /* MAC PHY_POWERDOWN MUX DISABLE */
> val = readl(pcie->parf + PARF_SYS_CTRL);
> @@ -811,13 +845,11 @@ static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
> u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> u32 val;
>
> - writel(SLV_ADDR_SPACE_SZ, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> -
> val = readl(pcie->parf + PARF_PHY_CTRL);
> val &= ~PHY_TEST_PWR_DOWN;
> writel(val, pcie->parf + PARF_PHY_CTRL);
>
> - writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
> + qcom_pcie_configure_dbi_atu_base(pcie);
>
> writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
> | SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
> @@ -913,8 +945,7 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> val &= ~PHY_TEST_PWR_DOWN;
> writel(val, pcie->parf + PARF_PHY_CTRL);
>
> - /* change DBI base address */
> - writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
> + qcom_pcie_configure_dbi_atu_base(pcie);
>
> /* MAC PHY_POWERDOWN MUX DISABLE */
> val = readl(pcie->parf + PARF_SYS_CTRL);
> @@ -1123,14 +1154,11 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> u32 val;
> int i;
>
> - writel(SLV_ADDR_SPACE_SZ,
> - pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> -
> val = readl(pcie->parf + PARF_PHY_CTRL);
> val &= ~PHY_TEST_PWR_DOWN;
> writel(val, pcie->parf + PARF_PHY_CTRL);
>
> - writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
> + qcom_pcie_configure_dbi_atu_base(pcie);
>
> writel(DEVICE_TYPE_RC, pcie->parf + PARF_DEVICE_TYPE);
> writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
Powered by blists - more mailing lists