Re: [Xen-devel] [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 27 Aug 2013, Wei Liu wrote:
> On Tue, Aug 27, 2013 at 02:34:42PM +0100, 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.
> > 
> > Which one of the BUG_ON in __set_phys_to_machine are hit?  We could
> 
> Both.
> 
> > 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.
> > 
> 
> OK.
> 
> > 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 think I did something silly, but I could only vaguely remember that I
> tried to play with very high virtual address, which could lead to pfn >=
> MAX_P2M_PFN.
> 
> AIUI we can definitively hit this case otherwise we would just have
> 
>  BUG_ON(pfn >= MAX_P2M_PFN)
> 
> right?

I think that the idea is that if you try to set the p2m for a pfn >=
MAX_P2M_PFN then the mfn has to be invalid, otherwise it would be an
existing and correct p2m mapping but in that case pfn would not be >=
MAX_P2M_PFN.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel