From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olaf Hering Subject: Re: bogus gfn - mfn - gfn - mfn checks in guest_physmap_add_entry Date: Thu, 25 Nov 2010 16:03:10 +0100 Message-ID: <20101125150310.GA12431@aepfle.de> References: <20101123210158.GA9425@aepfle.de> <20101124102202.GF19638@whitby.uk.xensource.com> <20101124144138.GA25619@aepfle.de> <20101124145326.GH19638@whitby.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Return-path: Content-Disposition: inline In-Reply-To: <20101124145326.GH19638@whitby.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Tim Deegan Cc: Patrick Colp , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On Wed, Nov 24, Tim Deegan wrote: > The problem is that PV guests set their own m2p entries and can't be > relied on to tear them down. What needs to happen for PV guests? Dont they use the machine_to_phys_mapping[] array like HVM guests? > The guest_physmap_add_entry code, and the p2m audit code, would be made > more reliable if, say, alloc_domheap_pages and/or free_domheap_pages > zapped the m2p entries for MFNs they touched. > > I think originally that wasn't done because the alloc is quickly > followed by another write of the m2p but that's probably over-keen > optimization. Could it be done like that? (not yet compile-tested) The mfn is probably always valid. I see memory_exchange uses assign_pages() to move mfns from one domain to another (havent studied the whole function yet). I think thats another place that needs an audit wether the machine_to_phys_mapping[] array is maintained properly. --- xen-4.0.1-testing.orig/xen/common/page_alloc.c +++ xen-4.0.1-testing/xen/common/page_alloc.c @@ -1146,6 +1146,8 @@ struct page_info *alloc_domheap_pages( struct page_info *pg = NULL; unsigned int bits = memflags >> _MEMF_bits, zone_hi = NR_ZONES - 1; unsigned int node = (uint8_t)((memflags >> _MEMF_node) - 1), dma_zone; + int i; + mfn_t mfn; ASSERT(!in_irq()); @@ -1170,6 +1172,13 @@ struct page_info *alloc_domheap_pages( free_heap_pages(pg, order); return NULL; } + /* this page is not yet a gfn */ + mfn = page_to_mfn(pg); + if (mfn_valid(mfn)) + { + for ( i = 0; i < (1 << order); i++ ) + set_gpfn_from_mfn(mfn_x(mfn) + j, INVALID_M2P_ENTRY); + } return pg; } @@ -1178,9 +1187,18 @@ void free_domheap_pages(struct page_info { int i, drop_dom_ref; struct domain *d = page_get_owner(pg); + mfn_t mfn; ASSERT(!in_irq()); + /* this page is not a gfn anymore */ + mfn = page_to_mfn(pg); + if (mfn_valid(mfn)) + { + for ( i = 0; i < (1 << order); i++ ) + set_gpfn_from_mfn(mfn_x(mfn) + j, INVALID_M2P_ENTRY); + } + if ( unlikely(is_xen_heap_page(pg)) ) { /* NB. May recursively lock from relinquish_memory(). */