[PATCH 0 of 4] x86/mm: Four fixes

[PATCH 0 of 4] x86/mm: Four fixes

[Andres Lagar-Cavilla]

In this series we post four patches with bugfixes to p2m, hap, paging and
sharing code:

- Make a few asserts on shared pages type and counts more accurate
 (this time done right, hopefully!)
- Bugfix interactions between the balloon and the paging and sharing
 subsystems. Posted a week ago, probably slipped through the cracks.
- Added a missing sanity check for sharing/paging/access memops.
- Fix vmx_load_pdptrs to crash the guest instead of the host in the
 presence of paged out cr3's.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

 xen/arch/x86/mm/mem_sharing.c |   3 ++-
 xen/arch/x86/mm/p2m.c         |   7 +++++--
 xen/arch/x86/mm/p2m.c         |   7 +++++--
 xen/common/memory.c           |  17 ++++++++++++++++-
 xen/arch/x86/mm/mem_access.c  |   3 +++
 xen/arch/x86/mm/mem_event.c   |   6 ++++--
 xen/arch/x86/mm/mem_paging.c  |   3 +++
 xen/arch/x86/mm/mem_sharing.c |   2 +-
 xen/arch/x86/hvm/vmx/vmx.c    |  16 +++++++++++++---
 xen/arch/x86/mm/hap/hap.c     |   2 +-
 10 files changed, 53 insertions(+), 13 deletions(-)

[PATCH 1 of 4] x86/mm: Make asserts on types and counts of shared pages more accurate

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/mem_sharing.c |  3 ++-
 xen/arch/x86/mm/p2m.c         |  7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 658530e2f380 -r a70a87d7bf84 xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -201,7 +201,8 @@ static struct page_info* mem_sharing_loo
             /* Count has to be at least two, because we're called
              * with the mfn locked (1) and this is supposed to be 
              * a shared page (1). */
-            ASSERT((page->u.inuse.type_info & PGT_count_mask) >= 2); 
+            unsigned long type = read_atomic(&page->u.inuse.type_info);
+            ASSERT((type & PGT_shared_page) && ((type & PGT_count_mask) >= 2));
             ASSERT(get_gpfn_from_mfn(mfn) == SHARED_M2P_ENTRY); 
             return page;
         }
diff -r 658530e2f380 -r a70a87d7bf84 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -735,6 +735,7 @@ set_shared_p2m_entry(struct domain *d, u
     p2m_access_t a;
     p2m_type_t ot;
     mfn_t omfn;
+    unsigned long pg_type;
 
     if ( !paging_mode_translate(p2m->domain) )
         return 0;
@@ -745,8 +746,10 @@ set_shared_p2m_entry(struct domain *d, u
      * sharable first */
     ASSERT(p2m_is_shared(ot));
     ASSERT(mfn_valid(omfn));
-    if ( ((mfn_to_page(omfn)->u.inuse.type_info & PGT_type_mask) 
-                    != PGT_shared_page) )
+    /* Set the m2p entry to invalid only if there are no further type
+     * refs to this page as shared */
+    pg_type = read_atomic(&(mfn_to_page(omfn)->u.inuse.type_info));
+    if ( !((pg_type & PGT_shared_page) && (pg_type & PGT_count_mask)) )
         set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
 
     P2M_DEBUG("set shared %lx %lx\n", gfn, mfn_x(mfn));

[PATCH 2 of 4] x86/mm: Fix more ballooning+paging and ballooning+sharing bugs

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/p2m.c |   7 +++++--
 xen/common/memory.c   |  17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 3 deletions(-)


If the guest balloons away a page that has been nominated for paging but not yet
paged out, we fix:
 - Send EVICT_FAIL flag in the event to the pager
 - Do not leak the underlying page

If the page was shared, we were not:
 - properly refreshing the mfn to balloon after the unshare.
 - unlocking the p2m on the error exit case

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r a70a87d7bf84 -r b03a10be1428 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -928,11 +928,14 @@ void p2m_mem_paging_drop_page(struct dom
     req.gfn = gfn;
     req.flags = MEM_EVENT_FLAG_DROP_PAGE;
 
-    mem_event_put_request(d, &d->mem_event->paging, &req);
-
     /* Update stats unless the page hasn't yet been evicted */
     if ( p2mt != p2m_ram_paging_out )
         atomic_dec(&d->paged_pages);
+    else
+        /* Evict will fail now, tag this request for pager */
+        req.flags |= MEM_EVENT_FLAG_EVICT_FAIL;
+
+    mem_event_put_request(d, &d->mem_event->paging, &req);
 }
 
 /**
diff -r a70a87d7bf84 -r b03a10be1428 xen/common/memory.c
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -167,6 +167,15 @@ int guest_remove_page(struct domain *d, 
     {
         guest_physmap_remove_page(d, gmfn, mfn, 0);
         put_gfn(d, gmfn);
+        /* If the page hasn't yet been paged out, there is an
+         * actual page that needs to be released. */
+        if ( p2mt == p2m_ram_paging_out )
+        {
+            ASSERT(mfn_valid(mfn));
+            page = mfn_to_page(mfn);
+            if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
+                put_page(page);
+        }
         p2m_mem_paging_drop_page(d, gmfn, p2mt);
         return 1;
     }
@@ -181,7 +190,6 @@ int guest_remove_page(struct domain *d, 
         return 0;
     }
             
-    page = mfn_to_page(mfn);
 #ifdef CONFIG_X86_64
     if ( p2m_is_shared(p2mt) )
     {
@@ -190,10 +198,17 @@ int guest_remove_page(struct domain *d, 
          * need to trigger proper cleanup. Once done, this is 
          * like any other page. */
         if ( mem_sharing_unshare_page(d, gmfn, 0) )
+        {
+            put_gfn(d, gmfn);
             return 0;
+        }
+        /* Maybe the mfn changed */
+        mfn = mfn_x(get_gfn_query_unlocked(d, gmfn, &p2mt));
+        ASSERT(!p2m_is_shared(p2mt));
     }
 #endif /* CONFIG_X86_64 */
 
+    page = mfn_to_page(mfn);
     if ( unlikely(!get_page(page, d)) )
     {
         put_gfn(d, gmfn);

[PATCH 3 of 4] x86/mm: Check sharing/paging/access have been enabled before processing a memop

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/mem_access.c  |  3 +++
 xen/arch/x86/mm/mem_event.c   |  6 ++++--
 xen/arch/x86/mm/mem_paging.c  |  3 +++
 xen/arch/x86/mm/mem_sharing.c |  2 +-
 4 files changed, 11 insertions(+), 3 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r b03a10be1428 -r 7a1d415a71d0 xen/arch/x86/mm/mem_access.c
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -29,6 +29,9 @@ int mem_access_memop(struct domain *d, x
 {
     int rc;
 
+    if ( unlikely(!d->mem_event->access.ring_page) )
+        return -ENODEV;
+
     switch( meo->op )
     {
     case XENMEM_access_op_resume:
diff -r b03a10be1428 -r 7a1d415a71d0 xen/arch/x86/mm/mem_event.c
--- a/xen/arch/x86/mm/mem_event.c
+++ b/xen/arch/x86/mm/mem_event.c
@@ -452,13 +452,15 @@ int mem_event_claim_slot(struct domain *
 /* Registered with Xen-bound event channel for incoming notifications. */
 static void mem_paging_notification(struct vcpu *v, unsigned int port)
 {
-    p2m_mem_paging_resume(v->domain);
+    if ( likely(v->domain->mem_event->paging.ring_page != NULL) )
+        p2m_mem_paging_resume(v->domain);
 }
 
 /* Registered with Xen-bound event channel for incoming notifications. */
 static void mem_access_notification(struct vcpu *v, unsigned int port)
 {
-    p2m_mem_access_resume(v->domain);
+    if ( likely(v->domain->mem_event->access.ring_page != NULL) )
+        p2m_mem_access_resume(v->domain);
 }
 
 struct domain *get_mem_event_op_target(uint32_t domain, int *rc)
diff -r b03a10be1428 -r 7a1d415a71d0 xen/arch/x86/mm/mem_paging.c
--- a/xen/arch/x86/mm/mem_paging.c
+++ b/xen/arch/x86/mm/mem_paging.c
@@ -27,6 +27,9 @@
 
 int mem_paging_memop(struct domain *d, xen_mem_event_op_t *mec)
 {
+    if ( unlikely(!d->mem_event->paging.ring_page) )
+        return -ENODEV;
+
     switch( mec->op )
     {
     case XENMEM_paging_op_nominate:
diff -r b03a10be1428 -r 7a1d415a71d0 xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1021,7 +1021,7 @@ int mem_sharing_memop(struct domain *d, 
     int rc = 0;
 
     /* Only HAP is supported */
-    if ( !hap_enabled(d) )
+    if ( !hap_enabled(d) || !d->arch.hvm_domain.mem_sharing_enabled )
          return -ENODEV;
 
     switch(mec->op)

[PATCH 4 of 4] x86/mm: Fix two PAE+paging bugs

[Andres Lagar-Cavilla]

 xen/arch/x86/hvm/vmx/vmx.c |  16 +++++++++++++---
 xen/arch/x86/mm/hap/hap.c  |   2 +-
 2 files changed, 14 insertions(+), 4 deletions(-)


In hap_paging_update_modes, we were getting the gpa of the cr3, rather than the
gfn.

Vmx_load_pdptrs was crashing the host if the cr3 is paged out. Now it will only
crash the guest.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 7a1d415a71d0 -r 62b1fe67b8d1 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1010,12 +1010,22 @@ static void vmx_load_pdptrs(struct vcpu 
     if ( (cr3 & 0x1fUL) && !hvm_pcid_enabled(v) )
         goto crash;
 
-    mfn = mfn_x(get_gfn(v->domain, cr3 >> PAGE_SHIFT, &p2mt));
-    if ( !p2m_is_ram(p2mt) )
+    mfn = mfn_x(get_gfn_unshare(v->domain, cr3 >> PAGE_SHIFT, &p2mt));
+    if ( !p2m_is_ram(p2mt) || !mfn_valid(mfn) || 
+         /* If we didn't succeed in unsharing, get_page will fail
+          * (page still belongs to dom_cow) */
+         !get_page(mfn_to_page(mfn), v->domain) )
     {
+        /* Ideally you don't want to crash but rather go into a wait 
+         * queue, but this is the wrong place. We're holding at least
+         * the paging lock */
+        gdprintk(XENLOG_ERR,
+                    "Bad cr3 on load pdptrs gfn %"PRIx64" mfn %"PRIx64
+                    " type %d\n",cr3 >> PAGE_SHIFT, mfn, (int)p2mt);
         put_gfn(v->domain, cr3 >> PAGE_SHIFT);
         goto crash;
     }
+    put_gfn(v->domain, cr3 >> PAGE_SHIFT);
 
     p = map_domain_page(mfn);
 
@@ -1043,7 +1053,7 @@ static void vmx_load_pdptrs(struct vcpu 
     vmx_vmcs_exit(v);
 
     unmap_domain_page(p);
-    put_gfn(v->domain, cr3 >> PAGE_SHIFT);
+    put_page(mfn_to_page(mfn));
     return;
 
  crash:
diff -r 7a1d415a71d0 -r 62b1fe67b8d1 xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -786,7 +786,7 @@ hap_paging_get_mode(struct vcpu *v)
 static void hap_update_paging_modes(struct vcpu *v)
 {
     struct domain *d = v->domain;
-    unsigned long cr3_gfn = v->arch.hvm_vcpu.guest_cr[3];
+    unsigned long cr3_gfn = v->arch.hvm_vcpu.guest_cr[3] >> PAGE_SHIFT;
     p2m_type_t t;
 
     /* We hold onto the cr3 as it may be modified later, and

Re: [PATCH 0 of 4] x86/mm: Four fixes

[Tim Deegan]

At 22:42 -0500 on 15 Feb (1329345744), Andres Lagar-Cavilla wrote:
> In this series we post four patches with bugfixes to p2m, hap, paging and
> sharing code:
> 
> - Make a few asserts on shared pages type and counts more accurate
>  (this time done right, hopefully!)

Nope, sorry! :)  I fixed the tests up and applied it; please check that
it still does what you wanted.

> - Bugfix interactions between the balloon and the paging and sharing
>  subsystems. Posted a week ago, probably slipped through the cracks.
> - Added a missing sanity check for sharing/paging/access memops.
> - Fix vmx_load_pdptrs to crash the guest instead of the host in the
>  presence of paged out cr3's.

This needed a minor adjustment to a printk format for 32-bit builds. 

> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

All applied, thanks.

Cheers,

Tim.

Re: [PATCH 0 of 4] x86/mm: Four fixes

[Andres Lagar-Cavilla]

> At 22:42 -0500 on 15 Feb (1329345744), Andres Lagar-Cavilla wrote:
>> In this series we post four patches with bugfixes to p2m, hap, paging
>> and
>> sharing code:
>>
>> - Make a few asserts on shared pages type and counts more accurate
>>  (this time done right, hopefully!)
>
> Nope, sorry! :)  I fixed the tests up and applied it; please check that
> it still does what you wanted.
Check. It wasn't that bad to begin with :)
Thanks
Andres
>
>> - Bugfix interactions between the balloon and the paging and sharing
>>  subsystems. Posted a week ago, probably slipped through the cracks.
>> - Added a missing sanity check for sharing/paging/access memops.
>> - Fix vmx_load_pdptrs to crash the guest instead of the host in the
>>  presence of paged out cr3's.
>
> This needed a minor adjustment to a printk format for 32-bit builds.
>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> All applied, thanks.
>
> Cheers,
>
> Tim.
>