WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 11/11] xen p2m: clear the old pte when adding a p

To: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 11/11] xen p2m: clear the old pte when adding a page to m2p_override
From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Date: Mon, 10 Jan 2011 10:32:38 +0000
Cc: Fitzhardinge <Jeremy.Fitzhardinge@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Jeremy, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Mon, 10 Jan 2011 02:33:00 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20110105202321.GA25875@xxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <alpine.DEB.2.00.1012151259510.2390@kaball-desktop> <1292420446-3348-11-git-send-email-stefano.stabellini@xxxxxxxxxxxxx> <20110105202321.GA25875@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (DEB 1167 2008-08-23)
On Wed, 5 Jan 2011, Konrad Rzeszutek Wilk wrote:
> On Wed, Dec 15, 2010 at 01:40:46PM +0000, stefano.stabellini@xxxxxxxxxxxxx 
> wrote:
> > From: Jeremy Fitzhardinge <jeremy.fitzhardinge@xxxxxxxxxx>
> > 
> > When adding a page to m2p_override we change the p2m of the page so we
> > need to also clear the old pte of the kernel linear mapping because it
> > doesn't correspond anymore.
> > 
> > When we remove the page from m2p_override we restore the original p2m of
> > the page and we also restore the old pte of the kernel linear mapping.
> > 
> > Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@xxxxxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  arch/x86/xen/p2m.c |   35 ++++++++++++++++++++++++++++++++++-
> >  1 files changed, 34 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> > index 7dde8e4..ca6552d 100644
> > --- a/arch/x86/xen/p2m.c
> > +++ b/arch/x86/xen/p2m.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/module.h>
> >  #include <linux/list.h>
> >  #include <linux/hash.h>
> > +#include <linux/sched.h>
> >  
> >  #include <asm/cache.h>
> >  #include <asm/setup.h>
> > @@ -412,6 +413,22 @@ void m2p_add_override(unsigned long mfn, struct page 
> > *page)
> >     page->index = pfn_to_mfn(pfn);
> >  
> >     __set_phys_to_machine(pfn, FOREIGN_FRAME(mfn));
> > +   if (!PageHighMem(page)) {
> > +           unsigned long address = (unsigned long)__va(pfn << PAGE_SHIFT);
> > +           unsigned level;
> > +           pte_t *ptep = lookup_address(address, &level);
> > +
> > +           if (WARN(ptep == NULL || level != PG_LEVEL_4K,
> > +                                   "m2p_add_override: pfn %lx not mapped", 
> > pfn)) {
> 
> And if we hit this,  then the 'page_to_pfn' must have returned a pretty bogus 
> PFN
> for us to not be able to find the PFN in the &init_mm pagetable.
> Or it truly is at the wrong level.
> > +                   __free_page(page);
> 
> Anyhow, if we are in this trouble, and if you free it here, won't that mess up
> add->pages[x] ? Maybe we should change this function to return an error?

I believe you are correct, in fact I rearranged the code to report an
error in this case before changing the p2m mappings.
Also I removed the __free_page that I don't think makes sense here.

> 
> > +
> > +                   return;
> > +           }
> > +
> > +           /* Just zap old mapping for now */
> > +           pte_clear(&init_mm, address, ptep);
> > +   }
> > +
> >     spin_lock_irqsave(&m2p_override_lock, flags);
> >     list_add(&page->lru,  &m2p_overrides[mfn_hash(mfn)]);
> >     spin_unlock_irqrestore(&m2p_override_lock, flags);
> > @@ -420,10 +437,26 @@ void m2p_add_override(unsigned long mfn, struct page 
> > *page)
> >  void m2p_remove_override(struct page *page)
> >  {
> >     unsigned long flags;
> > +   unsigned long pfn = page_to_pfn(page);
> >     spin_lock_irqsave(&m2p_override_lock, flags);
> >     list_del(&page->lru);
> >     spin_unlock_irqrestore(&m2p_override_lock, flags);
> > -   __set_phys_to_machine(page_to_pfn(page), page->index);
> > +   __set_phys_to_machine(pfn, page->index);
> 
> Ok, so you set the old 'mfn' value back for the pfn.
> > +
> > +   if (!PageHighMem(page)) {
> > +           unsigned long address = (unsigned long)__va(pfn << PAGE_SHIFT);
> > +           unsigned level;
> > +           pte_t *ptep = lookup_address(address, &level);
> > +
> > +           if (WARN(ptep == NULL || level != PG_LEVEL_4K,
> > +                                   "m2p_remove_override: pfn %lx not 
> > mapped", pfn))
> 
> So 'lookup_address' is using the M2P, and you are using the PFN from the
> the page. So the only condition we could in which this ends up being 
> ptep == NULL or a complete bogus value (0x55555555 or 0xfffffff) is if the
> page is still shared or an I/O (so PFN ends up in the PCI BAR space for 
> example)
> page.
> 
> The latter is not going to happen since there are no pages for that region. So
> is it possible that the page is still in M2P as shared? Or is that only 
> possible
> if something has gone horribly wrong in the hypervisor?
> 

I think the only way this can happen is that the page passed as argument
is invalid or wasn't previously added to the m2p_override.
In order to catch this case I have added a check: before modifying the
p2m I am going to read the current mfn of the page and make sure that is
valid and foreign.


> If it has gone wrong, should we reset in the P2M the old MFN?

As in the previous case I think you are correct and I have fixed the
code so that it doesn't modify the p2m if the page passed as argument is
not correct.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>