| Message ID | 20220810065633.156864-1-leo.yan@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pci: Sort resources before assignment | expand |
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 --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) {
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(-)