From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. Date: Wed, 08 Jan 2014 08:48:46 -0500 Message-ID: <52CD573E.1050506@terremark.com> References: <1389140748-26524-1-git-send-email-dslutz@verizon.com> <1389140748-26524-3-git-send-email-dslutz@verizon.com> <52CD1C1E02000078001116BC@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------050102010801030001030404" Return-path: In-Reply-To: <52CD1C1E02000078001116BC@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Don Slutz Cc: Keir Fraser , Ian Campbell , Stefano Stabellini , George Dunlap , Ian Jackson , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --------------050102010801030001030404 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit On 01/08/14 03:36, Jan Beulich wrote: >>>> On 08.01.14 at 01:25, Don Slutz 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 " 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; >> } > --------------050102010801030001030404 Content-Type: text/x-patch; name="0001-dbg_rw_guest_mem-need-to-call-put_gfn-in-error-path.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-dbg_rw_guest_mem-need-to-call-put_gfn-in-error-path.pat"; filename*1="ch" >>From 1c8ebc3cbc4f415c5942b32736ecb58ef9c2cc14 Mon Sep 17 00:00:00 2001 From: Andrew Cooper 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 = {} }, unlock_level = 0, recurse_count = 1, locker = 1, locker_function = 0xffff82c4c022c640 <__func__.13514> "__get_gfn_type_access" } Signed-off-by: Andrew Cooper Signed-off-by: Don Slutz Tested-by: Don Slutz --- 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 --------------050102010801030001030404 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------050102010801030001030404--