Re: [PATCH] x86/pci: make pci_mem_start to be aligned only -v4

[Yinghai Lu <yinghai@kernel.org>]

Yinghai Lu wrote:
> Linus Torvalds wrote:
>> On Sat, 18 Apr 2009, Yinghai Lu wrote:
>>> except need to change 
>>>> +		reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");
>>> ==> > +		reserve_region_with_split(&iomem_resource, start, end - 1, "RAM buffer");
>> Yes, I sent out a later email pointing that out.
>>
>>> it will make sure dynmical allocating code will not use those range.
>>>
>>> and could make e820_setup_gap much simple.
>> ACK. In fact:
>>
>>> Index: linux-2.6/arch/x86/kernel/e820.c
>>> ===================================================================
>>> --- linux-2.6.orig/arch/x86/kernel/e820.c
>>> +++ linux-2.6/arch/x86/kernel/e820.c
>>> @@ -635,14 +635,12 @@ __init void e820_setup_gap(void)
>>>  #endif
>>>  
>>>  	/*
>>> -	 * See how much we want to round up: start off with
>>> -	 * rounding to the next 1MB area.
>>> +	 * e820_reserve_resources_late will protect stolen RAM
>>> +	 * so just round it to 1M
>>>  	 */
>>>  	round = 0x100000;
>>> -	while ((gapsize >> 4) > round)
>>> -		round += round;
>>> -	/* Fun with two's complement */
>>> -	pci_mem_start = (gapstart + round) & -round;
>>> +
>>> +	pci_mem_start = roundup(gapstart, round);
>> You can just remove "round" entirely. It's no longer a variable, it's just 
>> an odd way of saying 1M ;)
>>
>>> Ingo, can you put those two patches in tip?
>> I would suggest that we first change "reserve_region_with_split()" to not 
>> recurse into the region.
>>
>> That function isn't used by anything else (we ended up using 
>> "expand_to_fit()" instead in the one place that migth have used it), and 
>> now th eone caller we do have would not want the recursion - if there 
>> already exists a resource at the top level, we want to just avoid it.
>>
>> This - again TOTALLY UNTESTED - patch removes the "recurse into conflicts" 
>> code. Comments? Testing?
>>
>> 			Linus
>> ---
>>  kernel/resource.c |   46 ++++++++++++----------------------------------
>>  1 files changed, 12 insertions(+), 34 deletions(-)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index fd5d7d5..ac5f3a3 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -533,43 +533,21 @@ static void __init __reserve_region_with_split(struct resource *root,
>>  	res->end = end;
>>  	res->flags = IORESOURCE_BUSY;
>>  
>> -	for (;;) {
>> -		conflict = __request_resource(parent, res);
>> -		if (!conflict)
>> -			break;
>> -		if (conflict != parent) {
>> -			parent = conflict;
>> -			if (!(conflict->flags & IORESOURCE_BUSY))
>> -				continue;
>> -		}
>> -
>> -		/* Uhhuh, that didn't work out.. */
>> -		kfree(res);
>> -		res = NULL;
>> -		break;
>> -	}
>> -
>> -	if (!res) {
>> -		/* failed, split and try again */
>> -
>> -		/* conflict covered whole area */
>> -		if (conflict->start <= start && conflict->end >= end)
>> -			return;
>> +	conflict = __request_resource(parent, res);
>> +	if (!conflict)
>> +		return;
>>  
>> -		if (conflict->start > start)
>> -			__reserve_region_with_split(root, start, conflict->start-1, name);
>> -		if (!(conflict->flags & IORESOURCE_BUSY)) {
>> -			resource_size_t common_start, common_end;
>> +	/* failed, split and try again */
>> +	kfree(res);
>>  
>> -			common_start = max(conflict->start, start);
>> -			common_end = min(conflict->end, end);
>> -			if (common_start < common_end)
>> -				__reserve_region_with_split(root, common_start, common_end, name);
>> -		}
>> -		if (conflict->end < end)
>> -			__reserve_region_with_split(root, conflict->end+1, end, name);
>> -	}
>> +	/* conflict covered whole area */
>> +	if (conflict->start <= start && conflict->end >= end)
>> +		return;
>>  
>> +	if (conflict->start > start)
>> +		__reserve_region_with_split(root, start, conflict->start-1, name);
>> +	if (conflict->end < end)
>> +		__reserve_region_with_split(root, conflict->end+1, end, name);
>>  }
>>  
>>  void __init reserve_region_with_split(struct resource *root,
> 
> with
> [    0.000000] BIOS-provided physical RAM map:
> [    0.000000]  BIOS-e820: 0000000000000100 - 0000000000097400 (usable)
> [    0.000000]  BIOS-e820: 0000000000097400 - 00000000000a0000 (reserved)
> [    0.000000]  BIOS-e820: 0000000000100000 - 00000000b7fa0000 (usable)
> [    0.000000]  BIOS-e820: 00000000b7fae000 - 00000000b7fb0000 (usable)
> [    0.000000]  BIOS-e820: 00000000b7fb0000 - 00000000b7fbe000 (ACPI data)
> [    0.000000]  BIOS-e820: 00000000b7fbe000 - 00000000b7ff0000 (ACPI NVS)
> [    0.000000]  BIOS-e820: 00000000b7ff0000 - 00000000b8000000 (reserved)
> [    0.000000]  BIOS-e820: 00000000e0000000 - 00000000f0000000 (reserved)
> [    0.000000]  BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
> [    0.000000]  BIOS-e820: 00000000fee00000 - 00000000fef00000 (reserved)
> [    0.000000]  BIOS-e820: 00000000ff700000 - 0000000100000000 (reserved)
> [    0.000000]  BIOS-e820: 0000000100000000 - 0000002048000000 (usable)
> 
> got
> 
> 00000100-000973ff : System RAM
> 00097400-0009ffff : reserved
> 000a0000-000bffff : PCI Bus #00
> 000c0000-000cffff : pnp 00:0c
> 000e0000-000fffff : pnp 00:0c
> 00100000-b7f9ffff : System RAM
>   00200000-00c68f6b : Kernel code
>   00c68f6c-01332f7f : Kernel data
>   015a6000-01fcaa57 : Kernel bss
>   20000000-23ffffff : GART
> b7fa0000-b7fadfff : RAM buffer
> b7fae000-b7faffff : System RAM
> b7fb0000-b7fbdfff : ACPI Tables
> b7fbe000-b7feffff : ACPI Non-volatile Storage
> b7ff0000-b7ffffff : reserved
> ...
> 
without your patch got
00000100-000973ff : System RAM
00097400-0009ffff : reserved
000a0000-000bffff : PCI Bus #00
000c0000-000cffff : pnp 00:0c
000e0000-000fffff : pnp 00:0c
00100000-b7f9ffff : System RAM
  00200000-00c68f6b : Kernel code
  00c68f6c-01332f7f : Kernel data
  015a6000-01fcaa57 : Kernel bss
  20000000-23ffffff : GART
b7fa0000-b7fadfff : RAM buffer
b7fae000-b7faffff : System RAM
b7fb0000-b7fbdfff : ACPI Tables
b7fbe000-b7feffff : ACPI Non-volatile Storage
b7ff0000-b7ffffff : reserved
  b7ff0000-b7ffffff : RAM buffer
b8000000-beffffff : PCI Bus #00
bf000000-bfffffff : PCI Bus #80