Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path.

[Don Slutz <dslutz@verizon.com>]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

On 01/08/14 03:36, Jan Beulich wrote:
>>>> On 08.01.14 at 01:25, Don Slutz <dslutz@verizon.com> wrote:
>> --- a/xen/arch/x86/debug.c
>> +++ b/xen/arch/x86/debug.c
>> @@ -63,10 +63,17 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
>>       if ( p2m_is_readonly(gfntype) && toaddr )
>>       {
>>           DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
>> -        return INVALID_MFN;
>> +        mfn = INVALID_MFN;
>>       }
>>   
>>       DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
> With the flow change above, this should be moved into an "else"
> to the earlier "if".

Ok.  I have done this and tested it.  (v3 attached).

Note: this means that patch v2 3/5 will not cleanly apply.  I am in the process of fixing and testing v3 of that.  Andrew Cooper in

http://lists.xen.org/archives/html/xen-devel/2014-01/msg00595.html

Says that this one patch "should be included ASAP"; which is why I am sending this 1st.

I did not add:

Acked-by: Mukesh Rathor ...

Even though my understanding is that I could have (please let me know if this is wrong) based on:

http://lists.xen.org/archives/html/xen-devel/2014-01/msg00601.html

since the v3 change is very minor (like a comment change).  This is because DBGP2:

#define DBGP2(...) ((void)0)

does nothing.  And if changed to do something, the file fails to compile without more changes (I.E. patch 3/5).  Even though I knew this does nothing I has compiled and run this code.


I did change the author to "Andrew Cooper <andrew.cooper3@citrix.com>" since he provided most of this change. (see

http://lists.xen.org/archives/html/xen-devel/2014-01/msg00631.html

)  Not sure that is correct since we have now both made changes.

    -Don Slutz



> Jan
>
>> +
>> +    if ( mfn == INVALID_MFN )
>> +    {
>> +        put_gfn(dp, *gfn);
>> +        *gfn = INVALID_GFN;
>> +    }
>> +
>>       return mfn;
>>   }
>


[-- Attachment #2: 0001-dbg_rw_guest_mem-need-to-call-put_gfn-in-error-path.patch --]
[-- Type: text/x-patch, Size: 3069 bytes --]

>From 1c8ebc3cbc4f415c5942b32736ecb58ef9c2cc14 Mon Sep 17 00:00:00 2001
From: Andrew Cooper <andrew.cooper3@citrix.com>
Date: Fri, 3 Jan 2014 16:12:20 -0500
Subject: [BUGFIX][PATCH v3] dbg_rw_guest_mem: need to call put_gfn in error
 path.

Using a 1G hvm domU (in grub) and gdbsx:

(gdb) set arch i8086
warning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB.  Attempting to continue with the default i8086 settings.

The target architecture is assumed to be i8086
(gdb) target remote localhost:9999
Remote debugging using localhost:9999
Remote debugging from host 127.0.0.1
0x0000d475 in ?? ()
(gdb) x/1xh 0x6ae9168b

Will reproduce this bug.

With a debug=y build you will get:

Assertion '!preempt_count()' failed at preempt.c:37

For a debug=n build you will get a dom0 VCPU hung (at some point) in:

         [ffff82c4c0126eec] _write_lock+0x3c/0x50
          ffff82c4c01e43a0  __get_gfn_type_access+0x150/0x230
          ffff82c4c0158885  dbg_rw_mem+0x115/0x360
          ffff82c4c0158fc8  arch_do_domctl+0x4b8/0x22f0
          ffff82c4c01709ed  get_page+0x2d/0x100
          ffff82c4c01031aa  do_domctl+0x2ba/0x11e0
          ffff82c4c0179662  do_mmuext_op+0x8d2/0x1b20
          ffff82c4c0183598  __update_vcpu_system_time+0x288/0x340
          ffff82c4c015c719  continue_nonidle_domain+0x9/0x30
          ffff82c4c012938b  add_entry+0x4b/0xb0
          ffff82c4c02223f9  syscall_enter+0xa9/0xae

And gdb output:

(gdb) x/1xh 0x6ae9168b
0x6ae9168b:     0x3024
(gdb) x/1xh 0x6ae9168b
0x6ae9168b:     Ignoring packet error, continuing...
Reply contains invalid hex digit 116

The 1st one worked because the p2m.lock is recursive and the PCPU
had not yet changed.

crash reports (for example):

crash> mm_rwlock_t 0xffff83083f913010
struct mm_rwlock_t {
  lock = {
    raw = {
      lock = 2147483647
    },
    debug = {<No data fields>}
  },
  unlock_level = 0,
  recurse_count = 1,
  locker = 1,
  locker_function = 0xffff82c4c022c640 <__func__.13514> "__get_gfn_type_access"
}

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Don Slutz <dslutz@verizon.com>
Tested-by: Don Slutz <dslutz@verizon.com>
---

Changes v2 to v3:
  Jan Beulich: Fix flow change (added an else to DBGP2).
  Ian Campbell: Fix author  

 xen/arch/x86/debug.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index a67a192..435bd40 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -63,10 +63,17 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
     if ( p2m_is_readonly(gfntype) && toaddr )
     {
         DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
-        return INVALID_MFN;
+        mfn = INVALID_MFN;
+    }
+    else
+        DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
+
+    if ( mfn == INVALID_MFN )
+    {
+        put_gfn(dp, *gfn);
+        *gfn = INVALID_GFN;
     }
 
-    DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
     return mfn;
 }
 
-- 
1.8.4


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel