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

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

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