[PATCH v5 0/6] PCI endpoint additional pci_epc_set_bar() checks

[PATCH v5 0/6] PCI endpoint additional pci_epc_set_bar() checks

[Niklas Cassel]

Hello all,

This series adds some extra checks to ensure that it is not possible to
program the iATU with an address which we did not intend to use.

If these checks were in place when testing some of the earlier revisions
of Frank's doorbell patches (which did not handle fixed BARs properly),
we would gotten an error, rather than silently using an address which we
did not intend to use.

Having these checks in place will hopefully avoid similar debugging in the
future.


Kind regards,
Niklas


Changes since v4:
-Split patch 1/5 into two patches (patch 1/6 and 2/6), as suggested by Mani.
-Added Cc: stable for patch 1/6, as suggested by Mani.
-Picked up tags from Mani.


Niklas Cassel (6):
  PCI: dwc: ep: iATU registers must be written after the BAR_MASK
  PCI: dwc: ep: Add missing checks when dynamically changing a BAR
  PCI: dwc: ep: Add 'address' alignment to 'size' check in
    dw_pcie_prog_ep_inbound_atu()
  PCI: artpec6: Implement dw_pcie_ep operation get_features
  PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar()
  PCI: endpoint: Verify that requested BAR size is a power of two

 drivers/pci/controller/dwc/pcie-artpec6.c     | 13 +++++
 .../pci/controller/dwc/pcie-designware-ep.c   | 52 ++++++++++++++-----
 drivers/pci/controller/dwc/pcie-designware.c  |  5 +-
 drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
 drivers/pci/endpoint/pci-epc-core.c           | 14 ++++-
 5 files changed, 67 insertions(+), 19 deletions(-)

-- 
2.47.0

[PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Niklas Cassel]

The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
"Field size depends on log2(BAR_MASK+1) in BAR match mode."

I.e. only the upper bits are writable, and the number of writable bits is
dependent on the configured BAR_MASK.

If we do not write the BAR_MASK before writing the iATU registers, we are
relying the reset value of the BAR_MASK being larger than the requested
size of the first set_bar() call. The reset value of the BAR_MASK is SoC
dependent.

Thus, if the first set_bar() call requests a size that is larger than the
reset value of the BAR_MASK, the iATU will try to write to read-only bits,
which will cause the iATU to end up redirecting to a physical address that
is different from the address that was intended.

Thus, we should always write the iATU registers after writing the BAR_MASK.

Cc: stable@vger.kernel.org
Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 28 ++++++++++---------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index f3ac7d46a855..bad588ef69a4 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -222,19 +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
 		return -EINVAL;
 
-	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
-
-	if (!(flags & PCI_BASE_ADDRESS_SPACE))
-		type = PCIE_ATU_TYPE_MEM;
-	else
-		type = PCIE_ATU_TYPE_IO;
-
-	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
-	if (ret)
-		return ret;
-
 	if (ep->epf_bar[bar])
-		return 0;
+		goto config_atu;
+
+	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
 
 	dw_pcie_dbi_ro_wr_en(pci);
 
@@ -246,9 +237,20 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 		dw_pcie_ep_writel_dbi(ep, func_no, reg + 4, 0);
 	}
 
-	ep->epf_bar[bar] = epf_bar;
 	dw_pcie_dbi_ro_wr_dis(pci);
 
+config_atu:
+	if (!(flags & PCI_BASE_ADDRESS_SPACE))
+		type = PCIE_ATU_TYPE_MEM;
+	else
+		type = PCIE_ATU_TYPE_IO;
+
+	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
+	if (ret)
+		return ret;
+
+	ep->epf_bar[bar] = epf_bar;
+
 	return 0;
 }
 
-- 
2.47.0

[PATCH v5 2/6] PCI: dwc: ep: Add missing checks when dynamically changing a BAR

[Niklas Cassel]

In commit 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update
inbound map address") set_bar() was modified to support dynamically
changing the physical address of a BAR.

This means that set_bar() can be called twice, without ever calling
clear_bar(), as calling clear_bar() would clear the BAR's PCI address
assigned by the host).

This can only be done if the new BAR configuration doesn't fundamentally
differ from the existing BAR configuration. Add these missing checks.

While at it, add comments which clarifies the support for dynamically
changing the physical address of a BAR. (Which was also missing.)

Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index bad588ef69a4..01c739aaf61a 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -222,8 +222,28 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
 		return -EINVAL;
 
-	if (ep->epf_bar[bar])
+	/*
+	 * Certain EPF drivers dynamically change the physical address of a BAR
+	 * (i.e. they call set_bar() twice, without ever calling clear_bar(), as
+	 * calling clear_bar() would clear the BAR's PCI address assigned by the
+	 * host).
+	 */
+	if (ep->epf_bar[bar]) {
+		/*
+		 * We can only dynamically change a BAR if the new configuration
+		 * doesn't fundamentally differ from the existing configuration.
+		 */
+		if (ep->epf_bar[bar]->barno != bar ||
+		    ep->epf_bar[bar]->size != size ||
+		    ep->epf_bar[bar]->flags != flags)
+			return -EINVAL;
+
+		/*
+		 * When dynamically changing a BAR, skip writing the BAR reg, as
+		 * that would clear the BAR's PCI address assigned by the host.
+		 */
 		goto config_atu;
+	}
 
 	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
 
-- 
2.47.0

[PATCH v5 3/6] PCI: dwc: ep: Add 'address' alignment to 'size' check in dw_pcie_prog_ep_inbound_atu()

[Niklas Cassel]

dw_pcie_prog_ep_inbound_atu() is used to program an inbound iATU in
"BAR Match Mode".

A memory address returned by e.g. kmalloc() is guaranteed to have natural
alignment (aligned to the size of the allocation). It is however not
guaranteed that pci_epc_set_bar() (and thus dw_pcie_prog_ep_inbound_atu())
is supplied an address that has natural alignment. (An EPF driver can send
in an arbitrary physical address to pci_epc_set_bar().)

The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
"Field size depends on log2(BAR_MASK+1) in BAR match mode."

I.e. only the upper bits are writable, and the number of writable bits is
dependent on the configured BAR_MASK.

Add a check to ensure that the physical address programmed in the iATU is
aligned to the size of the BAR (BAR_MASK+1), as without this, we can get
hard to debug errors, as we could write to bits that are read-only (without
getting a write error), which could cause the iATU to end up redirecting to
a physical address that is different from the address that we intended.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 8 +++++---
 drivers/pci/controller/dwc/pcie-designware.c    | 5 +++--
 drivers/pci/controller/dwc/pcie-designware.h    | 2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 01c739aaf61a..4914751950cb 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -128,7 +128,8 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 }
 
 static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
-				  dma_addr_t cpu_addr, enum pci_barno bar)
+				  dma_addr_t cpu_addr, enum pci_barno bar,
+				  size_t size)
 {
 	int ret;
 	u32 free_win;
@@ -145,7 +146,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
 	}
 
 	ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
-					  cpu_addr, bar);
+					  cpu_addr, bar, size);
 	if (ret < 0) {
 		dev_err(pci->dev, "Failed to program IB window\n");
 		return ret;
@@ -265,7 +266,8 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	else
 		type = PCIE_ATU_TYPE_IO;
 
-	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
+	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar,
+				     size);
 	if (ret)
 		return ret;
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 6d6cbc8b5b2c..3c683b6119c3 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -597,11 +597,12 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
 }
 
 int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
-				int type, u64 cpu_addr, u8 bar)
+				int type, u64 cpu_addr, u8 bar, size_t size)
 {
 	u32 retries, val;
 
-	if (!IS_ALIGNED(cpu_addr, pci->region_align))
+	if (!IS_ALIGNED(cpu_addr, pci->region_align) ||
+	    !IS_ALIGNED(cpu_addr, size))
 		return -EINVAL;
 
 	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 347ab74ac35a..fc0872711672 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -491,7 +491,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
 			     u64 cpu_addr, u64 pci_addr, u64 size);
 int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
-				int type, u64 cpu_addr, u8 bar);
+				int type, u64 cpu_addr, u8 bar, size_t size);
 void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
 void dw_pcie_setup(struct dw_pcie *pci);
 void dw_pcie_iatu_detect(struct dw_pcie *pci);
-- 
2.47.0

[PATCH v5 4/6] PCI: artpec6: Implement dw_pcie_ep operation get_features

[Niklas Cassel]

All non-DWC EPC drivers implement (struct pci_epc *)->ops->get_features().
All DWC EPC drivers implement (struct dw_pcie_ep *)->ops->get_features(),
except for pcie-artpec6.c.

epc_features has been required in pci-epf-test.c since commit 6613bc2301ba
("PCI: endpoint: Fix NULL pointer dereference for ->get_features()").

A follow-up commit will make further use of epc_features in EPC core code.

Implement epc_features in the only EPC driver where it is currently not
implemented.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Jesper Nilsson <jesper.nilsson@axis.com>
---
 drivers/pci/controller/dwc/pcie-artpec6.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c b/drivers/pci/controller/dwc/pcie-artpec6.c
index f8e7283dacd4..234c8cbcae3a 100644
--- a/drivers/pci/controller/dwc/pcie-artpec6.c
+++ b/drivers/pci/controller/dwc/pcie-artpec6.c
@@ -369,9 +369,22 @@ static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
 	return 0;
 }
 
+static const struct pci_epc_features artpec6_pcie_epc_features = {
+	.linkup_notifier = false,
+	.msi_capable = true,
+	.msix_capable = false,
+};
+
+static const struct pci_epc_features *
+artpec6_pcie_get_features(struct dw_pcie_ep *ep)
+{
+	return &artpec6_pcie_epc_features;
+}
+
 static const struct dw_pcie_ep_ops pcie_ep_ops = {
 	.init = artpec6_pcie_ep_init,
 	.raise_irq = artpec6_pcie_raise_irq,
+	.get_features = artpec6_pcie_get_features,
 };
 
 static int artpec6_pcie_probe(struct platform_device *pdev)
-- 
2.47.0

[PATCH v5 5/6] PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar()

[Niklas Cassel]

A BAR of type BAR_FIXED has a fixed BAR size (the size cannot be changed).

When using pci_epf_alloc_space() to allocate backing memory for a BAR,
pci_epf_alloc_space() will always set the size to the fixed BAR size if
the BAR type is BAR_FIXED (and will give an error if you the requested size
is larger than the fixed BAR size).

However, some drivers might not call pci_epf_alloc_space() before calling
pci_epc_set_bar(), so add a check in pci_epc_set_bar() to ensure that an
EPF driver cannot set a size different from the fixed BAR size, if the BAR
type is BAR_FIXED.

The pci_epc_function_is_valid() check is removed because this check is now
done by pci_epc_get_features().

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/endpoint/pci-epc-core.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index bed7c7d1fe3c..c69c133701c9 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -609,10 +609,17 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
 int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 		    struct pci_epf_bar *epf_bar)
 {
-	int ret;
+	const struct pci_epc_features *epc_features;
+	enum pci_barno bar = epf_bar->barno;
 	int flags = epf_bar->flags;
+	int ret;
 
-	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
+	epc_features = pci_epc_get_features(epc, func_no, vfunc_no);
+	if (!epc_features)
+		return -EINVAL;
+
+	if (epc_features->bar[bar].type == BAR_FIXED &&
+	    (epc_features->bar[bar].fixed_size != epf_bar->size))
 		return -EINVAL;
 
 	if ((epf_bar->barno == BAR_5 && flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
-- 
2.47.0

[PATCH v5 6/6] PCI: endpoint: Verify that requested BAR size is a power of two

[Niklas Cassel]

When allocating a BAR using pci_epf_alloc_space(), there are checks that
round up the size to a power of two.

However, there is no check in pci_epc_set_bar() which verifies that the
requested BAR size is a power of two.

Add a power of two check in pci_epc_set_bar(), so that we don't need to
add such a check in each and every PCI endpoint controller driver.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/endpoint/pci-epc-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index c69c133701c9..6062677e9ffe 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -622,6 +622,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	    (epc_features->bar[bar].fixed_size != epf_bar->size))
 		return -EINVAL;
 
+	if (!is_power_of_2(epf_bar->size))
+		return -EINVAL;
+
 	if ((epf_bar->barno == BAR_5 && flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
 	    (flags & PCI_BASE_ADDRESS_SPACE_IO &&
 	     flags & PCI_BASE_ADDRESS_IO_MASK) ||
-- 
2.47.0

Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Manivannan Sadhasivam]

On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote:
> The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
> in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
> "Field size depends on log2(BAR_MASK+1) in BAR match mode."
> 
> I.e. only the upper bits are writable, and the number of writable bits is
> dependent on the configured BAR_MASK.
> 
> If we do not write the BAR_MASK before writing the iATU registers, we are
> relying the reset value of the BAR_MASK being larger than the requested
> size of the first set_bar() call. The reset value of the BAR_MASK is SoC
> dependent.
> 
> Thus, if the first set_bar() call requests a size that is larger than the
> reset value of the BAR_MASK, the iATU will try to write to read-only bits,
> which will cause the iATU to end up redirecting to a physical address that
> is different from the address that was intended.
> 
> Thus, we should always write the iATU registers after writing the BAR_MASK.
> 
> Cc: stable@vger.kernel.org
> Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 28 ++++++++++---------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index f3ac7d46a855..bad588ef69a4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -222,19 +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
>  		return -EINVAL;
>  
> -	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> -
> -	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> -		type = PCIE_ATU_TYPE_MEM;
> -	else
> -		type = PCIE_ATU_TYPE_IO;
> -
> -	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> -	if (ret)
> -		return ret;
> -
>  	if (ep->epf_bar[bar])
> -		return 0;
> +		goto config_atu;
> +
> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
>  
> @@ -246,9 +237,20 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  		dw_pcie_ep_writel_dbi(ep, func_no, reg + 4, 0);
>  	}
>  
> -	ep->epf_bar[bar] = epf_bar;
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  
> +config_atu:
> +	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> +		type = PCIE_ATU_TYPE_MEM;
> +	else
> +		type = PCIE_ATU_TYPE_IO;
> +
> +	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> +	if (ret)
> +		return ret;
> +
> +	ep->epf_bar[bar] = epf_bar;
> +
>  	return 0;
>  }
>  
> -- 
> 2.47.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 2/6] PCI: dwc: ep: Add missing checks when dynamically changing a BAR

[Manivannan Sadhasivam]

On Wed, Nov 27, 2024 at 11:30:18AM +0100, Niklas Cassel wrote:
> In commit 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update
> inbound map address") set_bar() was modified to support dynamically
> changing the physical address of a BAR.
> 
> This means that set_bar() can be called twice, without ever calling
> clear_bar(), as calling clear_bar() would clear the BAR's PCI address
> assigned by the host).
> 
> This can only be done if the new BAR configuration doesn't fundamentally
> differ from the existing BAR configuration. Add these missing checks.
> 
> While at it, add comments which clarifies the support for dynamically
> changing the physical address of a BAR. (Which was also missing.)
> 
> Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index bad588ef69a4..01c739aaf61a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -222,8 +222,28 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
>  		return -EINVAL;
>  
> -	if (ep->epf_bar[bar])
> +	/*
> +	 * Certain EPF drivers dynamically change the physical address of a BAR
> +	 * (i.e. they call set_bar() twice, without ever calling clear_bar(), as
> +	 * calling clear_bar() would clear the BAR's PCI address assigned by the
> +	 * host).
> +	 */
> +	if (ep->epf_bar[bar]) {
> +		/*
> +		 * We can only dynamically change a BAR if the new configuration
> +		 * doesn't fundamentally differ from the existing configuration.
> +		 */
> +		if (ep->epf_bar[bar]->barno != bar ||
> +		    ep->epf_bar[bar]->size != size ||
> +		    ep->epf_bar[bar]->flags != flags)
> +			return -EINVAL;
> +
> +		/*
> +		 * When dynamically changing a BAR, skip writing the BAR reg, as
> +		 * that would clear the BAR's PCI address assigned by the host.
> +		 */
>  		goto config_atu;
> +	}
>  
>  	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>  
> -- 
> 2.47.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 2/6] PCI: dwc: ep: Add missing checks when dynamically changing a BAR

[Bjorn Helgaas]

[+cc NTB list, since NTB seems to be the main user of .set_bar()]

Can we say something specific in the subject line, like "prevent
changing size/flags" or whatever?

On Wed, Nov 27, 2024 at 11:30:18AM +0100, Niklas Cassel wrote:
> In commit 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update
> inbound map address") set_bar() was modified to support dynamically
> changing the physical address of a BAR.
> 
> This means that set_bar() can be called twice, without ever calling
> clear_bar(), as calling clear_bar() would clear the BAR's PCI address
> assigned by the host).

Unpaired parenthesis at end.

Apparently calling .set_bar() twice without calling .clear_bar() is a
problem?  What problem does it cause?  Sorry about my poor
understanding of the endpoint and NTB code; I'm sure this would be
obvious if I understood more.

Maybe a hint about the reason why we need to call .set_bar() twice
would help me understand.

> This can only be done if the new BAR configuration doesn't fundamentally
> differ from the existing BAR configuration. Add these missing checks.

Can you elaborate a bit on what "fundamentally differ" means?  Based
on the patch, I guess it has to do with changing the size or type?

And the problem this would cause is ...?  I guess it's a problem for
some other entity that knows about the BAR and is doing MMIO or DMA to
it?

> While at it, add comments which clarifies the support for dynamically
> changing the physical address of a BAR. (Which was also missing.)
> 
> Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index bad588ef69a4..01c739aaf61a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -222,8 +222,28 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
>  		return -EINVAL;
>  
> -	if (ep->epf_bar[bar])
> +	/*
> +	 * Certain EPF drivers dynamically change the physical address of a BAR
> +	 * (i.e. they call set_bar() twice, without ever calling clear_bar(), as
> +	 * calling clear_bar() would clear the BAR's PCI address assigned by the
> +	 * host).
> +	 */
> +	if (ep->epf_bar[bar]) {
> +		/*
> +		 * We can only dynamically change a BAR if the new configuration
> +		 * doesn't fundamentally differ from the existing configuration.
> +		 */
> +		if (ep->epf_bar[bar]->barno != bar ||
> +		    ep->epf_bar[bar]->size != size ||
> +		    ep->epf_bar[bar]->flags != flags)
> +			return -EINVAL;
> +
> +		/*
> +		 * When dynamically changing a BAR, skip writing the BAR reg, as
> +		 * that would clear the BAR's PCI address assigned by the host.
> +		 */
>  		goto config_atu;
> +	}
>  
>  	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>  
> -- 
> 2.47.0
> 

Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Bjorn Helgaas]

In subject, maybe "Write BAR_MASK before iATU registers"

I guess writing BAR_MASK is really configuring the *size* of the BAR?
Maybe the size is the important semantic connection with iATU config?

On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote:
> The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
> in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
> "Field size depends on log2(BAR_MASK+1) in BAR match mode."

Can we include a databook revision and section here to help future
maintainers?

> I.e. only the upper bits are writable, and the number of writable bits is
> dependent on the configured BAR_MASK.
> 
> If we do not write the BAR_MASK before writing the iATU registers, we are
> relying the reset value of the BAR_MASK being larger than the requested
> size of the first set_bar() call. The reset value of the BAR_MASK is SoC
> dependent.
> 
> Thus, if the first set_bar() call requests a size that is larger than the
> reset value of the BAR_MASK, the iATU will try to write to read-only bits,
> which will cause the iATU to end up redirecting to a physical address that
> is different from the address that was intended.
> 
> Thus, we should always write the iATU registers after writing the BAR_MASK.

Apparently we write BAR_MASK and the iATU registers in the wrong
order?  I assume dw_pcie_ep_inbound_atu() writes the iATU registers.

I can't quite connect the commit log with the code change.  I assume
the dw_pcie_ep_writel_dbi2() and dw_pcie_ep_writel_dbi() writes update
BAR_MASK?

And I guess the problem is that the previous code does:

  dw_pcie_ep_inbound_atu        # iATU
  dw_pcie_ep_writel_dbi2        # BAR_MASK (?)
  dw_pcie_ep_writel_dbi

and the new code basically does this:

  if (ep->epf_bar[bar]) {
    dw_pcie_ep_writel_dbi2      # BAR_MASK (?)
    dw_pcie_ep_writel_dbi
  }
  dw_pcie_ep_inbound_atu        # iATU
  ep->epf_bar[bar] = epf_bar

so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK
before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the 
BAR_MASK update?

> Cc: stable@vger.kernel.org
> Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 28 ++++++++++---------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index f3ac7d46a855..bad588ef69a4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -222,19 +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
>  		return -EINVAL;
>  
> -	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> -
> -	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> -		type = PCIE_ATU_TYPE_MEM;
> -	else
> -		type = PCIE_ATU_TYPE_IO;
> -
> -	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> -	if (ret)
> -		return ret;
> -
>  	if (ep->epf_bar[bar])
> -		return 0;
> +		goto config_atu;
> +
> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
>  
> @@ -246,9 +237,20 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  		dw_pcie_ep_writel_dbi(ep, func_no, reg + 4, 0);
>  	}
>  
> -	ep->epf_bar[bar] = epf_bar;
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  
> +config_atu:
> +	if (!(flags & PCI_BASE_ADDRESS_SPACE))
> +		type = PCIE_ATU_TYPE_MEM;
> +	else
> +		type = PCIE_ATU_TYPE_IO;
> +
> +	ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> +	if (ret)
> +		return ret;
> +
> +	ep->epf_bar[bar] = epf_bar;
> +
>  	return 0;
>  }
>  
> -- 
> 2.47.0
> 

Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Niklas Cassel]

Hello Bjorn,

On Wed, Dec 04, 2024 at 11:33:52AM -0600, Bjorn Helgaas wrote:
> In subject, maybe "Write BAR_MASK before iATU registers"

Ok. Will fix in v6.


> 
> I guess writing BAR_MASK is really configuring the *size* of the BAR?

I am quite sure that you know how host software determines the size of
a BAR :)

But yes, writing the BAR_MASK will directly decide a BARs size:
https://wiki.osdev.org/PCI#Address_and_size_of_the_BAR

So BAR_MASK is "BAR size - 1".


> Maybe the size is the important semantic connection with iATU config?

The connection is:
"Field size depends on log2(BAR_MASK+1) in BAR match mode."

So I think it makes sense for the subject to include BAR_MASK
rather than BAR size.


> 
> On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote:
> > The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
> > in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
> > "Field size depends on log2(BAR_MASK+1) in BAR match mode."
> 
> Can we include a databook revision and section here to help future
> maintainers?

Ok. Will fix in v6.


> 
> > I.e. only the upper bits are writable, and the number of writable bits is
> > dependent on the configured BAR_MASK.
> > 
> > If we do not write the BAR_MASK before writing the iATU registers, we are
> > relying the reset value of the BAR_MASK being larger than the requested
> > size of the first set_bar() call. The reset value of the BAR_MASK is SoC
> > dependent.
> > 
> > Thus, if the first set_bar() call requests a size that is larger than the
> > reset value of the BAR_MASK, the iATU will try to write to read-only bits,
> > which will cause the iATU to end up redirecting to a physical address that
> > is different from the address that was intended.
> > 
> > Thus, we should always write the iATU registers after writing the BAR_MASK.
> 
> Apparently we write BAR_MASK and the iATU registers in the wrong
> order?  I assume dw_pcie_ep_inbound_atu() writes the iATU registers.

Yes.


> 
> I can't quite connect the commit log with the code change.  I assume
> the dw_pcie_ep_writel_dbi2() and dw_pcie_ep_writel_dbi() writes update
> BAR_MASK?

dw_pcie_ep_writel_dbi2() writes the BAR_MASK.
dw_pcie_ep_writel_dbi() writes the BAR type.


> 
> And I guess the problem is that the previous code does:
> 
>   dw_pcie_ep_inbound_atu        # iATU
>   dw_pcie_ep_writel_dbi2        # BAR_MASK (?)
>   dw_pcie_ep_writel_dbi
> 
> and the new code basically does this:
> 
>   if (ep->epf_bar[bar]) {
>     dw_pcie_ep_writel_dbi2      # BAR_MASK (?)
>     dw_pcie_ep_writel_dbi
>   }
>   dw_pcie_ep_inbound_atu        # iATU
>   ep->epf_bar[bar] = epf_bar
> 
> so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK
> before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the 
> BAR_MASK update?

The problem is as described in the commit message:
"If we do not write the BAR_MASK before writing the iATU registers, we are
relying the reset value of the BAR_MASK being larger than the requested
size of the first set_bar() call. The reset value of the BAR_MASK is SoC
dependent."


Before:
dw_pcie_ep_inbound_atu() # iATU - the writable bits in this write depends on
                         # BAR_MASK, which has not been written yet, thus the
			 # write will be at the mercy of the reset value of
			 # BAR_MASK, which is SoC dependent.
dw_pcie_ep_writel_dbi2() # BAR_MASK
dw_pcie_ep_writel_dbi()  # BAR type

After:
dw_pcie_ep_writel_dbi2() # BAR_MASK
dw_pcie_ep_writel_dbi()  # BAR type
dw_pcie_ep_inbound_atu() # iATU - this write is now done after BAR_MASK has
			 # been written, so we know that all the bits in this
			 # write is writable.


Kind regards,
Niklas

Re: [PATCH v5 2/6] PCI: dwc: ep: Add missing checks when dynamically changing a BAR

[Niklas Cassel]

On Wed, Dec 04, 2024 at 11:17:16AM -0600, Bjorn Helgaas wrote:
> [+cc NTB list, since NTB seems to be the main user of .set_bar()]
> 
> Can we say something specific in the subject line, like "prevent
> changing size/flags" or whatever?

Ok. Will fix in v6.


> 
> On Wed, Nov 27, 2024 at 11:30:18AM +0100, Niklas Cassel wrote:
> > In commit 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update
> > inbound map address") set_bar() was modified to support dynamically
> > changing the physical address of a BAR.
> > 
> > This means that set_bar() can be called twice, without ever calling
> > clear_bar(), as calling clear_bar() would clear the BAR's PCI address
> > assigned by the host).
> 
> Unpaired parenthesis at end.

Ok. Will fix in v6.


> 
> Apparently calling .set_bar() twice without calling .clear_bar() is a
> problem?  What problem does it cause?  Sorry about my poor
> understanding of the endpoint and NTB code; I'm sure this would be
> obvious if I understood more.

Calling .set_bar() without calling .clear_bar() is not a problem.
In fact, that is how the NTB EPF dynamically switches the inbound address
translation for a BAR on the fly, after the host has assigned a PCI address
for the BAR.

It is just that for most EPF drivers, you would normally call .clear_bar()
before calling .set_bar() again.


> 
> Maybe a hint about the reason why we need to call .set_bar() twice
> would help me understand.

The NTB EPF does this to change the inbound address translation from one
address to another on the fly. For details about why the NTB EPF driver does
this, you would need to ask Frank who implemented this.


> 
> > This can only be done if the new BAR configuration doesn't fundamentally
> > differ from the existing BAR configuration. Add these missing checks.
> 
> Can you elaborate a bit on what "fundamentally differ" means?  Based
> on the patch, I guess it has to do with changing the size or type?

Correct, BAR size and BAR type has to be the same.
I don't know why the NTB EPF does this, but it is obvious that very bad
things could happen if you would dynamically change from one BAR size to
another. (This is not a resizable BAR...)


> 
> And the problem this would cause is ...?  I guess it's a problem for
> some other entity that knows about the BAR and is doing MMIO or DMA to
> it?

If we allowed the BAR sizes of the old area and the new area to differ,
that could mean that we could leak kernel memory, since if e.g. the new area
is smaller than the old area, a read past the new area size would potentially
read memory that wasn't allocated by the EPF.

To me, is seems like quite a big oversight that these safe guards wasn't
added as part of commit 4284c88fff0e ("PCI: designware-ep: Allow
pci_epc_set_bar() update inbound map address"), but then as you might have
seen, that patch was also merged via the NTB tree without any Ack from the
PCI endpoint maintainers. This patch has a Fixes-tag against that commit,
so stable should pick this up. I will also add an explict Cc: stable in v6.


Kind regards,
Niklas

Re: [PATCH v5 1/6] PCI: dwc: ep: iATU registers must be written after the BAR_MASK

[Niklas Cassel]

Hello Bjorn,

On Fri, Dec 13, 2024 at 02:34:35PM +0100, Niklas Cassel wrote:
> > 
> > And I guess the problem is that the previous code does:
> > 
> >   dw_pcie_ep_inbound_atu        # iATU
> >   dw_pcie_ep_writel_dbi2        # BAR_MASK (?)
> >   dw_pcie_ep_writel_dbi
> > 
> > and the new code basically does this:
> > 
> >   if (ep->epf_bar[bar]) {
> >     dw_pcie_ep_writel_dbi2      # BAR_MASK (?)
> >     dw_pcie_ep_writel_dbi
> >   }
> >   dw_pcie_ep_inbound_atu        # iATU
> >   ep->epf_bar[bar] = epf_bar
> > 
> > so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK
> > before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the 
> > BAR_MASK update?
> 
> The problem is as described in the commit message:
> "If we do not write the BAR_MASK before writing the iATU registers, we are
> relying the reset value of the BAR_MASK being larger than the requested
> size of the first set_bar() call. The reset value of the BAR_MASK is SoC
> dependent."

Re-reading this commit message, I can see that it is a bit confusing.

I re-wrote the commit messages for patch 1/2 and patch 2/6 in this series,
based on your feedback. (I also updated the code comment in patch 2/6.)

I hope that it slightly clearer now :)

Please review:
https://lore.kernel.org/linux-pci/20241213143301.4158431-8-cassel@kernel.org/T/#u


Kind regards,
Niklas