From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH] minios: don't rely on specific page table allocation scheme Date: Fri, 20 Nov 2015 13:43:13 +0000 Message-ID: <20151120134313.GY1495@citrix.com> References: <1447949116-9543-1-git-send-email-jgross@suse.com> <20151120124625.GX1495@citrix.com> <564F1841.3040305@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <564F1841.3040305@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Juergen Gross Cc: Wei Liu , Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, samuel.thibault@ens-lyon.org List-Id: xen-devel@lists.xenproject.org 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 > > > > 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 >