From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH] x86: make show_page_walk() more robust Date: Thu, 24 Jan 2008 16:00:34 +0000 Message-ID: <4798C432.76E4.0078.0@novell.com> References: <4798B6E3.76E4.0078.0@novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org As I said, I found this during debugging - I had added explicit calls to show_page_walk(), and am also considering adding it to the early page fault handler (which provides pretty little information at present - the stub handler for all other exceptions is doing better - and does dubious [to me] things with a dubious [to me] comment). And regardless of that, the mfn_valid() check is absolutely required. Jan >>> Keir Fraser 24.01.08 16:35 >>> How can show_page_walk() be called before paging_init() sets up the M2P table? It is invoked only from exception and interrupt handlers, which are not installed until trap_init() has run. And trap_init() is invoked later than paging_init() during boot. -- Keir On 24/1/08 15:03, "Jan Beulich" wrote: > While adding 1Gb page support for the 1:1 mapping I noticed that > show_page_walk() crashes when used before the M2P table gets created, > and from looking at the code I realized that it would also crash if a > corrupt page table with an out of range MFN would be encountered. > While it would have been possible to make get_gpfn_from_mfn() more > robust, it seemed like keeping the overhead there low (as in the > general case proper values can be expected and would likely have been > checked for already), and hence I made the checks privates to > show_page_walk(). >=20 > Signed-off-by: Jan Beulich >=20 > Index: 2008-01-18/xen/arch/x86/x86_32/mm.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 2008-01-18.orig/xen/arch/x86/x86_32/mm.c 2008-01-23 11:54:09.00000000= 0 > +0100 > +++ 2008-01-18/xen/arch/x86/x86_32/mm.c 2008-01-23 11:48:16.000000000 = +0100 > @@ -41,6 +41,7 @@ l2_pgentry_t __attribute__ ((__section__ > unsigned int PAGE_HYPERVISOR =3D __PAGE_HYPERVISOR; > unsigned int PAGE_HYPERVISOR_NOCACHE =3D __PAGE_HYPERVISOR_NOCACHE; > =20 > +int mpt_valid; > static unsigned long mpt_size; > =20 > void *alloc_xen_pagetable(void) > @@ -110,6 +111,8 @@ void __init paging_init(void) > pg, (__PAGE_HYPERVISOR | _PAGE_PSE) & ~_PAGE_RW));= > } > =20 > + mpt_valid =3D 1; > + > /* Fill with an obvious debug pattern. */ > for ( i =3D 0; i < (mpt_size / BYTES_PER_LONG); i++) > set_gpfn_from_mfn(i, 0x55555555); > Index: 2008-01-18/xen/arch/x86/x86_32/traps.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 2008-01-18.orig/xen/arch/x86/x86_32/traps.c 2008-01-23 11:54:09.00000= 0000 > +0100 > +++ 2008-01-18/xen/arch/x86/x86_32/traps.c 2008-01-23 11:53:58.000000000 = +0100 > @@ -132,7 +132,8 @@ void show_page_walk(unsigned long addr) > l3t +=3D (cr3 & 0xFE0UL) >> 3; > l3e =3D l3t[l3_table_offset(addr)]; > mfn =3D l3e_get_pfn(l3e); > - pfn =3D get_gpfn_from_mfn(mfn); > + pfn =3D mfn_valid(mfn) && mpt_valid ? > + get_gpfn_from_mfn(mfn) : INVALID_M2P_ENTRY; > printk(" L3[0x%03lx] =3D %"PRIpte" %08lx\n", > l3_table_offset(addr), l3e_get_intpte(l3e), pfn); > unmap_domain_page(l3t); > @@ -143,7 +144,8 @@ void show_page_walk(unsigned long addr) > l2t =3D map_domain_page(mfn); > l2e =3D l2t[l2_table_offset(addr)]; > mfn =3D l2e_get_pfn(l2e); > - pfn =3D get_gpfn_from_mfn(mfn); > + pfn =3D mfn_valid(mfn) && mpt_valid ? > + get_gpfn_from_mfn(mfn) : INVALID_M2P_ENTRY; > printk(" L2[0x%03lx] =3D %"PRIpte" %08lx %s\n", > l2_table_offset(addr), l2e_get_intpte(l2e), pfn, > (l2e_get_flags(l2e) & _PAGE_PSE) ? "(PSE)" : ""); > @@ -155,7 +157,8 @@ void show_page_walk(unsigned long addr) > l1t =3D map_domain_page(mfn); > l1e =3D l1t[l1_table_offset(addr)]; > mfn =3D l1e_get_pfn(l1e); > - pfn =3D get_gpfn_from_mfn(mfn); > + pfn =3D mfn_valid(mfn) && mpt_valid ? > + get_gpfn_from_mfn(mfn) : INVALID_M2P_ENTRY; > printk(" L1[0x%03lx] =3D %"PRIpte" %08lx\n", > l1_table_offset(addr), l1e_get_intpte(l1e), pfn); > unmap_domain_page(l1t); > Index: 2008-01-18/xen/arch/x86/x86_64/mm.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 2008-01-18.orig/xen/arch/x86/x86_64/mm.c 2008-01-23 11:54:09.00000000= 0 > +0100 > +++ 2008-01-18/xen/arch/x86/x86_64/mm.c 2008-01-23 11:50:56.000000000 = +0100 > @@ -32,6 +32,7 @@ > #include > #include > =20 > +int mpt_valid; > #ifdef CONFIG_COMPAT > unsigned int m2p_compat_vstart =3D __HYPERVISOR_COMPAT_VIRT_START; > #endif > @@ -144,6 +145,8 @@ void __init paging_init(void) > l2_ro_mpt++; > } > =20 > + mpt_valid =3D 1; > + > /* Create user-accessible L2 directory to map the MPT for compat = guests. > */ > BUILD_BUG_ON(l4_table_offset(RDWR_MPT_VIRT_START) !=3D > l4_table_offset(HIRO_COMPAT_MPT_VIRT_START)); > Index: 2008-01-18/xen/arch/x86/x86_64/traps.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 2008-01-18.orig/xen/arch/x86/x86_64/traps.c 2008-01-23 11:54:09.00000= 0000 > +0100 > +++ 2008-01-18/xen/arch/x86/x86_64/traps.c 2008-01-23 11:53:27.000000000 = +0100 > @@ -136,7 +136,8 @@ void show_page_walk(unsigned long addr) > l4t =3D mfn_to_virt(mfn); > l4e =3D l4t[l4_table_offset(addr)]; > mfn =3D l4e_get_pfn(l4e); > - pfn =3D get_gpfn_from_mfn(mfn); > + pfn =3D mfn_valid(mfn) && mpt_valid ? > + get_gpfn_from_mfn(mfn) : INVALID_M2P_ENTRY; > printk(" L4[0x%03lx] =3D %"PRIpte" %016lx\n", > l4_table_offset(addr), l4e_get_intpte(l4e), pfn); > if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) ) > @@ -145,7 +146,8 @@ void show_page_walk(unsigned long addr) > l3t =3D mfn_to_virt(mfn); > l3e =3D l3t[l3_table_offset(addr)]; > mfn =3D l3e_get_pfn(l3e); > - pfn =3D get_gpfn_from_mfn(mfn); > + pfn =3D mfn_valid(mfn) && mpt_valid ? > + get_gpfn_from_mfn(mfn) : INVALID_M2P_ENTRY; > printk(" L3[0x%03lx] =3D %"PRIpte" %016lx\n", > l3_table_offset(addr), l3e_get_intpte(l3e), pfn); > if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) ) > @@ -154,7 +156,8 @@ void show_page_walk(unsigned long addr) > l2t =3D mfn_to_virt(mfn); > l2e =3D l2t[l2_table_offset(addr)]; > mfn =3D l2e_get_pfn(l2e); > - pfn =3D get_gpfn_from_mfn(mfn); > + pfn =3D mfn_valid(mfn) && mpt_valid ? > + get_gpfn_from_mfn(mfn) : INVALID_M2P_ENTRY; > printk(" L2[0x%03lx] =3D %"PRIpte" %016lx %s\n", > l2_table_offset(addr), l2e_get_intpte(l2e), pfn, > (l2e_get_flags(l2e) & _PAGE_PSE) ? "(PSE)" : ""); > @@ -165,7 +168,8 @@ void show_page_walk(unsigned long addr) > l1t =3D mfn_to_virt(mfn); > l1e =3D l1t[l1_table_offset(addr)]; > mfn =3D l1e_get_pfn(l1e); > - pfn =3D get_gpfn_from_mfn(mfn); > + pfn =3D mfn_valid(mfn) && mpt_valid ? > + get_gpfn_from_mfn(mfn) : INVALID_M2P_ENTRY; > printk(" L1[0x%03lx] =3D %"PRIpte" %016lx\n", > l1_table_offset(addr), l1e_get_intpte(l1e), pfn); > } > Index: 2008-01-18/xen/include/asm-x86/mm.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 2008-01-18.orig/xen/include/asm-x86/mm.h 2008-01-23 11:54:09.00000000= 0 > +0100 > +++ 2008-01-18/xen/include/asm-x86/mm.h 2008-01-23 11:55:37.000000000 = +0100 > @@ -267,6 +267,7 @@ TYPE_SAFE(unsigned long,mfn); > #define machine_to_phys_mapping ((unsigned long *)RDWR_MPT_VIRT_START) > #define INVALID_M2P_ENTRY (~0UL) > #define VALID_M2P(_e) (!((_e) & (1UL<<(BITS_PER_LONG-1)))) > +extern int mpt_valid; > =20 > #ifdef CONFIG_COMPAT > #define compat_machine_to_phys_mapping ((unsigned int > *)RDWR_COMPAT_MPT_VIRT_START) >=20 >=20 >=20 > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com=20 > http://lists.xensource.com/xen-devel=20