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

[Juergen Gross <jgross@suse.com>]

On 20/11/15 15:35, Samuel Thibault wrote:
> Hello,
> 
> Juergen Gross, on Fri 20 Nov 2015 14:52:57 +0100, 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.
> 
> I agree on the principle
> 
>> @@ -200,8 +145,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);
> 
> Why aligning up on L1_PAGETABLE_ENTRIES.  Because Xen always maps
> a whole L1 pt frame?  I'd say just assume even less by just taking
> *start_pfn.  Yes, it's a (small) waste, but it makes the code less
> obscure.  What do you think Wei?
> 
>> @@ -229,9 +174,8 @@ 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) )
>> +            new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> 
> It replaces a test on the address with systematically reading the tab,
> but that tab will most probably be in the L1d cache, so not much more
> costly than the test while getting more readable code.
> 
>>          pt_mfn = pte_to_mfn(page);
>>          tab = to_virt(mfn_to_pfn(pt_mfn) << PAGE_SHIFT);
>>          offset = l2_table_offset(start_address);        
>>          /* Need new L1 pt frame */
>> -        if ( !(start_address & L1_MASK) )
>> -            if ( need_pt_frame(start_address, L1_FRAME) )
>> -                new_pt_frame(&pt_pfn, pt_mfn, offset, L1_FRAME);
>> +        if ( !(tab[offset] & _PAGE_PRESENT) )
>> +            new_pt_frame(&pt_pfn, pt_mfn, offset, L1_FRAME);
>> +        else if ( !(start_address & L1_MASK) )
>> +        {
>> +            /* Already mapped, skip this L1 entry. */
> 
> Again, I'd say not assume anything here, to keep the code simple at the
> expense of a waste. It means instead...
> 
>> +            start_address += L1_PAGETABLE_ENTRIES << PAGE_SHIFT;
>> +            pfn_to_map += L1_PAGETABLE_ENTRIES;
>> +            continue;
>> +        }
>>  
>>          page = tab[offset];
>>          pt_mfn = pte_to_mfn(page);
> tab = to_virt(mfn_to_pfn(pt_mfn) << PAGE_SHIFT);
>>          offset = l1_table_offset(start_address);
> 
> ... checking tab[offset] & _PAGE_PRESENT here before adding an MMU update.
> 
> In the end that'll make us re-read the whole L1 page tables built by the
> domain builder, but this looks cheap enough while being more readable
> code and safer to me.

Okay, I'll have a try.


Juergen