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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel