Re: [PATCH 02/11] PCI: Try to allocate mem64 above 4G at first

[Yinghai Lu <yinghai@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3819 bytes --]

On Fri, May 25, 2012 at 12:37 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, May 25, 2012 at 11:39:26AM -0700, Yinghai Lu wrote:
>> On Fri, May 25, 2012 at 10:53 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> >> I don't really like the dependency on PCIBIOS_MAX_MEM_32 + 1ULL
>> >> overflowing to zero -- that means the reader has to know what the
>> >> value of PCIBIOS_MAX_MEM_32 is, and things would break in non-obvious
>> >> ways if we changed it.
>> >>
>>
>> please check if attached one is more clear.
>>
>> make max and bottom is only related to _MEM and not default one.
>>
>> -       if (!(res->flags & IORESOURCE_MEM_64))
>> -               max = PCIBIOS_MAX_MEM_32;
>> +       if (res->flags & IORESOURCE_MEM) {
>> +               if (!(res->flags & IORESOURCE_MEM_64))
>> +                       max = PCIBIOS_MAX_MEM_32;
>> +               else if (PCIBIOS_MAX_MEM_32 != -1)
>> +                       bottom = (resource_size_t)(1ULL<<32);
>> +       }
>>
>> will still not affect to other arches.
>
> That's goofy.  You're proposing to make only x86_64 and x86-PAE try to put
> 64-bit BARs above 4GB.  Why should this be specific to x86?  I acknowledge
> that there's risk in doing this, but if it's a good idea for x86_64, it
> should also be a good idea for other 64-bit architectures.
>
> And testing for "is this x86_32 without PAE?" with
> "PCIBIOS_MAX_MEM_32 == -1" is just plain obtuse and hides an
> important bit of arch-specific behavior.
>
> Tangential question about allocate_resource():  Is its "max" argument
> really necessary?  We'll obviously only allocate from inside the root
> resource, so "max" is just a way to artificially avoid the end of
> that resource.  Is there really a case where that's required?
>
> "min" makes sense because in a case like this, it's valid to allocate from
> anywhere in the root resource, but we want to try to allocate from the >4GB
> part first, then fall back to allocating from the whole resource.  I'm not
> sure there's a corresponding case for "max."
>
> Getting back to this patch, I don't think we should need to adjust "max" at
> all.  For example, this:
>
> commit cb1c8e46244cfd84a1a2fe91be860a74c1cf4e25
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Thu May 24 22:15:26 2012 -0600
>
>    PCI: try to allocate 64-bit mem resources above 4GB
>
>    If we have a 64-bit mem resource, try to allocate it above 4GB first.  If
>    that fails, we'll fall back to allocating space below 4GB.
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 4ce5ef2..075e5b1 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -121,14 +121,16 @@ pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
>  {
>        int i, ret = -ENOMEM;
>        struct resource *r;
> -       resource_size_t max = -1;
> +       resource_size_t start = 0;
> +       resource_size_t end = MAX_RESOURCE;

yeah, MAX_RESOURCE is better than -1.

>
>        type_mask |= IORESOURCE_IO | IORESOURCE_MEM;
>
> -       /* don't allocate too high if the pref mem doesn't support 64bit*/
> -       if (!(res->flags & IORESOURCE_MEM_64))
> -               max = PCIBIOS_MAX_MEM_32;

can not remove this one.
otherwise will could allocate above 4g range to non MEM64 resource.


> +       /* If this is a 64-bit mem resource, try above 4GB first */
> +       if (res->flags & IORESOURCE_MEM_64)
> +               start = (resource_size_t) (1ULL << 32);

could affect other arches. let's see if other arches is ok.

please check merged version.

also we have

include/linux/range.h:#define MAX_RESOURCE ((resource_size_t)~0)
arch/x86/kernel/e820.c:#define MAX_RESOURCE_SIZE ((resource_size_t)-1)

we should merge them later?

Thanks

Yinghai

[-- Attachment #2: allocate_high_at_first_v3.patch --]
[-- Type: application/octet-stream, Size: 2249 bytes --]

Subject: [PATCH] PCI: Try to allocate mem64 above 4G at first

and will fall back to below 4g if it can not find any above 4g.

only x86 have PCIBIOS_MAX_MEM_32 set to 0xffffffff.
it will only affect x86_64 and 32bit with resource_size_t 64bit support.
only that case bottom is set to 4g for first try.

x86 32bit without X86_PAE support will have bottom set to 0, becuase
resource_size_t is 32bit.

Also for 32bit with resource_size_t 64bit kernel on machine with pae support
we are safe because iomem_resource is limited to 32bit according to
x86_phys_bits.

-v2: update bottom assigning to make it clear for non-pae support machine.
-v3: Bjorn's change:
	use MAX_REOURCE instead of -1
	use start/end instead of bottom/max
	for all arch instead of just x86_64

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/bus.c |   29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/pci/bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/bus.c
+++ linux-2.6/drivers/pci/bus.c
@@ -121,14 +121,23 @@ pci_bus_alloc_resource(struct pci_bus *b
 {
 	int i, ret = -ENOMEM;
 	struct resource *r;
-	resource_size_t max = -1;
+	resource_size_t end = MAX_RESOURCE;
+	resource_size_t start = 0;
 
 	type_mask |= IORESOURCE_IO | IORESOURCE_MEM;
 
-	/* don't allocate too high if the pref mem doesn't support 64bit*/
-	if (!(res->flags & IORESOURCE_MEM_64))
-		max = PCIBIOS_MAX_MEM_32;
+	/*
+	 * don't allocate too high if the pref mem doesn't support 64bit
+	 * also if this is a 64-bit mem resource, try above 4GB first
+	 */
+	if (res->flags & IORESOURCE_MEM) {
+		if (!(res->flags & IORESOURCE_MEM_64))
+			end = PCIBIOS_MAX_MEM_32;
+		else
+			start = (resource_size_t)(1ULL<<32);
+	}
 
+again:
 	pci_bus_for_each_resource(bus, r, i) {
 		if (!r)
 			continue;
@@ -145,12 +154,18 @@ pci_bus_alloc_resource(struct pci_bus *b
 
 		/* Ok, try it out.. */
 		ret = allocate_resource(r, res, size,
-					r->start ? : min,
-					max, align,
+					max(start, r->start ? : min),
+					end, align,
 					alignf, alignf_data);
 		if (ret == 0)
-			break;
+			return 0;
+	}
+
+	if (start != 0) {
+		start = 0;
+		goto again;
 	}
+
 	return ret;
 }