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