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

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

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.

Samuel