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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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