Re: [PATCH u-boot-marvell] arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function again
On 2/23/22 13:52, Marek Behún wrote: From: Pali Rohár The a3700_fdt_fix_pcie_regions() function still computes nonsense. It computes the fixup offset from the PCI address taken from the first row of the "ranges" array, which means that: - PCI address must equal CPU address (otherwise the computed fix offset will be wrong), - the first row must contain the lowest address. This is the case for the default device-tree, which is why we didn't notice it. It also adds the fixup offset to all PCI and CPU addresses, which is wrong. Instead: 1) The fixup offset must be computed from the CPU address, not PCI address. 2) The fixup offset must be computed from the row containing the lowest CPU address, which is not necessarily contained in the first row. 3) The PCI address - the address to which the PCIe controller remaps the address space as seen from the point of view of the PCIe device - must be fixed by the fix offset in the same way as the CPU address only in the special case when the CPU adn PCI addresses are the same. Same addresses means that remapping is disabled, and thus if we change the CPU address, we need also to change the PCI address so that the remapping is still disabled afterwards. Consider an example: The ranges entries contain: PCI address CPU address 7000 EA00 E900 E900 EB00 EB00 By default CPU PCIe window is at:E800 - F000 Consider the case when TF-A moves it to: F200 - FA00 Until now the function would take the PCI address of the first entry: 7000, and the new base, F200, to compute the fix offset: F200 - 7000 = 8200, and then add 820 to all addresses, resulting in PCI address CPU address F200 6C00 6B00 6B00 6D00 6D00 which is complete nonsense - none of the CPU addresses is in the requested window. Now it will take the lowest CPU address, which is in second row, E900, and compute the fix offset F200 - E900 = 0900, and then add it to all CPU addresses and those PCI addresses which equal to their corresponding CPU addresses, resulting in PCI address CPU address 7000 F300 F200 F200 F400 F400 where all of the CPU addresses are in the needed window. Fixes: 4a82fca8e330 ("arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function") Signed-off-by: Pali Rohár Signed-off-by: Marek Behún Applied to u-boot-marvell/master Thanks, Stefan --- arch/arm/mach-mvebu/armada3700/cpu.c | 81 +++- 1 file changed, 55 insertions(+), 26 deletions(-) diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index 23492f49da..52b5109b73 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -316,8 +316,8 @@ static int fdt_setprop_inplace_u32_partial(void *blob, int node, int a3700_fdt_fix_pcie_regions(void *blob) { - int acells, pacells, scells; - u32 base, fix_offset; + u32 base, lowest_cpu_addr, fix_offset; + int pci_cells, cpu_cells, size_cells; const u32 *ranges; int node, pnode; int ret, i, len; @@ -331,51 +331,80 @@ int a3700_fdt_fix_pcie_regions(void *blob) return node; ranges = fdt_getprop(blob, node, "ranges", ); - if (!ranges || len % sizeof(u32)) - return -ENOENT; + if (!ranges || !len || len % sizeof(u32)) + return -EINVAL; /* * The "ranges" property is an array of -* {} +* {} +* where number of PCI address cells and size cells is stored in the +* "#address-cells" and "#size-cells" properties of the same node +* containing the "ranges" property and number of CPU address cells +* is stored in the parent's "#address-cells" property. * -* All 3 elements can span a diffent number of cells. Fetch their sizes. +* All 3 elements can span a diffent number of cells. Fetch them. */ pnode = fdt_parent_offset(blob, node); - acells = fdt_address_cells(blob, node); - pacells = fdt_address_cells(blob, pnode); - scells = fdt_size_cells(blob, node); + pci_cells = fdt_address_cells(blob, node); + cpu_cells = fdt_address_cells(blob, pnode); + size_cells = fdt_size_cells(blob, node); - /* Child PCI addresses always use 3 cells */ - if (acells != 3) - return -ENOENT; + /* PCI addresses always use 3 cells */ + if (pci_cells != 3) + return -EINVAL; + + /* CPU addresses on Armada 37xx always use 2 cells */ + if (cpu_cells != 2) + return -EINVAL; + + for (i = 0; i < len / sizeof(u32); +i += pci_cells + cpu_cells + size_cells) { +
Re: [PATCH u-boot-marvell] arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function again
On 2/23/22 13:52, Marek Behún wrote: From: Pali Rohár The a3700_fdt_fix_pcie_regions() function still computes nonsense. It computes the fixup offset from the PCI address taken from the first row of the "ranges" array, which means that: - PCI address must equal CPU address (otherwise the computed fix offset will be wrong), - the first row must contain the lowest address. This is the case for the default device-tree, which is why we didn't notice it. It also adds the fixup offset to all PCI and CPU addresses, which is wrong. Instead: 1) The fixup offset must be computed from the CPU address, not PCI address. 2) The fixup offset must be computed from the row containing the lowest CPU address, which is not necessarily contained in the first row. 3) The PCI address - the address to which the PCIe controller remaps the address space as seen from the point of view of the PCIe device - must be fixed by the fix offset in the same way as the CPU address only in the special case when the CPU adn PCI addresses are the same. Same addresses means that remapping is disabled, and thus if we change the CPU address, we need also to change the PCI address so that the remapping is still disabled afterwards. Consider an example: The ranges entries contain: PCI address CPU address 7000 EA00 E900 E900 EB00 EB00 By default CPU PCIe window is at:E800 - F000 Consider the case when TF-A moves it to: F200 - FA00 Until now the function would take the PCI address of the first entry: 7000, and the new base, F200, to compute the fix offset: F200 - 7000 = 8200, and then add 820 to all addresses, resulting in PCI address CPU address F200 6C00 6B00 6B00 6D00 6D00 which is complete nonsense - none of the CPU addresses is in the requested window. Now it will take the lowest CPU address, which is in second row, E900, and compute the fix offset F200 - E900 = 0900, and then add it to all CPU addresses and those PCI addresses which equal to their corresponding CPU addresses, resulting in PCI address CPU address 7000 F300 F200 F200 F400 F400 where all of the CPU addresses are in the needed window. Fixes: 4a82fca8e330 ("arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function") Signed-off-by: Pali Rohár Signed-off-by: Marek Behún Reviewed-by: Stefan Roese Thanks, Stefan --- arch/arm/mach-mvebu/armada3700/cpu.c | 81 +++- 1 file changed, 55 insertions(+), 26 deletions(-) diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index 23492f49da..52b5109b73 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -316,8 +316,8 @@ static int fdt_setprop_inplace_u32_partial(void *blob, int node, int a3700_fdt_fix_pcie_regions(void *blob) { - int acells, pacells, scells; - u32 base, fix_offset; + u32 base, lowest_cpu_addr, fix_offset; + int pci_cells, cpu_cells, size_cells; const u32 *ranges; int node, pnode; int ret, i, len; @@ -331,51 +331,80 @@ int a3700_fdt_fix_pcie_regions(void *blob) return node; ranges = fdt_getprop(blob, node, "ranges", ); - if (!ranges || len % sizeof(u32)) - return -ENOENT; + if (!ranges || !len || len % sizeof(u32)) + return -EINVAL; /* * The "ranges" property is an array of -* {} +* {} +* where number of PCI address cells and size cells is stored in the +* "#address-cells" and "#size-cells" properties of the same node +* containing the "ranges" property and number of CPU address cells +* is stored in the parent's "#address-cells" property. * -* All 3 elements can span a diffent number of cells. Fetch their sizes. +* All 3 elements can span a diffent number of cells. Fetch them. */ pnode = fdt_parent_offset(blob, node); - acells = fdt_address_cells(blob, node); - pacells = fdt_address_cells(blob, pnode); - scells = fdt_size_cells(blob, node); + pci_cells = fdt_address_cells(blob, node); + cpu_cells = fdt_address_cells(blob, pnode); + size_cells = fdt_size_cells(blob, node); - /* Child PCI addresses always use 3 cells */ - if (acells != 3) - return -ENOENT; + /* PCI addresses always use 3 cells */ + if (pci_cells != 3) + return -EINVAL; + + /* CPU addresses on Armada 37xx always use 2 cells */ + if (cpu_cells != 2) + return -EINVAL; + + for (i = 0; i < len / sizeof(u32); +i += pci_cells + cpu_cells + size_cells) { +
[PATCH u-boot-marvell] arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function again
From: Pali Rohár The a3700_fdt_fix_pcie_regions() function still computes nonsense. It computes the fixup offset from the PCI address taken from the first row of the "ranges" array, which means that: - PCI address must equal CPU address (otherwise the computed fix offset will be wrong), - the first row must contain the lowest address. This is the case for the default device-tree, which is why we didn't notice it. It also adds the fixup offset to all PCI and CPU addresses, which is wrong. Instead: 1) The fixup offset must be computed from the CPU address, not PCI address. 2) The fixup offset must be computed from the row containing the lowest CPU address, which is not necessarily contained in the first row. 3) The PCI address - the address to which the PCIe controller remaps the address space as seen from the point of view of the PCIe device - must be fixed by the fix offset in the same way as the CPU address only in the special case when the CPU adn PCI addresses are the same. Same addresses means that remapping is disabled, and thus if we change the CPU address, we need also to change the PCI address so that the remapping is still disabled afterwards. Consider an example: The ranges entries contain: PCI address CPU address 7000 EA00 E900 E900 EB00 EB00 By default CPU PCIe window is at:E800 - F000 Consider the case when TF-A moves it to: F200 - FA00 Until now the function would take the PCI address of the first entry: 7000, and the new base, F200, to compute the fix offset: F200 - 7000 = 8200, and then add 820 to all addresses, resulting in PCI address CPU address F200 6C00 6B00 6B00 6D00 6D00 which is complete nonsense - none of the CPU addresses is in the requested window. Now it will take the lowest CPU address, which is in second row, E900, and compute the fix offset F200 - E900 = 0900, and then add it to all CPU addresses and those PCI addresses which equal to their corresponding CPU addresses, resulting in PCI address CPU address 7000 F300 F200 F200 F400 F400 where all of the CPU addresses are in the needed window. Fixes: 4a82fca8e330 ("arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function") Signed-off-by: Pali Rohár Signed-off-by: Marek Behún --- arch/arm/mach-mvebu/armada3700/cpu.c | 81 +++- 1 file changed, 55 insertions(+), 26 deletions(-) diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index 23492f49da..52b5109b73 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -316,8 +316,8 @@ static int fdt_setprop_inplace_u32_partial(void *blob, int node, int a3700_fdt_fix_pcie_regions(void *blob) { - int acells, pacells, scells; - u32 base, fix_offset; + u32 base, lowest_cpu_addr, fix_offset; + int pci_cells, cpu_cells, size_cells; const u32 *ranges; int node, pnode; int ret, i, len; @@ -331,51 +331,80 @@ int a3700_fdt_fix_pcie_regions(void *blob) return node; ranges = fdt_getprop(blob, node, "ranges", ); - if (!ranges || len % sizeof(u32)) - return -ENOENT; + if (!ranges || !len || len % sizeof(u32)) + return -EINVAL; /* * The "ranges" property is an array of -* {} +* {} +* where number of PCI address cells and size cells is stored in the +* "#address-cells" and "#size-cells" properties of the same node +* containing the "ranges" property and number of CPU address cells +* is stored in the parent's "#address-cells" property. * -* All 3 elements can span a diffent number of cells. Fetch their sizes. +* All 3 elements can span a diffent number of cells. Fetch them. */ pnode = fdt_parent_offset(blob, node); - acells = fdt_address_cells(blob, node); - pacells = fdt_address_cells(blob, pnode); - scells = fdt_size_cells(blob, node); + pci_cells = fdt_address_cells(blob, node); + cpu_cells = fdt_address_cells(blob, pnode); + size_cells = fdt_size_cells(blob, node); - /* Child PCI addresses always use 3 cells */ - if (acells != 3) - return -ENOENT; + /* PCI addresses always use 3 cells */ + if (pci_cells != 3) + return -EINVAL; + + /* CPU addresses on Armada 37xx always use 2 cells */ + if (cpu_cells != 2) + return -EINVAL; + + for (i = 0; i < len / sizeof(u32); +i += pci_cells + cpu_cells + size_cells) { + /* +* Parent CPU addresses on Armada 37xx are always 32-bit, so +* check that