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: Tue, 07 Jan 2014 20:06:30 -0500 Message-ID: <52CCA496.80008@terremark.com> References: <1389140748-26524-1-git-send-email-dslutz@verizon.com> <1389140748-26524-3-git-send-email-dslutz@verizon.com> <52CCA204.2020601@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52CCA204.2020601@citrix.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: Andrew Cooper , Don Slutz , xen-devel@lists.xen.org Cc: Keir Fraser , Ian Campbell , Stefano Stabellini , George Dunlap , Tim Deegan , Ian Jackson , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 01/07/14 19:55, Andrew Cooper wrote: > On 08/01/2014 00:25, Don Slutz wrote: >> 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: Don Slutz > Technically this should include by Signed-off-by: Andrew Cooper > tag (being the author of the code) as well > as your own (being the discoverer of the bug and author of the commit > message), but I notice I accidentally omitted it from the original email > thread, so my apologies. I was not sure if I should have added it without you adding it... So I went without. > It should probably also include your Tested-by: tag That is fine with me. Should I make a v3 of just this with both tags? -Don Slutz > > Ian (with RM hat on): > This is a hypervisor reference counting error on a toolstack hypercall > path. Irrespective of any of the other patches in this series, I think > this should be included ASAP (although probably subject to review from a > third person), which will fix the hypervisor crashes from gdbsx usage. > > ~Andrew > >> --- >> xen/arch/x86/debug.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c >> index a67a192..ba6a64d 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; >> } >> >> 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; >> + } >> + >> return mfn; >> } >>