Re: [PATCH]: fix vram tracking (was Re: [Xen-devel] xen assert in latest 3.3.2-rc bits)

[Gianluca Guida <gianluca.guida@xxxxxxxxxxxxx>]

Gianluca Guida wrote:
> Wops, just realized that the patch I've sent it's against 
> xen-unstable.hg; should be easy to backport, but in case I'll send you 
> the correct one ASAP.
Here's the patch for xen-3.3-testing.hg.

Gianluca

diff -r 587e81dd3540 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c    Mon Mar 02 14:19:35 2009 +0000
+++ b/xen/arch/x86/mm/shadow/multi.c    Wed Mar 11 01:30:55 2009 +0000
@@ -1320,23 +1320,23 @@ static inline void shadow_vram_get_l1e(s
                                        mfn_t sl1mfn,
                                        struct domain *d)
 { 
-    mfn_t mfn;
+    mfn_t mfn = shadow_l1e_get_mfn(new_sl1e);
+    int flags = shadow_l1e_get_flags(new_sl1e);
     unsigned long gfn;
 
-    if ( !d->dirty_vram ) return;
-
-    mfn = shadow_l1e_get_mfn(new_sl1e);
-
-    if ( !mfn_valid(mfn) ) return; /* m2p for mmio_direct may not exist */
+    if ( !d->dirty_vram         /* tracking disabled? */
+         || !(flags & _PAGE_RW) /* read-only mapping? */
+         || !mfn_valid(mfn) )   /* mfn can be invalid in mmio_direct */
+        return;
 
     gfn = mfn_to_gfn(d, mfn);
 
-    if ( (gfn >= d->dirty_vram->begin_pfn) && (gfn < d->dirty_vram->end_pfn) ) 
{
+    if ( (gfn >= d->dirty_vram->begin_pfn) && (gfn < d->dirty_vram->end_pfn) )
+    {
         unsigned long i = gfn - d->dirty_vram->begin_pfn;
         struct page_info *page = mfn_to_page(mfn);
-        u32 count_info = page->u.inuse.type_info & PGT_count_mask;
-        
-        if ( count_info == 1 )
+        
+        if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
             /* Initial guest reference, record it */
             d->dirty_vram->sl1ma[i] = pfn_to_paddr(mfn_x(sl1mfn))
                 | ((unsigned long)sl1e & ~PAGE_MASK);
@@ -1348,49 +1348,59 @@ static inline void shadow_vram_put_l1e(s
                                        mfn_t sl1mfn,
                                        struct domain *d)
 {
-    mfn_t mfn;
+    mfn_t mfn = shadow_l1e_get_mfn(old_sl1e);
+    int flags = shadow_l1e_get_flags(old_sl1e);
     unsigned long gfn;
 
-    if ( !d->dirty_vram ) return;
-
-    mfn = shadow_l1e_get_mfn(old_sl1e);
-
-    if ( !mfn_valid(mfn) ) return;
+    if ( !d->dirty_vram         /* tracking disabled? */
+         || !(flags & _PAGE_RW) /* read-only mapping? */
+         || !mfn_valid(mfn) )   /* mfn can be invalid in mmio_direct */
+        return;
 
     gfn = mfn_to_gfn(d, mfn);
 
-    if ( (gfn >= d->dirty_vram->begin_pfn) && (gfn < d->dirty_vram->end_pfn) ) 
{
+    if ( (gfn >= d->dirty_vram->begin_pfn) && (gfn < d->dirty_vram->end_pfn) )
+    {
         unsigned long i = gfn - d->dirty_vram->begin_pfn;
         struct page_info *page = mfn_to_page(mfn);
-        u32 count_info = page->u.inuse.type_info & PGT_count_mask;
         int dirty = 0;
         paddr_t sl1ma = pfn_to_paddr(mfn_x(sl1mfn))
             | ((unsigned long)sl1e & ~PAGE_MASK);
 
-        if ( count_info == 1 ) {
+        if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
+        {
             /* Last reference */
-            if ( d->dirty_vram->sl1ma[i] == INVALID_PADDR ) {
+            if ( d->dirty_vram->sl1ma[i] == INVALID_PADDR )
+            {
                 /* We didn't know it was that one, let's say it is dirty */
                 dirty = 1;
-            } else {
+            }
+            else
+            {
                 ASSERT(d->dirty_vram->sl1ma[i] == sl1ma);
                 d->dirty_vram->sl1ma[i] = INVALID_PADDR;
-                if ( shadow_l1e_get_flags(old_sl1e) & _PAGE_DIRTY )
+                if ( flags & _PAGE_DIRTY )
                     dirty = 1;
             }
-        } else {
+        }
+        else
+        {
             /* We had more than one reference, just consider the page dirty. */
             dirty = 1;
             /* Check that it's not the one we recorded. */
-            if ( d->dirty_vram->sl1ma[i] == sl1ma ) {
+            if ( d->dirty_vram->sl1ma[i] == sl1ma )
+            {
                 /* Too bad, we remembered the wrong one... */
                 d->dirty_vram->sl1ma[i] = INVALID_PADDR;
-            } else {
+            }
+            else
+            {
                 /* Ok, our recorded sl1e is still pointing to this page, let's
                  * just hope it will remain. */
             }
         }
-        if ( dirty ) {
+        if ( dirty )
+        {
             d->dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
             d->dirty_vram->last_dirty = NOW();
         }

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