diff mbox series

pci: Sort resources before assignment

Message ID 20220810065633.156864-1-leo.yan@linaro.org
State New
Headers show
Series pci: Sort resources before assignment | expand

Commit Message

Leo Yan Aug. 10, 2022, 6:56 a.m. UTC
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(-)

Comments

Simon Glass Aug. 10, 2022, 2:24 p.m. UTC | #1
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
diff mbox series

Patch

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) {