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

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-11-20 at 14:52 +0100, Juergen Gross wrote:

Copying minios-devel.

I'll send out a patch to MAINTAINERS to add this in a second.

> 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>
> ---
> Changes in V2:
> - remove need_pt_frame() as it simplifies code (suggested by Wei Liu)
> 
> ---
>  arch/x86/mm.c         | 81 ++++++++++-----------------------------------
> ------
>  include/x86/arch_mm.h |  7 -----
>  2 files changed, 15 insertions(+), 73 deletions(-)
> 
> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> index 9c6d1b8..b828efc 100644
> --- a/arch/x86/mm.c
> +++ b/arch/x86/mm.c
> @@ -132,61 +132,6 @@ static void new_pt_frame(unsigned long *pt_pfn,
> unsigned long prev_l_mfn,
>  }
>  
>  /*
> - * Checks if a pagetable frame is needed at 'level' to map a given
> - * address. Note, this function is specific to the initial page table
> - * building.
> - */
> -static int need_pt_frame(unsigned long va, int level)
> -{
> -    unsigned long hyp_virt_start = HYPERVISOR_VIRT_START;
> -#if defined(__x86_64__)
> -    unsigned long hyp_virt_end = HYPERVISOR_VIRT_END;
> -#else
> -    unsigned long hyp_virt_end = 0xffffffff;
> -#endif
> -
> -    /* In general frames will _not_ be needed if they were already
> -       allocated to map the hypervisor into our VA space */
> -#if defined(__x86_64__)
> -    if ( level == L3_FRAME )
> -    {
> -        if ( l4_table_offset(va) >= 
> -             l4_table_offset(hyp_virt_start) &&
> -             l4_table_offset(va) <= 
> -             l4_table_offset(hyp_virt_end))
> -            return 0;
> -        return 1;
> -    } 
> -    else
> -#endif
> -
> -    if ( level == L2_FRAME )
> -    {
> -#if defined(__x86_64__)
> -        if ( l4_table_offset(va) >= 
> -             l4_table_offset(hyp_virt_start) &&
> -             l4_table_offset(va) <= 
> -             l4_table_offset(hyp_virt_end))
> -#endif
> -            if ( l3_table_offset(va) >= 
> -                 l3_table_offset(hyp_virt_start) &&
> -                 l3_table_offset(va) <= 
> -                 l3_table_offset(hyp_virt_end))
> -                return 0;
> -
> -        return 1;
> -    } 
> -    else 
> -        /* Always need l1 frames */
> -        if ( level == L1_FRAME )
> -            return 1;
> -
> -    printk("ERROR: Unknown frame level %d, hypervisor %llx,%llx\n", 
> -           level, hyp_virt_start, hyp_virt_end);
> -    return -1;
> -}
> -
> -/*
>   * Build the initial pagetable.
>   */
>  static void build_pagetable(unsigned long *start_pfn, unsigned long
> *max_pfn)
> @@ -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);
>  
>      if ( *max_pfn >= virt_to_pfn(HYPERVISOR_VIRT_START) )
>      {
> @@ -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);
>  
>          page = tab[offset];
>          pt_mfn = pte_to_mfn(page);
> @@ -239,18 +183,23 @@ static void build_pagetable(unsigned long
> *start_pfn, unsigned long *max_pfn)
>  #endif
>          offset = l3_table_offset(start_address);
>          /* Need new L2 pt frame */
> -        if ( !(start_address & L2_MASK) )
> -            if ( need_pt_frame(start_address, L2_FRAME) )
> -                new_pt_frame(&pt_pfn, pt_mfn, offset, L2_FRAME);
> +        if ( !(tab[offset] & _PAGE_PRESENT) )
> +            new_pt_frame(&pt_pfn, pt_mfn, offset, L2_FRAME);
>  
>          page = tab[offset];
>          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. */
> +            start_address += L1_PAGETABLE_ENTRIES << PAGE_SHIFT;
> +            pfn_to_map += L1_PAGETABLE_ENTRIES;
> +            continue;
> +        }
>  
>          page = tab[offset];
>          pt_mfn = pte_to_mfn(page);
> diff --git a/include/x86/arch_mm.h b/include/x86/arch_mm.h
> index 23cfca7..58f29fc 100644
> --- a/include/x86/arch_mm.h
> +++ b/include/x86/arch_mm.h
> @@ -56,12 +56,6 @@
>  
>  #define L2_MASK  ((1UL << L3_PAGETABLE_SHIFT) - 1)
>  
> -/*
> - * If starting from virtual address greater than 0xc0000000,
> - * this value will be 2 to account for final mid-level page
> - * directory which is always mapped in at this location.
> - */
> -#define NOT_L1_FRAMES           3
>  #define PRIpte "016llx"
>  #ifndef __ASSEMBLY__
>  typedef uint64_t pgentry_t;
> @@ -87,7 +81,6 @@ typedef uint64_t pgentry_t;
>  #define L2_MASK  ((1UL << L3_PAGETABLE_SHIFT) - 1)
>  #define L3_MASK  ((1UL << L4_PAGETABLE_SHIFT) - 1)
>  
> -#define NOT_L1_FRAMES           3
>  #define PRIpte "016lx"
>  #ifndef __ASSEMBLY__
>  typedef unsigned long pgentry_t;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel