From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages Date: Tue, 27 Aug 2013 14:42:26 +0100 Message-ID: <521CACC2.2060801@citrix.com> References: <1377176248-10113-1-git-send-email-wei.liu2@citrix.com> <52160B55.3000806@citrix.com> <20130826160805.GA29163@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: boris.ostrovsky@oracle.com, Wei Liu , ian.campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 27/08/13 14:34, Stefano Stabellini wrote: > On Mon, 26 Aug 2013, Wei Liu wrote: >> On Thu, Aug 22, 2013 at 02:00:05PM +0100, David Vrabel wrote: >>> On 22/08/13 13:57, Wei Liu wrote: >>>> In commit cd9151e2: xen/balloon: set a mapping for ballooned out pages >>>> we have the ballooned out page's mapping set to a scratch page. That commit >>>> also set the P2M entry of ballooned out page to the scratch, which is >>>> not necessary. That triggers BUG_ONs in __set_phys_to_machine when the >>>> page is ballooned in again. >>>> >>>> We only need to ensure that the ballooned out pages have valid mapping. >>>> P2M entries of those pages should still be set to INVALID_P2M_ENTRY. >>> >>> I know I suggested this but I would like to get Stefano's acked-by or >>> reviewed-by for this. >>> >> >> Stefano, what do you think? > > I think it is wrong: if the page has a valid mapping it should also have > a valid p2m entry. It's easier to think about and it's going to avoid > bugs when generic linux code ends up calling a pvop that on Xen looks > into the p2m. But in the auto_translated physmap case it does /not/ have a valid p2m entry -- only the virtual mappping.... > Which one of the BUG_ON in __set_phys_to_machine are hit? We could > avoid calling set_phys_to_machine in decrease_reservation on > XENFEAT_auto_translated_physmap guests, that would avoid the first > BUG_ON in __set_phys_to_machine. Arguably it's the right thing to do > anyway. ... which makes the suggestion to avoid the __set_phys_to_machine() calls sensible to me. > Regarding the second BUG_ON: > > if (unlikely(pfn >= MAX_P2M_PFN)) { > BUG_ON(mfn != INVALID_P2M_ENTRY); > return true; > } > > We shouldn't really be hitting it, right? Why is pfn >= MAX_P2M_PFN? I believe it was the check for pfn == mfn. David