From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Andres Lagar-Cavilla" Subject: Re: [PATCH] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap Date: Wed, 18 Apr 2012 08:13:32 -0700 Message-ID: <73a9d24a68ebfc8358cb55c6b4864831.squirrel@webmail.lagarcavilla.org> References: <495d3df95c1d59f55d5a.1334754400@xdev.gridcentric.ca> <20120418135952.GG7013@ocelot.phlegethon.org> <80232f2ec1589fa3d0be7f6a575eb7f1.squirrel@webmail.lagarcavilla.org> <20120418150147.GI7013@ocelot.phlegethon.org> Reply-To: andres@lagarcavilla.org Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20120418150147.GI7013@ocelot.phlegethon.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: Tim Deegan 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? Is there a chance for a p2m_mmio_dm entry to hold a valid mfn? Thanks, Andres > > Tim. >