* [PATCH] pci: Sort resources before assignment
@ 2022-08-10 6:56 Leo Yan
2022-08-10 14:24 ` Simon Glass
0 siblings, 1 reply; 2+ messages in thread
From: Leo Yan @ 2022-08-10 6:56 UTC (permalink / raw)
To: Stefan Roese, Pali Rohár, Maciej W. Rozycki, Bin Meng,
Vladimir Oltean, u-boot, Matsumoto Misako
Cc: Leo Yan
A PCI device can request resource for multiple memory regions, e.g. a
PCI device tries to allocate prefetchable memory for two regions, one
region size is 0x1000_0000 and another is 0x1_0000_0000. And the PCIe
controller provides prefetchable memory window size is 0x1_8000_0000,
thus in theory the PCIe controller can meet the memory requirement for
the PCI device:
0x1_8000_0000 > 0x1_1000_0000 (0x1000_0000 + 0x1_0000_0000)
When allocate the memory region, pciauto_region_allocate() applies the
alignment for the start address, we can see the memory regions are
allocated as:
=> pci bar 1.0.0
ID Base Size Width Type
----------------------------------------------------------
0 0x0000000088000000 0x0000000004000000 32 MEM
1 0x000000008c000000 0x0000000000100000 32 MEM
2 0x0000001000000000 0x0000000010000000 64 MEM Prefetchable
3 0x0000001100000000 0x0000000100000000 64 MEM Prefetchable
The problem is for the last memory region, we can see its start address
is 0x11_0000_0000 and the size is 0x1_0000_0000, therefore, these two
memory regions occupy memory size is:
0x11_0000_0000 + 0x1_0000_0000 - 0x10_0000_0000 = 0x2_0000_0000
The allocated memory region (0x2_0000_0000) is out of the range, because
the maximum space provided by PCI controller is only 0x1_8000_0000.
To fix this issue, this patch sorts resources with descending, this can
give the priority for big chunk memory region, the big memory region is
allocated ahead before a small region, so that its start address does
not necessarily introduce big hole caused by the alignment, which is
finished by function pdev_sort_resources().
And we use another function pdev_assign_resources() to set BARs based on
the sorted list.
As result, we can see the updated memory regions are altered as below;
the end address is: 0x11_0000_0000 + 0x1000_0000, which falls into the
permitted memory window.
=> pci bar 1.0.0
ID Base Size Width Type
----------------------------------------------------------
0 0x0000000088000000 0x0000000004000000 32 MEM
1 0x000000008c000000 0x0000000000100000 32 MEM
2 0x0000001100000000 0x0000000010000000 64 MEM Prefetchable
3 0x0000001000000000 0x0000000100000000 64 MEM Prefetchable
Reported-by: Matsumoto Misako <matsumoto.misako@socionext.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
drivers/pci/pci_auto.c | 104 +++++++++++++++++++++++++++++++----------
1 file changed, 79 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
index c7968926a1..69c801fc62 100644
--- a/drivers/pci/pci_auto.c
+++ b/drivers/pci/pci_auto.c
@@ -14,6 +14,8 @@
#include <log.h>
#include <pci.h>
#include <time.h>
+#include <linux/compiler.h>
+#include <linux/list.h>
#include "pci_internal.h"
/* the user can define CONFIG_SYS_PCI_CACHE_LINE_SIZE to avoid problems */
@@ -21,6 +23,69 @@
#define CONFIG_SYS_PCI_CACHE_LINE_SIZE 8
#endif
+struct pci_dev_resource {
+ struct list_head list;
+ int bar;
+ pci_size_t bar_size;
+ int found_mem64;
+ struct pci_region *bar_res;
+};
+
+/* Sort resources by bar size */
+static void pdev_sort_resources(struct pci_dev_resource *new,
+ struct list_head *head)
+{
+ struct pci_dev_resource *dev_res;
+ struct list_head *n;
+
+ /* Fallback is smallest one or list is empty */
+ n = head;
+ list_for_each_entry(dev_res, head, list) {
+ if (new->bar_size > dev_res->bar_size) {
+ n = &dev_res->list;
+ break;
+ }
+ }
+
+ /* Insert it just before n */
+ list_add_tail(&new->list, n);
+}
+
+static void pdev_assign_resources(struct udevice *dev, struct list_head *head)
+{
+ struct pci_dev_resource *dev_res;
+ int bar;
+ pci_addr_t bar_value;
+ int ret = 0;
+
+ list_for_each_entry(dev_res, head, list) {
+ ret = pciauto_region_allocate(dev_res->bar_res, dev_res->bar_size,
+ &bar_value, dev_res->found_mem64);
+ if (ret)
+ printf("PCI: Failed autoconfig bar %x\n", dev_res->bar);
+
+ bar = dev_res->bar;
+ if (!ret) {
+ /* Write it out and update our limit */
+ dm_pci_write_config32(dev, bar, (u32)bar_value);
+
+ if (dev_res->found_mem64) {
+ bar += 4;
+ if (IS_ENABLED(CONFIG_SYS_PCI_64BIT))
+ dm_pci_write_config32(dev, bar,
+ (u32)(bar_value >> 32));
+ else
+ /*
+ * If we are a 64-bit decoder then increment to
+ * the upper 32 bits of the bar and force it to
+ * locate in the lower 4GB of memory.
+ */
+ dm_pci_write_config32(dev, bar, 0x00000000);
+ }
+ }
+ }
+}
+
static void dm_pciauto_setup_device(struct udevice *dev,
struct pci_region *mem,
struct pci_region *prefetch,
@@ -37,6 +102,10 @@ static void dm_pciauto_setup_device(struct udevice *dev,
struct pci_region *bar_res = NULL;
int found_mem64 = 0;
u16 class;
+ LIST_HEAD(head);
+ struct pci_dev_resource pdev_resource[6];
+
+ memset(pdev_resource, 0x0, sizeof(pdev_resource));
dm_pci_read_config16(dev, PCI_COMMAND, &cmdstat);
cmdstat = (cmdstat & ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) |
@@ -64,8 +133,6 @@ static void dm_pciauto_setup_device(struct udevice *dev,
for (bar = PCI_BASE_ADDRESS_0;
bar < PCI_BASE_ADDRESS_0 + (bars_num * 4); bar += 4) {
- int ret = 0;
-
/* Tickle the BAR and get the response */
dm_pci_write_config32(dev, bar, 0xffffffff);
dm_pci_read_config32(dev, bar, &bar_response);
@@ -118,30 +185,15 @@ static void dm_pciauto_setup_device(struct udevice *dev,
(unsigned long long)bar_size);
}
- ret = pciauto_region_allocate(bar_res, bar_size,
- &bar_value, found_mem64);
- if (ret)
- printf("PCI: Failed autoconfig bar %x\n", bar);
-
- if (!ret) {
- /* Write it out and update our limit */
- dm_pci_write_config32(dev, bar, (u32)bar_value);
+ INIT_LIST_HEAD(&pdev_resource[bar_nr].list);
+ pdev_resource[bar_nr].bar = bar;
+ pdev_resource[bar_nr].bar_size = bar_size;
+ pdev_resource[bar_nr].bar_res = bar_res;
+ pdev_resource[bar_nr].found_mem64 = found_mem64;
+ pdev_sort_resources(&pdev_resource[bar_nr], &head);
- if (found_mem64) {
- bar += 4;
-#ifdef CONFIG_SYS_PCI_64BIT
- dm_pci_write_config32(dev, bar,
- (u32)(bar_value >> 32));
-#else
- /*
- * If we are a 64-bit decoder then increment to
- * the upper 32 bits of the bar and force it to
- * locate in the lower 4GB of memory.
- */
- dm_pci_write_config32(dev, bar, 0x00000000);
-#endif
- }
- }
+ if (found_mem64)
+ bar += 4;
cmdstat |= (bar_response & PCI_BASE_ADDRESS_SPACE) ?
PCI_COMMAND_IO : PCI_COMMAND_MEMORY;
@@ -151,6 +203,8 @@ static void dm_pciauto_setup_device(struct udevice *dev,
bar_nr++;
}
+ pdev_assign_resources(dev, &head);
+
/* Configure the expansion ROM address */
if (header_type == PCI_HEADER_TYPE_NORMAL ||
header_type == PCI_HEADER_TYPE_BRIDGE) {
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] pci: Sort resources before assignment
2022-08-10 6:56 [PATCH] pci: Sort resources before assignment Leo Yan
@ 2022-08-10 14:24 ` Simon Glass
0 siblings, 0 replies; 2+ messages in thread
From: Simon Glass @ 2022-08-10 14:24 UTC (permalink / raw)
To: Leo Yan
Cc: Stefan Roese, Pali Rohár, Maciej W. Rozycki, Bin Meng,
Vladimir Oltean, U-Boot Mailing List, Matsumoto Misako
Hi Leo,
On Wed, 10 Aug 2022 at 06:58, Leo Yan <leo.yan@linaro.org> wrote:
>
> A PCI device can request resource for multiple memory regions, e.g. a
> PCI device tries to allocate prefetchable memory for two regions, one
> region size is 0x1000_0000 and another is 0x1_0000_0000. And the PCIe
> controller provides prefetchable memory window size is 0x1_8000_0000,
> thus in theory the PCIe controller can meet the memory requirement for
> the PCI device:
>
> 0x1_8000_0000 > 0x1_1000_0000 (0x1000_0000 + 0x1_0000_0000)
>
> When allocate the memory region, pciauto_region_allocate() applies the
> alignment for the start address, we can see the memory regions are
> allocated as:
>
> => pci bar 1.0.0
> ID Base Size Width Type
> ----------------------------------------------------------
> 0 0x0000000088000000 0x0000000004000000 32 MEM
> 1 0x000000008c000000 0x0000000000100000 32 MEM
> 2 0x0000001000000000 0x0000000010000000 64 MEM Prefetchable
> 3 0x0000001100000000 0x0000000100000000 64 MEM Prefetchable
>
> The problem is for the last memory region, we can see its start address
> is 0x11_0000_0000 and the size is 0x1_0000_0000, therefore, these two
> memory regions occupy memory size is:
>
> 0x11_0000_0000 + 0x1_0000_0000 - 0x10_0000_0000 = 0x2_0000_0000
>
> The allocated memory region (0x2_0000_0000) is out of the range, because
> the maximum space provided by PCI controller is only 0x1_8000_0000.
>
> To fix this issue, this patch sorts resources with descending, this can
> give the priority for big chunk memory region, the big memory region is
> allocated ahead before a small region, so that its start address does
> not necessarily introduce big hole caused by the alignment, which is
> finished by function pdev_sort_resources().
>
> And we use another function pdev_assign_resources() to set BARs based on
> the sorted list.
>
> As result, we can see the updated memory regions are altered as below;
> the end address is: 0x11_0000_0000 + 0x1000_0000, which falls into the
> permitted memory window.
>
> => pci bar 1.0.0
> ID Base Size Width Type
> ----------------------------------------------------------
> 0 0x0000000088000000 0x0000000004000000 32 MEM
> 1 0x000000008c000000 0x0000000000100000 32 MEM
> 2 0x0000001100000000 0x0000000010000000 64 MEM Prefetchable
> 3 0x0000001000000000 0x0000000100000000 64 MEM Prefetchable
>
> Reported-by: Matsumoto Misako <matsumoto.misako@socionext.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> drivers/pci/pci_auto.c | 104 +++++++++++++++++++++++++++++++----------
> 1 file changed, 79 insertions(+), 25 deletions(-)
Pease add a test for this to test/dm/pci.c
>
> diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
> index c7968926a1..69c801fc62 100644
> --- a/drivers/pci/pci_auto.c
> +++ b/drivers/pci/pci_auto.c
> @@ -14,6 +14,8 @@
> #include <log.h>
> #include <pci.h>
> #include <time.h>
> +#include <linux/compiler.h>
> +#include <linux/list.h>
> #include "pci_internal.h"
>
> /* the user can define CONFIG_SYS_PCI_CACHE_LINE_SIZE to avoid problems */
> @@ -21,6 +23,69 @@
> #define CONFIG_SYS_PCI_CACHE_LINE_SIZE 8
> #endif
>
> +struct pci_dev_resource {
> + struct list_head list;
> + int bar;
> + pci_size_t bar_size;
> + int found_mem64;
> + struct pci_region *bar_res;
> +};
Please add full comments. Should this be named pci_dev_node?
> +
> +/* Sort resources by bar size */
> +static void pdev_sort_resources(struct pci_dev_resource *new,
> + struct list_head *head)
> +{
> + struct pci_dev_resource *dev_res;
> + struct list_head *n;
> +
> + /* Fallback is smallest one or list is empty */
> + n = head;
> + list_for_each_entry(dev_res, head, list) {
> + if (new->bar_size > dev_res->bar_size) {
> + n = &dev_res->list;
> + break;
> + }
> + }
> +
> + /* Insert it just before n */
> + list_add_tail(&new->list, n);
> +}
> +
> +static void pdev_assign_resources(struct udevice *dev, struct list_head *head)
> +{
> + struct pci_dev_resource *dev_res;
> + int bar;
> + pci_addr_t bar_value;
> + int ret = 0;
> +
> + list_for_each_entry(dev_res, head, list) {
> + ret = pciauto_region_allocate(dev_res->bar_res, dev_res->bar_size,
> + &bar_value, dev_res->found_mem64);
> + if (ret)
> + printf("PCI: Failed autoconfig bar %x\n", dev_res->bar);
> +
> + bar = dev_res->bar;
> + if (!ret) {
> + /* Write it out and update our limit */
> + dm_pci_write_config32(dev, bar, (u32)bar_value);
> +
> + if (dev_res->found_mem64) {
> + bar += 4;
> + if (IS_ENABLED(CONFIG_SYS_PCI_64BIT))
> + dm_pci_write_config32(dev, bar,
> + (u32)(bar_value >> 32));
> + else
> + /*
> + * If we are a 64-bit decoder then increment to
> + * the upper 32 bits of the bar and force it to
> + * locate in the lower 4GB of memory.
> + */
> + dm_pci_write_config32(dev, bar, 0x00000000);
> + }
> + }
> + }
> +}
> +
> static void dm_pciauto_setup_device(struct udevice *dev,
> struct pci_region *mem,
> struct pci_region *prefetch,
> @@ -37,6 +102,10 @@ static void dm_pciauto_setup_device(struct udevice *dev,
> struct pci_region *bar_res = NULL;
> int found_mem64 = 0;
> u16 class;
> + LIST_HEAD(head);
> + struct pci_dev_resource pdev_resource[6];
> +
> + memset(pdev_resource, 0x0, sizeof(pdev_resource));
>
> dm_pci_read_config16(dev, PCI_COMMAND, &cmdstat);
> cmdstat = (cmdstat & ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) |
> @@ -64,8 +133,6 @@ static void dm_pciauto_setup_device(struct udevice *dev,
>
> for (bar = PCI_BASE_ADDRESS_0;
> bar < PCI_BASE_ADDRESS_0 + (bars_num * 4); bar += 4) {
> - int ret = 0;
> -
> /* Tickle the BAR and get the response */
> dm_pci_write_config32(dev, bar, 0xffffffff);
> dm_pci_read_config32(dev, bar, &bar_response);
> @@ -118,30 +185,15 @@ static void dm_pciauto_setup_device(struct udevice *dev,
> (unsigned long long)bar_size);
> }
>
> - ret = pciauto_region_allocate(bar_res, bar_size,
> - &bar_value, found_mem64);
> - if (ret)
> - printf("PCI: Failed autoconfig bar %x\n", bar);
> -
> - if (!ret) {
> - /* Write it out and update our limit */
> - dm_pci_write_config32(dev, bar, (u32)bar_value);
> + INIT_LIST_HEAD(&pdev_resource[bar_nr].list);
> + pdev_resource[bar_nr].bar = bar;
> + pdev_resource[bar_nr].bar_size = bar_size;
> + pdev_resource[bar_nr].bar_res = bar_res;
> + pdev_resource[bar_nr].found_mem64 = found_mem64;
> + pdev_sort_resources(&pdev_resource[bar_nr], &head);
>
> - if (found_mem64) {
> - bar += 4;
> -#ifdef CONFIG_SYS_PCI_64BIT
> - dm_pci_write_config32(dev, bar,
> - (u32)(bar_value >> 32));
> -#else
> - /*
> - * If we are a 64-bit decoder then increment to
> - * the upper 32 bits of the bar and force it to
> - * locate in the lower 4GB of memory.
> - */
> - dm_pci_write_config32(dev, bar, 0x00000000);
> -#endif
> - }
> - }
> + if (found_mem64)
> + bar += 4;
>
> cmdstat |= (bar_response & PCI_BASE_ADDRESS_SPACE) ?
> PCI_COMMAND_IO : PCI_COMMAND_MEMORY;
> @@ -151,6 +203,8 @@ static void dm_pciauto_setup_device(struct udevice *dev,
> bar_nr++;
> }
>
> + pdev_assign_resources(dev, &head);
> +
> /* Configure the expansion ROM address */
> if (header_type == PCI_HEADER_TYPE_NORMAL ||
> header_type == PCI_HEADER_TYPE_BRIDGE) {
> --
> 2.25.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-08-10 14:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-10 6:56 [PATCH] pci: Sort resources before assignment Leo Yan
2022-08-10 14:24 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.