From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Andres Lagar-Cavilla" Subject: Re: [PATCH 2 of 2] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap Date: Wed, 25 Apr 2012 08:20:13 -0700 Message-ID: <1eca79e04d540f6c44b72e865c19c3b9.squirrel@webmail.lagarcavilla.org> References: <51646b89b1822fe74ffb.1335295219@xdev.gridcentric.ca> <20120425124147.GD51354@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: <20120425124147.GD51354@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, Andres Lagar-Cavilla , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org > At 15:20 -0400 on 24 Apr (1335280819), Andres Lagar-Cavilla wrote: >> xen/arch/x86/mm/mem_sharing.c | 7 ++++--- >> xen/include/asm-x86/p2m.h | 8 ++++++++ >> 2 files changed, 12 insertions(+), 3 deletions(-) >> >> >> Signed-off-by: Andres Lagar-Cavilla >> >> diff -r 5be9a05f17fd -r 51646b89b182 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,10 @@ 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 */ >> - if ( mfn_valid(cmfn) || >> - (!(p2m_is_ram(cmfn_type))) ) >> + /* Make sure the target page is a hole in the physmap. These are >> typically >> + * p2m_mmio_dm, but also accept p2m_invalid and paged out pages. >> See the >> + * definition of p2m_is_hole in p2m.h. */ >> + if ( !p2m_is_hole(cmfn_type) || mfn_valid(cmfn) ) > > Hmm. Is the mfn_valid() to handle p2m_ram_paging_in entries sometimes > having an MFN and sometimes not? I think it would be nicer to either > always replace paging-in pages or never do it. In any case, it's bogus > to test mfn_valid() for any of the other types. Yes, that is the concern. paging_in entries may have a backing mfn. I am not opposed to just replacing the entry if it is in paging_in state, regardless of the mfn. It would be similar to any of the cases in which guest_physmap_add_entry is used with a valid mfn in the entry to be updated. Andres > > Cheers, > > Tim. >