From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call put_gfn in error path. Date: Sun, 5 Jan 2014 23:09:21 +0000 Message-ID: <52C9E621.4020608@citrix.com> References: <1388857936-664-1-git-send-email-dslutz@verizon.com> <1388857936-664-2-git-send-email-dslutz@verizon.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1388857936-664-2-git-send-email-dslutz@verizon.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: Don Slutz , xen-devel@lists.xen.org Cc: Keir Fraser , Ian Campbell , Stefano Stabellini , George Dunlap , Ian Jackson , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 04/01/2014 17:52, 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 > --- > xen/arch/x86/debug.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c > index 3e21ca8..eceb805 100644 > --- a/xen/arch/x86/debug.c > +++ b/xen/arch/x86/debug.c > @@ -161,8 +161,11 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len, struct domain *dp, > mfn = (has_hvm_container_domain(dp) > ? dbg_hvm_va2mfn(addr, dp, toaddr, &gfn) > : dbg_pv_va2mfn(addr, dp, pgd3)); > - if ( mfn == INVALID_MFN ) > + if ( mfn == INVALID_MFN ) { Xen coding style would have this brace on the next line. Content-wise, I think it would be better to fix up the error path in dbg_hvm_va2mfn() so it doesn't exit with INVALID_MFN having taken a reference on the gfn. ~Andrew > + if ( gfn != INVALID_GFN ) > + put_gfn(dp, gfn); > break; > + } > > va = map_domain_page(mfn); > va = va + (addr & (PAGE_SIZE-1));