diff mbox

pci: restrict 64-bit pci device to assign resource from behind of max_pfn

Message ID 1444906004-2083-1-git-send-email-wenlin.kang@windriver.com
State Changes Requested
Headers show

Commit Message

Wenlin Kang Oct. 15, 2015, 10:46 a.m. UTC 
This patch restricts 64-bit pci device to assign resource from behind of
max_pfn when kernel config enables CONFIG_64BIT.

On some system that pci device requires assignment of large resource(eg,
1GB or larger), sometimes,the assignment of some devices may fail under
the current way.

e.g. the follow case is that.

...
[    0.000000] e820: [mem 0x90000000-0xfed1bfff] available for PCI devices
[    0.000000] setup_percpu: NR_CPUS:128 nr_cpumask_bits:128 nr_cpu_ids:72 nr_node_ids:2
...
[    6.564750] pci 0000:00:1c.7:   bridge window [mem 0x9f000000-0xa08fffff]
[    6.587130] pci 0000:84:06.0: BAR 14: can't assign mem (size 0x60000000)
[    6.609158] pci 0000:84:00.0: BAR 14: can't assign mem (size 0x100000)
...

On this case, although the kernel has [0x90000000-0xfed1bfff] [1.73GB]
size space is available for PCI devices, assignment of some devices fail
yet, the cause is the resource isn't yet enough for devices.

After apply this patch, this problem get resolved, the patch makes kernel
assign resource from behind of max_pfn for 64-bit pci devices when kernel
is 64BIT, however, previous way always assigns resource below 4GB no
matter it is 32 or 64-bit devices, so this patch extends this window of
pci resource on 64-bit system that both 32 and 64-bit pci device exist or
only 64-bit device exists, makes it more likely that we can assign
resource to more or all devices.

Signed-off-by: wlkang <wlkang2008@126.com>
Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
---
 drivers/pci/setup-res.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

kernel test robot Oct. 15, 2015, 11:32 a.m. UTC | #1 
Hi Wenlin,

[auto build test ERROR on pci/next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Wenlin-Kang/pci-restrict-64-bit-pci-device-to-assign-resource-from-behind-of-max_pfn/20151015-184913
config: mips-fuloong2e_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All errors (new ones prefixed by >>):

   drivers/pci/setup-res.c: In function '__pci_assign_resource':
>> drivers/pci/setup-res.c:224:6: error: 'max_pfn' undeclared (first use in this function)
        (max_pfn + 1) << PAGE_SHIFT : PCIBIOS_MIN_MEM;
         ^
   drivers/pci/setup-res.c:224:6: note: each undeclared identifier is reported only once for each function it appears in

vim +/max_pfn +224 drivers/pci/setup-res.c

   218		 * For 64-bit pci device, assign resource start from the next page
   219		 * boundary above the maximum physical page address
   220		 */
   221		resource_size_t min_iomem;
   222	
   223		min_iomem = (res->flags & IORESOURCE_MEM_64) ?
 > 224					(max_pfn + 1) << PAGE_SHIFT : PCIBIOS_MIN_MEM;
   225		min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : min_iomem;
   226	#else
   227		min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Bjorn Helgaas Oct. 15, 2015, 2:05 p.m. UTC | #2 
Hi Wenlin,

On Thu, Oct 15, 2015 at 06:46:44PM +0800, Wenlin Kang wrote:
> This patch restricts 64-bit pci device to assign resource from behind of
> max_pfn when kernel config enables CONFIG_64BIT.
> 
> On some system that pci device requires assignment of large resource(eg,
> 1GB or larger), sometimes,the assignment of some devices may fail under
> the current way.
> 
> e.g. the follow case is that.
> 
> ...
> [    0.000000] e820: [mem 0x90000000-0xfed1bfff] available for PCI devices
> [    0.000000] setup_percpu: NR_CPUS:128 nr_cpumask_bits:128 nr_cpu_ids:72 nr_node_ids:2
> ...
> [    6.564750] pci 0000:00:1c.7:   bridge window [mem 0x9f000000-0xa08fffff]
> [    6.587130] pci 0000:84:06.0: BAR 14: can't assign mem (size 0x60000000)
> [    6.609158] pci 0000:84:00.0: BAR 14: can't assign mem (size 0x100000)
> ...
> 
> On this case, although the kernel has [0x90000000-0xfed1bfff] [1.73GB]
> size space is available for PCI devices, assignment of some devices fail
> yet, the cause is the resource isn't yet enough for devices.

We do PCI resource allocation inside the host bridge windows, which
you didn't include here.

The [mem 0x90000000-0xfed1bfff] range is derived from the E820 table
and is sort of a theoretical maximum -- it's the range that isn't
being used by anything else.  The "available for PCI devices" text is
not quite accurate -- this range is potentially available for *any*
device that has programmable addresses.

Long ago, we did use that range directly for PCI allocation, but now
that we have machines with multiple host bridges, it's not sufficient:
the range may be carved up between several bridges.  We have to look
at the host bridge windows to see what parts of the range are
available for devices below each host bridge.

That's a long-winded way of saying that we can't simply look at
max_pfn as you do in this patch.  We need to look at the host bridge
windows themselves.  On x86, these come from ACPI _CRS methods, and
they probably aren't big enough to accommodate this device.  If that's
the case, we can't do much about it.  If the BIOS tells us "this is
the only range routed to the PCI bus," we have to assume it's telling
us the truth.  The BIOS knows how the bridges are configured, and we
don't.

> After apply this patch, this problem get resolved, the patch makes kernel
> assign resource from behind of max_pfn for 64-bit pci devices when kernel
> is 64BIT, however, previous way always assigns resource below 4GB no
> matter it is 32 or 64-bit devices, so this patch extends this window of
> pci resource on 64-bit system that both 32 and 64-bit pci device exist or
> only 64-bit device exists, makes it more likely that we can assign
> resource to more or all devices.
> 
> Signed-off-by: wlkang <wlkang2008@126.com>
> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>

I assume both of these Signed-off-by lines refer to you.  If that's
the case, you should at least use the same "real name" (the "wlkang"
and "Wenlin Kang" parts).  I think it makes the most sense to use a
single address, but if you need both, I would put the second in a
"CC:" line or something.  The Signed-off-by is more of an ownership
thing where it doesn't really make sense to have two names for a
single person.

> ---
>  drivers/pci/setup-res.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 232f925..dafd667 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -206,7 +206,19 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>  	resource_size_t min;
>  	int ret;
>  
> +#ifdef CONFIG_64BIT
> +	/*
> +	 * For 64-bit pci device, assign resource start from the next page
> +	 * boundary above the maximum physical page address
> +	 */
> +	resource_size_t min_iomem;
> +
> +	min_iomem = (res->flags & IORESOURCE_MEM_64) ?
> +				(max_pfn + 1) << PAGE_SHIFT : PCIBIOS_MIN_MEM;
> +	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : min_iomem;
> +#else
>  	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
> +#endif
>  
>  	/*
>  	 * First, try exact prefetching match.  Even if a 64-bit
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wenlin Kang Oct. 16, 2015, 6:28 a.m. UTC | #3 
On 2015年10月15日 19:32, kbuild test robot wrote:
> Hi Wenlin,
>
> [auto build test ERROR on pci/next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
>
> url:    https://github.com/0day-ci/linux/commits/Wenlin-Kang/pci-restrict-64-bit-pci-device-to-assign-resource-from-behind-of-max_pfn/20151015-184913
> config: mips-fuloong2e_defconfig (attached as .config)
> reproduce:
>          wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # save the attached .config to linux build tree
>          make.cross ARCH=mips
>
> All errors (new ones prefixed by >>):
>
>     drivers/pci/setup-res.c: In function '__pci_assign_resource':
>>> drivers/pci/setup-res.c:224:6: error: 'max_pfn' undeclared (first use in this function)
>          (max_pfn + 1) << PAGE_SHIFT : PCIBIOS_MIN_MEM;
>           ^
>     drivers/pci/setup-res.c:224:6: note: each undeclared identifier is reported only once for each function it appears in
>
> vim +/max_pfn +224 drivers/pci/setup-res.c
>
>     218		 * For 64-bit pci device, assign resource start from the next page
>     219		 * boundary above the maximum physical page address
>     220		 */
>     221		resource_size_t min_iomem;
>     222	
>     223		min_iomem = (res->flags & IORESOURCE_MEM_64) ?
>   > 224					(max_pfn + 1) << PAGE_SHIFT : PCIBIOS_MIN_MEM;
>     225		min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : min_iomem;
>     226	#else
>     227		min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation


Hi

Thanks you point out this, due to I built it on x86 previously , so 
don't see the error.

After added  "#include <linux/bootmem.h>" in drivers/pci/setup-res.c, it 
will be ok.
Wenlin Kang Oct. 16, 2015, 8:01 a.m. UTC | #4 
On 2015年10月15日 22:05, Bjorn Helgaas wrote:
> Hi Wenlin,
>
> On Thu, Oct 15, 2015 at 06:46:44PM +0800, Wenlin Kang wrote:
>> This patch restricts 64-bit pci device to assign resource from behind of
>> max_pfn when kernel config enables CONFIG_64BIT.
>>
>> On some system that pci device requires assignment of large resource(eg,
>> 1GB or larger), sometimes,the assignment of some devices may fail under
>> the current way.
>>
>> e.g. the follow case is that.
>>
>> ...
>> [    0.000000] e820: [mem 0x90000000-0xfed1bfff] available for PCI devices
>> [    0.000000] setup_percpu: NR_CPUS:128 nr_cpumask_bits:128 nr_cpu_ids:72 nr_node_ids:2
>> ...
>> [    6.564750] pci 0000:00:1c.7:   bridge window [mem 0x9f000000-0xa08fffff]
>> [    6.587130] pci 0000:84:06.0: BAR 14: can't assign mem (size 0x60000000)
>> [    6.609158] pci 0000:84:00.0: BAR 14: can't assign mem (size 0x100000)
>> ...
>>
>> On this case, although the kernel has [0x90000000-0xfed1bfff] [1.73GB]
>> size space is available for PCI devices, assignment of some devices fail
>> yet, the cause is the resource isn't yet enough for devices.
> We do PCI resource allocation inside the host bridge windows, which
> you didn't include here.
>
> The [mem 0x90000000-0xfed1bfff] range is derived from the E820 table
> and is sort of a theoretical maximum -- it's the range that isn't
> being used by anything else.  The "available for PCI devices" text is
> not quite accurate -- this range is potentially available for *any*
> device that has programmable addresses.
>
> Long ago, we did use that range directly for PCI allocation, but now
> that we have machines with multiple host bridges, it's not sufficient:
> the range may be carved up between several bridges.  We have to look
> at the host bridge windows to see what parts of the range are
> available for devices below each host bridge.
>
> That's a long-winded way of saying that we can't simply look at
> max_pfn as you do in this patch.  We need to look at the host bridge
> windows themselves.  On x86, these come from ACPI _CRS methods, and
> they probably aren't big enough to accommodate this device.  If that's
> the case, we can't do much about it.  If the BIOS tells us "this is
> the only range routed to the PCI bus," we have to assume it's telling
> us the truth.  The BIOS knows how the bridges are configured, and we
> don't.

Hi Bjorn

Thanks your reply quickly.

I understand what you say above, due to on our machine, three host bridge
window all are "root bus resource [mem 0x00000000-0x3fffffffffff]",
and after apply the patch, it also is ok, so I do ignore the consideration
for each host bridge window on restriction, thanks you point out it.

As you said, if we think about each host bridge, that do need to do more
thing, it do be a long-winded way.  I will reconsider this problem.

>> After apply this patch, this problem get resolved, the patch makes kernel
>> assign resource from behind of max_pfn for 64-bit pci devices when kernel
>> is 64BIT, however, previous way always assigns resource below 4GB no
>> matter it is 32 or 64-bit devices, so this patch extends this window of
>> pci resource on 64-bit system that both 32 and 64-bit pci device exist or
>> only 64-bit device exists, makes it more likely that we can assign
>> resource to more or all devices.
>>
>> Signed-off-by: wlkang <wlkang2008@126.com>
>> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
> I assume both of these Signed-off-by lines refer to you.  If that's
> the case, you should at least use the same "real name" (the "wlkang"
> and "Wenlin Kang" parts).  I think it makes the most sense to use a
> single address, but if you need both, I would put the second in a
> "CC:" line or something.  The Signed-off-by is more of an ownership
> thing where it doesn't really make sense to have two names for a
> single person.

Thinks, I see.

>> ---
>>   drivers/pci/setup-res.c |   12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
>> index 232f925..dafd667 100644
>> --- a/drivers/pci/setup-res.c
>> +++ b/drivers/pci/setup-res.c
>> @@ -206,7 +206,19 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
>>   	resource_size_t min;
>>   	int ret;
>>   
>> +#ifdef CONFIG_64BIT
>> +	/*
>> +	 * For 64-bit pci device, assign resource start from the next page
>> +	 * boundary above the maximum physical page address
>> +	 */
>> +	resource_size_t min_iomem;
>> +
>> +	min_iomem = (res->flags & IORESOURCE_MEM_64) ?
>> +				(max_pfn + 1) << PAGE_SHIFT : PCIBIOS_MIN_MEM;
>> +	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : min_iomem;
>> +#else
>>   	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
>> +#endif
>>   
>>   	/*
>>   	 * First, try exact prefetching match.  Even if a 64-bit
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
diff mbox

Patch

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 232f925..dafd667 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -206,7 +206,19 @@  static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	resource_size_t min;
 	int ret;
 
+#ifdef CONFIG_64BIT
+	/*
+	 * For 64-bit pci device, assign resource start from the next page
+	 * boundary above the maximum physical page address
+	 */
+	resource_size_t min_iomem;
+
+	min_iomem = (res->flags & IORESOURCE_MEM_64) ?
+				(max_pfn + 1) << PAGE_SHIFT : PCIBIOS_MIN_MEM;
+	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : min_iomem;
+#else
 	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
+#endif
 
 	/*
 	 * First, try exact prefetching match.  Even if a 64-bit