From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [PATCH] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap Date: Wed, 18 Apr 2012 16:01:47 +0100 Message-ID: <20120418150147.GI7013@ocelot.phlegethon.org> References: <495d3df95c1d59f55d5a.1334754400@xdev.gridcentric.ca> <20120418135952.GG7013@ocelot.phlegethon.org> <80232f2ec1589fa3d0be7f6a575eb7f1.squirrel@webmail.lagarcavilla.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <80232f2ec1589fa3d0be7f6a575eb7f1.squirrel@webmail.lagarcavilla.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andres Lagar-Cavilla Cc: andres@gridcentric.ca, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org At 07:18 -0700 on 18 Apr (1334733529), Andres Lagar-Cavilla wrote: > > At 09:06 -0400 on 18 Apr (1334740000), Andres Lagar-Cavilla wrote: > >> xen/arch/x86/mm/mem_sharing.c | 6 ++++-- > >> 1 files changed, 4 insertions(+), 2 deletions(-) > >> > >> > >> Signed-off-by: Andres Lagar-Cavilla > >> > >> diff -r 8609bbbba141 -r 495d3df95c1d xen/arch/x86/mm/mem_sharing.c > >> --- a/xen/arch/x86/mm/mem_sharing.c > >> +++ b/xen/arch/x86/mm/mem_sharing.c > >> @@ -1073,9 +1073,11 @@ int mem_sharing_add_to_physmap(struct do > >> if ( spage->sharing->handle != sh ) > >> goto err_unlock; > >> > >> - /* Make sure the target page is a hole in the physmap */ > >> + /* Make sure the target page is a hole in the physmap. This is not > >> as > >> + * simple a test as we'd like it to be. Non-existent p2m entries > >> return > >> + * invalid mfn and p2m_mmio_dm type when queried. */ > >> if ( mfn_valid(cmfn) || > >> - (!(p2m_is_ram(cmfn_type))) ) > >> + ( (!(p2m_is_ram(cmfn_type))) && (cmfn_type != p2m_mmio_dm) ) ) > > > > This test looks wrong, even after the fix. I think it should be testing > > for cmfn_type == p2m_mmio_dm || cmfn_type == p2m_invalid and ignoring > > mfn_valid(). > > Maybe :) > > I'm coming up with the semantics for this one, loosely based on the > regular populate physmap code path. That one can end up replacing existing > regular pages, or paged out entries, or PoD entries, with the new > populated pages (in guest_physmap_add_entry). I don't know that all of the > above is reasonable. > > But certainly I would like to keep the ability to replace paged out > entries with (identical) pages that are already present in other domains. Fair enough. Maybe you could define a new p2m-type-mask of all the things you think it's OK to replace in this kind of situation, and apply it in all cases? Tim.