Re: [PATCH 0/4] memblock related fixes for -tip

[Jeremy Fitzhardinge <jeremy@goop.org>]

 On 10/13/2010 05:08 PM, Yinghai wrote:
>
> On Oct 13, 2010, at 4:34 PM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>> On 10/13/2010 04:18 PM, H. Peter Anvin wrote:
>>> On 10/13/2010 04:14 PM, Jeremy Fitzhardinge wrote:
>>>> The Xen code is setting the max_pfn_mapped correctly, but it is just
>>>> being overridden by:
>>>>
>>>>    max_pfn_mapped = KERNEL_IMAGE_SIZE >> PAGE_SHIFT;
>>>>
>>>> in setup_arch() - and KERNEL_IMAGE_SIZE is hard-coded to 512MB...
>>>>
>>>> How is this correct?  Does kernel/head_64.S map everything up to 512MB
>>>> or something?
>>>>
>>> Yes:
>>>        /*
>>>         * 512 MB kernel mapping. We spend a full page on this pagetable
>>>         * anyway.
>>>         *
>>>         * The kernel code+data+bss must not be bigger than that.
>>>         *
>>>         * (NOTE: at +512MB starts the module area, see MODULES_VADDR.
>>>         *  If you want to increase this then increase MODULES_VADDR
>>>         *  too.)
>>>         */
>>>        PMDS(0, __PAGE_KERNEL_LARGE_EXEC,
>>>                KERNEL_IMAGE_SIZE/PMD_SIZE)
>>>
>>> This is, however, wrong in the sense that it obviously shouldn't be
>>> getting executed on the Xen codepath.
>> I see.  head_32.S sets max_pfn_mapped for itself, so head_64.S should
>> probably do that too.  Patch below, but I haven't checked to see if it
>> fixes the original problem (but I don't see why the pagetable memory
>> allocation should be limited to the mapped region, especially if you
>> want to put it as high as possible, especially since the mm/init_64.c
>> code doesn't expect it to be mapped).
>>
>>    J
>>
>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>> Date: Wed, 13 Oct 2010 16:31:11 -0700
>> Subject: [PATCH] x86-64: don't force max_pfn_mapped to 512MB
>>
>> head_64.S maps up to 512MB, but the Xen entry path doesn't necessarily
>> map that high.  Set max_pfn_mapped in head_64.S accordingly.  (The Xen
>> code already sets max_pfn_mapped correctly.)
>>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>
>> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>> index 239046b..b75c6f4 100644
>> --- a/arch/x86/kernel/head_64.S
>> +++ b/arch/x86/kernel/head_64.S
>> @@ -141,6 +141,8 @@ ident_complete:
>>    addq    %rbp, trampoline_level4_pgt + (511*8)(%rip)
>> #endif
>>
>> +    movq    $KERNEL_IMAGE_SIZE / PAGE_SIZE, max_pfn_mapped(%rip)
>> +
>>    /* Due to ENTRY(), sometimes the empty space gets filled with
>>     * zeros. Better take a jmp than relying on empty space being
>>     * filled with 0x90 (nop)
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index b11a238..c3cebfe 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -932,7 +932,6 @@ void __init setup_arch(char **cmdline_p)
>>        max_low_pfn = max_pfn;
>>
>>    high_memory = (void *)__va(max_pfn * PAGE_SIZE - 1) + 1;
>> -    max_pfn_mapped = KERNEL_IMAGE_SIZE >> PAGE_SHIFT;
>> #endif
>>
>>    /*
>>
> I like this one, can you check if only this one is needed ?

It is sufficient, but I'd prefer to have both.  I don't think limiting
the pagetable allocation to mapped memory is necessarily correct, and
there's no harm in making the Xen code more robust (plus it cleans up a
couple of #ifdefs).

    J