Re: [PATCH v8 21/46] x86, mm: setup page table in top-down

[Yinghai Lu <yinghai@kernel.org>]

On Wed, Nov 28, 2012 at 9:50 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>>  /*
>> - * Iterate through E820 memory map and create direct mappings for only E820_RAM
>> - * regions. We cannot simply create direct mappings for all pfns from
>> - * [0 to max_low_pfn) and [4GB to max_pfn) because of possible memory holes in
>> - * high addresses that cannot be marked as UC by fixed/variable range MTRRs.
>> - * Depending on the alignment of E820 ranges, this may possibly result in using
>> - * smaller size (i.e. 4K instead of 2M or 1G) page tables.
>> + * would have hole in the middle or ends, and only ram parts will be mapped.
>
>
> What? What is the 'would' refering to? Why remove a good comment that explains
> the function. Why not just modify it a bit please?
>

==> update to

/*
 * We need to iterate through E820 memory map and create direct mappings
 * for only E820_RAM and E820_KERN_RESERVED regions. We cannot simply
 * create direct mappings for all pfns from [0 to max_low_pfn) and
 * [4GB to max_pfn) because of possible memory holes in high addresses
 * that cannot be marked as UC by fixed/variable range MTRRs.
 * Depending on the alignment of E820 ranges, this may possibly result
 * in using smaller size (i.e. 4K instead of 2M or 1G) page tables.
 *
 * init_mem_mapping call init_range_memory_mapping with big range.
 * That range would have hole in the middle or ends, and only ram parts
 * will be mapped in init_range_memory_mapping.
 */



>> -     max_pfn_mapped = 0; /* will get exact value next */
>>       /* the ISA range is always mapped regardless of memory holes */
>>       init_memory_mapping(0, ISA_END_ADDRESS);
>> -     init_range_memory_mapping(ISA_END_ADDRESS, end);
>> +
>> +     /* xen has big range in reserved near end of ram, skip it at first */
>
> I am not seeing the logic for doing it? The loop is quite generic
> in doing it in reverse order, and the memblock_find_in_range
> gets a nice PMD_SIZE region from the end of the memory.
>
> If the memory at the end is reserved, then it looks like it won't
> be even considered in the loop, but it does get included in the fallback:
>
>         if (real_end < end)
>                 init_range_memory_mapping(real_end, end);

that reserved in in memblock.reserved and it is not in e820.

so memblock.memory will have that range too. then if we use all of
first 2M to map

those reserved range, we would not have enough mapped pages to be used
as new page tables.

>
>
>
>> +     addr = memblock_find_in_range(ISA_END_ADDRESS, end, PMD_SIZE,
>> +                      PAGE_SIZE);
>> +     real_end = addr + PMD_SIZE;
>> +
>> +     /* step_size need to be small so pgt_buf from BRK could cover it */
>> +     step_size = PMD_SIZE;
>> +     max_pfn_mapped = 0; /* will get exact value next */
>> +     min_pfn_mapped = real_end >> PAGE_SHIFT;
>> +     last_start = start = real_end;
>
> Everytime I look at this loop, I keep on forgetting that it goes in reverse.
> I am not sure if it is just me, but it might be useful for other
> folks who are going to look at this in a year or so to have
> a little hint:
>
> N.B. We start from the top (end of memory) and go to the bottom. The
> memblock_find_in_range gets us a block of RAM from the end
> of RAM.

put the that in the comments.