Re: [PATCH] minios: don't rely on specific page table allocation scheme

[Juergen Gross <jgross@suse.com>]

On 20/11/15 13:46, Wei Liu wrote:
> On Thu, Nov 19, 2015 at 05:05:16PM +0100, Juergen Gross wrote:
>> Today mini-os is making assumptions how the page tables it is started
>> with are being allocated. Especially it is using the number of page
>> table frames to calculate which is the first unmapped pfn.
>>
>> Instead of relying on page table number assumptions just look into the
>> page tables to find the first pfn not already mapped.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I ran both pv-grub-x86_{64,32} up to the point when grub menu was
> shown -- That's beyond the point of the original crash.
> 
> I also ran mini-os 32 and 64 bit with this patch on toolstack without
> large p2m series. They worked.
> 
>> ---
>>  arch/x86/mm.c         | 47 ++++++++++++++++++++++++++++++++++++-----------
>>  include/x86/arch_mm.h |  7 -------
>>  2 files changed, 36 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
>> index 9c6d1b8..5d7c006 100644
>> --- a/arch/x86/mm.c
>> +++ b/arch/x86/mm.c
>> @@ -200,8 +200,8 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
>>      int count = 0;
>>      int rc;
>>  
>> -    pfn_to_map = 
>> -        (start_info.nr_pt_frames - NOT_L1_FRAMES) * L1_PAGETABLE_ENTRIES;
>> +    pfn_to_map = (*start_pfn + L1_PAGETABLE_ENTRIES - 1) &
>> +                 ~(L1_PAGETABLE_ENTRIES - 1);
>>  
>>      if ( *max_pfn >= virt_to_pfn(HYPERVISOR_VIRT_START) )
>>      {
>> @@ -229,9 +229,15 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
>>  #if defined(__x86_64__)
>>          offset = l4_table_offset(start_address);
>>          /* Need new L3 pt frame */
>> -        if ( !(start_address & L3_MASK) )
>> -            if ( need_pt_frame(start_address, L3_FRAME) ) 
>> -                new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
>> +        if ( !(tab[offset] & _PAGE_PRESENT) )
>> +        {
>> +            if ( !need_pt_frame(start_address, L3_FRAME) )
>> +            {
>> +                printk("ERROR: build_pagetable(): L3 frame not present\n");
>> +                do_exit();
>> +            }
>> +            new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> 
> I'm not sure about this hunk and other similar hunks. Shouldn't this be
> 
>                if ( need_pt_frame(start_address, L3_FRAME) )
> 	           new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> 
> That is, just like before.

I didn't want to keep it like before. What sense does it make to
find a page table is not existing and to decide it don't need to be
created as it should exist already and then to carry on as if it would
be there? The only viable alternative would have been to nuke the
need_pt_frame() call.

Juergen