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"
-George
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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home