[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] minios: don't rely on specific page table allocation scheme
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |