Re: [PATCH RESEND v4 13/15] PCI: dwc: Verify in/out regions against iATU constraints

[Manivannan Sadhasivam]

On Fri, Jun 24, 2022 at 05:39:45PM +0300, Serge Semin wrote:
> Since the DWC PCIe driver private data now contains the iATU inbound and
> outbound regions constraints info like alignment, minimum and maximum
> limits, we can use them to make the in- and outbound iATU regions setup
> methods more strict to the ranges a callee tries to specify.  That will
> give us the safer dw_pcie_prog_outbound_atu(),
> dw_pcie_prog_ep_outbound_atu() and dw_pcie_prog_inbound_atu() functions.
> 
> First of all let's update the outbound ATU entries setup methods to
> returning the operation status. The methods will fail either in case if
> the range is failed to be activated or the passed region doesn't fulfill
> iATU constraints. Secondly the passed to the
> dw_pcie_prog_{ep_}outbound_atu() methods region-related parameters are
> verified against the detected iATU regions constraints. In particular the
> region limit address must not overflow the lower/upper limit CSR RW-fields
> otherwise the specified range will be just silently clamped. That
> verification will also protect the code from having u64 type overflow.
> Secondly let's make sure base address (CPU-address), target address
> (PCI-address) and size are properly aligned. Unaligned ranges will be
> silently aligned down (addresses) and up (limit) on writing the values to
> the corresponding registers, which in it turn may lead to unpredictable
> results like ranges virtual overlap. Finally the CPU-address alignment
> needs to be verified in the dw_pcie_prog_inbound_atu() method too as the
> DWC PCIe RC/EP registers manual demands seeing the lower bits of the in-
> and outbound iATU base address are always zeros.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>

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

Thanks,
Mani

> Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> 
> ---
> 
> Changelog v3:
> - Drop outbound iATU window size alignment constraint. (@Manivannan)
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 38 +++++++++++++-------
>  drivers/pci/controller/dwc/pcie-designware.h | 10 +++---
>  2 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 776752891d11..9c622b635fdd 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -8,6 +8,7 @@
>   * Author: Jingoo Han <jg1.han@xxxxxxxxxxx>
>   */
>  
> +#include <linux/align.h>
>  #include <linux/bitops.h>
>  #include <linux/delay.h>
>  #include <linux/of.h>
> @@ -308,9 +309,9 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
>  	return val | PCIE_ATU_TD;
>  }
>  
> -static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> -					int index, int type, u64 cpu_addr,
> -					u64 pci_addr, u64 size)
> +static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> +				       int index, int type, u64 cpu_addr,
> +				       u64 pci_addr, u64 size)
>  {
>  	u32 retries, val;
>  	u64 limit_addr;
> @@ -320,6 +321,12 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  
>  	limit_addr = cpu_addr + size - 1;
>  
> +	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
> +	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> +	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> +		return -EINVAL;
> +	}
> +
>  	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
>  			      lower_32_bits(cpu_addr));
>  	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> @@ -353,27 +360,29 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
>  		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
>  		if (val & PCIE_ATU_ENABLE)
> -			return;
> +			return 0;
>  
>  		mdelay(LINK_WAIT_IATU);
>  	}
>  
>  	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
> +
> +	return -ETIMEDOUT;
>  }
>  
> -void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> -			       u64 cpu_addr, u64 pci_addr, u64 size)
> +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> +			      u64 cpu_addr, u64 pci_addr, u64 size)
>  {
> -	__dw_pcie_prog_outbound_atu(pci, 0, index, type,
> -				    cpu_addr, pci_addr, size);
> +	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
> +					   cpu_addr, pci_addr, size);
>  }
>  
> -void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -				  int type, u64 cpu_addr, u64 pci_addr,
> -				  u64 size)
> +int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> +				 int type, u64 cpu_addr, u64 pci_addr,
> +				 u64 size)
>  {
> -	__dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> -				    cpu_addr, pci_addr, size);
> +	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> +					   cpu_addr, pci_addr, size);
>  }
>  
>  static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
> @@ -392,6 +401,9 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  {
>  	u32 retries, val;
>  
> +	if (!IS_ALIGNED(cpu_addr, pci->region_align))
> +		return -EINVAL;
> +
>  	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
>  			      lower_32_bits(cpu_addr));
>  	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_TARGET,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 25c86771c810..60f1ddc54933 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -304,12 +304,10 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
>  int dw_pcie_link_up(struct dw_pcie *pci);
>  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
>  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> -void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
> -			       int type, u64 cpu_addr, u64 pci_addr,
> -			       u64 size);
> -void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -				  int type, u64 cpu_addr, u64 pci_addr,
> -				  u64 size);
> +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> +			      u64 cpu_addr, u64 pci_addr, u64 size);
> +int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> +				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
>  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  			     int type, u64 cpu_addr, u8 bar);
>  void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
> -- 
> 2.35.1
> 

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