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

[Wei Liu <wei.liu2@citrix.com>]

On Fri, Nov 20, 2015 at 01:55:29PM +0100, Juergen Gross wrote:
> 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.
> 

Yeah, just nuking the need_pt_frame is fine. That's simpler.

Wei.


> Juergen
>