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

Re: [Xen-devel] [PATCH 1 of 3] p2m: Allow l2 pages to be replaced by sup

At 14:49 +0000 on 19 Jan (1295448547), George Dunlap wrote:
> Just to be clear, should I read your response this as something like below?
> 
> "Please rework this patch to do the following:
> * Generalize for 1GB pages
> * Make the p2m case as careful as the ept case"

Yes, please. 

Tim.

> On Wed, Jan 19, 2011 at 12:22 PM, Tim Deegan <Tim.Deegan@xxxxxxxxxx> wrote:
> > Hi,
> >
> > At 14:36 +0000 on 17 Jan (1295274993), George Dunlap wrote:
> >> # HG changeset patch
> >> # User George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> >> # Date 1295274250 0
> >> # Node ID 366d675630fd6ecbd6228426b3f7723d8a9dd944
> >> # Parent  75b6287626ee0b852d725543568001e99b13be5b
> >> p2m: Allow l2 pages to be replaced by superpages
> >>
> >> Allow second-level p2m pages to be replaced with superpage entries,
> >> freeing the p2m table page properly.  This allows, for example, a
> >> sequence of 512 singleton zero pages to be replaced with a superpage
> >> populate-on-demand entry.
> >
> > This problem became more general under your feet - xen 4.1 supports 1GiB
> > superpages as well, so the same thing needs to be fixed for them.
> > (I understand that PoD only uses 2MiB superpages but to half-fix it
> > invites future bugs.)
> >
> > Also, although in the EPT code you're careful to do all the flushing &c
> > before freeing the old page, in the vanilla p2m code you free the page
> > even before writing the new l2e!
> >
> > Cheers,
> >
> > Tim.
> >
> >> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> >>
> >> diff -r 75b6287626ee -r 366d675630fd xen/arch/x86/mm/hap/hap.c
> >> --- a/xen/arch/x86/mm/hap/hap.c       Fri Jan 14 16:38:51 2011 +0000
> >> +++ b/xen/arch/x86/mm/hap/hap.c       Mon Jan 17 14:24:10 2011 +0000
> >> @@ -333,9 +333,11 @@
> >>
> >>      ASSERT(page_get_owner(pg) == d);
> >>      /* Should have just the one ref we gave it in alloc_p2m_page() */
> >> -    if ( (pg->count_info & PGC_count_mask) != 1 )
> >> -        HAP_ERROR("Odd p2m page count c=%#lx t=%"PRtype_info"\n",
> >> -                  pg->count_info, pg->u.inuse.type_info);
> >> +    if ( (pg->count_info & PGC_count_mask) != 1 ) {
> >> +        HAP_ERROR("Odd p2m page %p count c=%#lx t=%"PRtype_info"\n",
> >> +                     pg, pg->count_info, pg->u.inuse.type_info);
> >> +        WARN();
> >> +    }
> >>      pg->count_info &= ~PGC_count_mask;
> >>      /* Free should not decrement domain's total allocation, since
> >>       * these pages were allocated without an owner. */
> >> diff -r 75b6287626ee -r 366d675630fd xen/arch/x86/mm/hap/p2m-ept.c
> >> --- a/xen/arch/x86/mm/hap/p2m-ept.c   Fri Jan 14 16:38:51 2011 +0000
> >> +++ b/xen/arch/x86/mm/hap/p2m-ept.c   Mon Jan 17 14:24:10 2011 +0000
> >> @@ -317,6 +317,7 @@
> >>      int vtd_pte_present = 0;
> >>      int needs_sync = 1;
> >>      struct domain *d = p2m->domain;
> >> +    struct page_info *intermediate_table = NULL;
> >>
> >>      /*
> >>       * the caller must make sure:
> >> @@ -369,6 +370,12 @@
> >>          /* No need to flush if the old entry wasn't valid */
> >>          if ( !is_epte_present(ept_entry) )
> >>              needs_sync = 0;
> >> +        else if ( order == 9 && ! ept_entry->sp )
> >> +        {
> >> +            /* We're replacing a non-SP page with a superpage.  Make sure 
> >> to
> >> +             * handle freeing the table properly. */
> >> +            intermediate_table = mfn_to_page(ept_entry->mfn);
> >> +        }
> >>
> >>          if ( mfn_valid(mfn_x(mfn)) || direct_mmio || p2m_is_paged(p2mt) ||
> >>               (p2mt == p2m_ram_paging_in_start) )
> >> @@ -487,6 +494,17 @@
> >>          }
> >>      }
> >>
> >> +    if ( intermediate_table )
> >> +    {
> >> +        /* Release the old intermediate table.  This has to be the
> >> +           last thing we do, after the ept_sync_domain() and removal
> >> +           from the iommu tables, so as to avoid a potential
> >> +           use-after-free. */
> >> +
> >> +        page_list_del(intermediate_table, &d->arch.p2m->pages);
> >> +        d->arch.paging.free_page(d, intermediate_table);
> >> +    }
> >> +
> >>      return rv;
> >>  }
> >>
> >> diff -r 75b6287626ee -r 366d675630fd xen/arch/x86/mm/p2m.c
> >> --- a/xen/arch/x86/mm/p2m.c   Fri Jan 14 16:38:51 2011 +0000
> >> +++ b/xen/arch/x86/mm/p2m.c   Mon Jan 17 14:24:10 2011 +0000
> >> @@ -1361,9 +1361,15 @@
> >>          if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
> >>               !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
> >>          {
> >> -            P2M_ERROR("configure P2M table 4KB L2 entry with large 
> >> page\n");
> >> -            domain_crash(p2m->domain);
> >> -            goto out;
> >> +            struct page_info *pg;
> >> +
> >> +            /* We're replacing a non-SP page with a superpage.  Make sure 
> >> to
> >> +             * handle freeing the table properly. */
> >> +            pg = mfn_to_page(_mfn(l1e_get_pfn(*p2m_entry)));
> >> +            paging_write_p2m_entry(p2m->domain, gfn, p2m_entry, table_mfn,
> >> +                                   l1e_empty(), 2);
> >> +            page_list_del(pg, &p2m->pages);
> >> +            p2m->domain->arch.paging.free_page(p2m->domain, pg);
> >>          }
> >>
> >>          ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@xxxxxxxxxxxxxxxxxxx
> >> http://lists.xensource.com/xen-devel
> >
> > --
> > Tim Deegan <Tim.Deegan@xxxxxxxxxx>
> > Principal Software Engineer, Xen Platform Team
> > Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xen-devel
> >

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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