[PATCH 1/3] arm: a37xx: pci: Extend validation for PCIe resources and oubound windows

[PATCH 1/3] arm: a37xx: pci: Extend validation for PCIe resources and oubound windows

[Pali Rohár]

Remapped address of PCIe outbound window may have set only bits from the
mask. Add additional check that remapped address which is calculated from
PCIe bus address specified in DTS file is valid.

Remove also useless clearing of low 16 bits in win_mask. As win_size is
power of two and is at least 0x10000 it means that it always has zero low
16 bits.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/pci-aardvark.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
index 96aa039bdc26..77ce9d0c8d87 100644
--- a/drivers/pci/pci-aardvark.c
+++ b/drivers/pci/pci-aardvark.c
@@ -609,21 +609,22 @@ static void pcie_advk_set_ob_region(struct pcie_advk *pcie, int *wins,
 	 * match with given mask.
 	 * So every PCIe window size must be a power of two and every start
 	 * address must be aligned to window size. Minimal size is 64 KiB
-	 * because lower 16 bits of mask must be zero.
+	 * because lower 16 bits of mask must be zero. Remapped address
+	 * may have set only bits from the mask.
 	 */
 	while (*wins < OB_WIN_COUNT && size > 0) {
 		/* Calculate the largest aligned window size */
 		win_size = (1ULL << (fls64(size) - 1)) |
 			   (phys_start ? (1ULL << __ffs64(phys_start)) : 0);
 		win_size = 1ULL << __ffs64(win_size);
-		if (win_size < 0x10000)
+		win_mask = ~(win_size - 1);
+		if (win_size < 0x10000 || (bus_start & ~win_mask))
 			break;
 
 		dev_dbg(pcie->dev,
 			"Configuring PCIe window %d: [0x%llx-0x%llx] as 0x%x\n",
 			*wins, (u64)phys_start, (u64)phys_start + win_size,
 			actions);
-		win_mask = ~(win_size - 1) & ~0xffff;
 		pcie_advk_set_ob_win(pcie, *wins, phys_start, bus_start,
 				     win_mask, actions);
 
-- 
2.20.1

[PATCH 2/3] arm: a37xx: pci: Optimize a3700_fdt_fix_pcie_regions() when fixup offset is zero

[Pali Rohár]

If fixup offset is zero then there is nothing to fix. All calculation in
this case just increase addresses by value zero which results in identity.
So in this case skip whole fixup re-calculation as it is not needed.

This is just an optimization for special case when fix_offset is zero which
skips code path which does only identity operations (meaning nothing). No
functional changes.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/arm/mach-mvebu/armada3700/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c
index 4dd9de471b4e..64838b16c4d8 100644
--- a/arch/arm/mach-mvebu/armada3700/cpu.c
+++ b/arch/arm/mach-mvebu/armada3700/cpu.c
@@ -339,6 +339,10 @@ int a3700_fdt_fix_pcie_regions(void *blob)
 	/* Calculate fixup offset from first child address (in last cell) */
 	fix_offset = base - fdt32_to_cpu(ranges[2]);
 
+	/* If fixup offset is zero then there is nothing to fix */
+	if (!fix_offset)
+		return 0;
+
 	/*
 	 * Fix address (last cell) of each child address and each parent
 	 * address
-- 
2.20.1

RE: [EXT] [PATCH 2/3] arm: a37xx: pci: Optimize a3700_fdt_fix_pcie_regions() when fixup offset is zero

[Kostya Porotchkin]

> -----Original Message-----
> From: Pali Rohár <pali@kernel.org>
> Sent: Thursday, July 8, 2021 21:19
> To: Stefan Roese <sr@denx.de>; Kostya Porotchkin <kostap@marvell.com>
> Cc: Marek Behún <marek.behun@nic.cz>; u-boot@lists.denx.de
> Subject: [EXT] [PATCH 2/3] arm: a37xx: pci: Optimize
> a3700_fdt_fix_pcie_regions() when fixup offset is zero
> 
> External Email
> 
> ----------------------------------------------------------------------
> If fixup offset is zero then there is nothing to fix. All calculation in this case just
> increase addresses by value zero which results in identity.
> So in this case skip whole fixup re-calculation as it is not needed.
> 
> This is just an optimization for special case when fix_offset is zero which skips
> code path which does only identity operations (meaning nothing). No functional
> changes.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  arch/arm/mach-mvebu/armada3700/cpu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-
> mvebu/armada3700/cpu.c
> index 4dd9de471b4e..64838b16c4d8 100644
> --- a/arch/arm/mach-mvebu/armada3700/cpu.c
> +++ b/arch/arm/mach-mvebu/armada3700/cpu.c
> @@ -339,6 +339,10 @@ int a3700_fdt_fix_pcie_regions(void *blob)
>  	/* Calculate fixup offset from first child address (in last cell) */
>  	fix_offset = base - fdt32_to_cpu(ranges[2]);
> 
> +	/* If fixup offset is zero then there is nothing to fix */
> +	if (!fix_offset)
> +		return 0;
> +
>  	/*
>  	 * Fix address (last cell) of each child address and each parent
>  	 * address
> --
> 2.20.1
Reviewed-by: Konstantin Porotchkin <kostap@marvell.com>

Re: [PATCH 2/3] arm: a37xx: pci: Optimize a3700_fdt_fix_pcie_regions() when fixup offset is zero

[Stefan Roese]

On 08.07.21 20:18, Pali Rohár wrote:
> If fixup offset is zero then there is nothing to fix. All calculation in
> this case just increase addresses by value zero which results in identity.
> So in this case skip whole fixup re-calculation as it is not needed.
> 
> This is just an optimization for special case when fix_offset is zero which
> skips code path which does only identity operations (meaning nothing). No
> functional changes.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   arch/arm/mach-mvebu/armada3700/cpu.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c
> index 4dd9de471b4e..64838b16c4d8 100644
> --- a/arch/arm/mach-mvebu/armada3700/cpu.c
> +++ b/arch/arm/mach-mvebu/armada3700/cpu.c
> @@ -339,6 +339,10 @@ int a3700_fdt_fix_pcie_regions(void *blob)
>   	/* Calculate fixup offset from first child address (in last cell) */
>   	fix_offset = base - fdt32_to_cpu(ranges[2]);
>   
> +	/* If fixup offset is zero then there is nothing to fix */
> +	if (!fix_offset)
> +		return 0;
> +
>   	/*
>   	 * Fix address (last cell) of each child address and each parent
>   	 * address
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH 3/3] arm: a37xx: pci: Fix typo in comment

[Pali Rohár]

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
index 77ce9d0c8d87..1b9bae7cca72 100644
--- a/drivers/pci/pci-aardvark.c
+++ b/drivers/pci/pci-aardvark.c
@@ -605,7 +605,7 @@ static void pcie_advk_set_ob_region(struct pcie_advk *pcie, int *wins,
 
 	/*
 	 * The n-th PCIe window is configured by tuple (match, remap, mask)
-	 * and an access to address A uses this window it if A matches the
+	 * and an access to address A uses this window if A matches the
 	 * match with given mask.
 	 * So every PCIe window size must be a power of two and every start
 	 * address must be aligned to window size. Minimal size is 64 KiB
-- 
2.20.1

RE: [EXT] [PATCH 3/3] arm: a37xx: pci: Fix typo in comment

[Kostya Porotchkin]

> -----Original Message-----
> From: Pali Rohár <pali@kernel.org>
> Sent: Thursday, July 8, 2021 21:19
> To: Stefan Roese <sr@denx.de>; Kostya Porotchkin <kostap@marvell.com>
> Cc: Marek Behún <marek.behun@nic.cz>; u-boot@lists.denx.de
> Subject: [EXT] [PATCH 3/3] arm: a37xx: pci: Fix typo in comment
> 
> External Email
> 
> ----------------------------------------------------------------------
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/pci-aardvark.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c index
> 77ce9d0c8d87..1b9bae7cca72 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -605,7 +605,7 @@ static void pcie_advk_set_ob_region(struct pcie_advk
> *pcie, int *wins,
> 
>  	/*
>  	 * The n-th PCIe window is configured by tuple (match, remap, mask)
> -	 * and an access to address A uses this window it if A matches the
> +	 * and an access to address A uses this window if A matches the
>  	 * match with given mask.
>  	 * So every PCIe window size must be a power of two and every start
>  	 * address must be aligned to window size. Minimal size is 64 KiB
> --
> 2.20.1
Reviewed-by: Konstantin Porotchkin <kostap@marvell.com>

Re: [PATCH 3/3] arm: a37xx: pci: Fix typo in comment

[Stefan Roese]

On 08.07.21 20:19, Pali Rohár wrote:
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 77ce9d0c8d87..1b9bae7cca72 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -605,7 +605,7 @@ static void pcie_advk_set_ob_region(struct pcie_advk *pcie, int *wins,
>   
>   	/*
>   	 * The n-th PCIe window is configured by tuple (match, remap, mask)
> -	 * and an access to address A uses this window it if A matches the
> +	 * and an access to address A uses this window if A matches the
>   	 * match with given mask.
>   	 * So every PCIe window size must be a power of two and every start
>   	 * address must be aligned to window size. Minimal size is 64 KiB
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

RE: [EXT] [PATCH 1/3] arm: a37xx: pci: Extend validation for PCIe resources and oubound windows

[Kostya Porotchkin]

> -----Original Message-----
> From: Pali Rohár <pali@kernel.org>
> Sent: Thursday, July 8, 2021 21:19
> To: Stefan Roese <sr@denx.de>; Kostya Porotchkin <kostap@marvell.com>
> Cc: Marek Behún <marek.behun@nic.cz>; u-boot@lists.denx.de
> Subject: [EXT] [PATCH 1/3] arm: a37xx: pci: Extend validation for PCIe
> resources and oubound windows
> 
> External Email
> 
> ----------------------------------------------------------------------
> Remapped address of PCIe outbound window may have set only bits from the
> mask. Add additional check that remapped address which is calculated from
> PCIe bus address specified in DTS file is valid.
> 
> Remove also useless clearing of low 16 bits in win_mask. As win_size is power
> of two and is at least 0x10000 it means that it always has zero low
> 16 bits.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/pci-aardvark.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c index
> 96aa039bdc26..77ce9d0c8d87 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -609,21 +609,22 @@ static void pcie_advk_set_ob_region(struct
> pcie_advk *pcie, int *wins,
>  	 * match with given mask.
>  	 * So every PCIe window size must be a power of two and every start
>  	 * address must be aligned to window size. Minimal size is 64 KiB
> -	 * because lower 16 bits of mask must be zero.
> +	 * because lower 16 bits of mask must be zero. Remapped address
> +	 * may have set only bits from the mask.
>  	 */
>  	while (*wins < OB_WIN_COUNT && size > 0) {
>  		/* Calculate the largest aligned window size */
>  		win_size = (1ULL << (fls64(size) - 1)) |
>  			   (phys_start ? (1ULL << __ffs64(phys_start)) : 0);
>  		win_size = 1ULL << __ffs64(win_size);
> -		if (win_size < 0x10000)
> +		win_mask = ~(win_size - 1);
> +		if (win_size < 0x10000 || (bus_start & ~win_mask))
>  			break;
> 
>  		dev_dbg(pcie->dev,
>  			"Configuring PCIe window %d: [0x%llx-0x%llx] as
> 0x%x\n",
>  			*wins, (u64)phys_start, (u64)phys_start + win_size,
>  			actions);
> -		win_mask = ~(win_size - 1) & ~0xffff;
>  		pcie_advk_set_ob_win(pcie, *wins, phys_start, bus_start,
>  				     win_mask, actions);
> 
> --
> 2.20.1
Reviewed-by: Konstantin Porotchkin <kostap@marvell.com>

Re: [PATCH 1/3] arm: a37xx: pci: Extend validation for PCIe resources and oubound windows

[Stefan Roese]

On 08.07.21 20:18, Pali Rohár wrote:
> Remapped address of PCIe outbound window may have set only bits from the
> mask. Add additional check that remapped address which is calculated from
> PCIe bus address specified in DTS file is valid.
> 
> Remove also useless clearing of low 16 bits in win_mask. As win_size is
> power of two and is at least 0x10000 it means that it always has zero low
> 16 bits.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 96aa039bdc26..77ce9d0c8d87 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -609,21 +609,22 @@ static void pcie_advk_set_ob_region(struct pcie_advk *pcie, int *wins,
>   	 * match with given mask.
>   	 * So every PCIe window size must be a power of two and every start
>   	 * address must be aligned to window size. Minimal size is 64 KiB
> -	 * because lower 16 bits of mask must be zero.
> +	 * because lower 16 bits of mask must be zero. Remapped address
> +	 * may have set only bits from the mask.
>   	 */
>   	while (*wins < OB_WIN_COUNT && size > 0) {
>   		/* Calculate the largest aligned window size */
>   		win_size = (1ULL << (fls64(size) - 1)) |
>   			   (phys_start ? (1ULL << __ffs64(phys_start)) : 0);
>   		win_size = 1ULL << __ffs64(win_size);
> -		if (win_size < 0x10000)
> +		win_mask = ~(win_size - 1);
> +		if (win_size < 0x10000 || (bus_start & ~win_mask))
>   			break;
>   
>   		dev_dbg(pcie->dev,
>   			"Configuring PCIe window %d: [0x%llx-0x%llx] as 0x%x\n",
>   			*wins, (u64)phys_start, (u64)phys_start + win_size,
>   			actions);
> -		win_mask = ~(win_size - 1) & ~0xffff;
>   		pcie_advk_set_ob_win(pcie, *wins, phys_start, bus_start,
>   				     win_mask, actions);
>   
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 1/3] arm: a37xx: pci: Extend validation for PCIe resources and oubound windows

[Stefan Roese]

On 08.07.21 20:18, Pali Rohár wrote:
> Remapped address of PCIe outbound window may have set only bits from the
> mask. Add additional check that remapped address which is calculated from
> PCIe bus address specified in DTS file is valid.
> 
> Remove also useless clearing of low 16 bits in win_mask. As win_size is
> power of two and is at least 0x10000 it means that it always has zero low
> 16 bits.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

All patches:

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 96aa039bdc26..77ce9d0c8d87 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -609,21 +609,22 @@ static void pcie_advk_set_ob_region(struct pcie_advk *pcie, int *wins,
>   	 * match with given mask.
>   	 * So every PCIe window size must be a power of two and every start
>   	 * address must be aligned to window size. Minimal size is 64 KiB
> -	 * because lower 16 bits of mask must be zero.
> +	 * because lower 16 bits of mask must be zero. Remapped address
> +	 * may have set only bits from the mask.
>   	 */
>   	while (*wins < OB_WIN_COUNT && size > 0) {
>   		/* Calculate the largest aligned window size */
>   		win_size = (1ULL << (fls64(size) - 1)) |
>   			   (phys_start ? (1ULL << __ffs64(phys_start)) : 0);
>   		win_size = 1ULL << __ffs64(win_size);
> -		if (win_size < 0x10000)
> +		win_mask = ~(win_size - 1);
> +		if (win_size < 0x10000 || (bus_start & ~win_mask))
>   			break;
>   
>   		dev_dbg(pcie->dev,
>   			"Configuring PCIe window %d: [0x%llx-0x%llx] as 0x%x\n",
>   			*wins, (u64)phys_start, (u64)phys_start + win_size,
>   			actions);
> -		win_mask = ~(win_size - 1) & ~0xffff;
>   		pcie_advk_set_ob_win(pcie, *wins, phys_start, bus_start,
>   				     win_mask, actions);
>   
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de