From mboxrd@z Thu Jan  1 00:00:00 1970
From: Don Slutz <dslutz@verizon.com>
Subject: Re: [BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call
 put_gfn in error path.
Date: Mon, 06 Jan 2014 07:43:47 -0500
Message-ID: <52CAA503.5080707@terremark.com>
References: <1388857936-664-1-git-send-email-dslutz@verizon.com>
	<1388857936-664-2-git-send-email-dslutz@verizon.com>
	<52C9E621.4020608@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <52C9E621.4020608@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, George Dunlap <george.dunlap@eu.citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Don Slutz <dslutz@verizon.com>, xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>
List-Id: xen-devel@lists.xenproject.org

On 01/05/14 18:09, Andrew Cooper wrote:
> 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 = {<No data fields>}
>>    },
>>    unlock_level = 0,
>>    recurse_count = 1,
>>    locker = 1,
>>    locker_function = 0xffff82c4c022c640 <__func__.13514> "__get_gfn_type_access"
>> }
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   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.

Will fix.


> 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.

That would only fix "p2m_is_readonly(gfntype) and writing memory" case 
(see cover letter). To fix "the requested vaddr does not exist" case, I 
would need to add a check for INVALID_MFN in dbg_hvm_va2mfn() also.  As 
It currently stands it is a smaller fix.

    -Don Slutz


> ~Andrew
>
>> +            if ( gfn != INVALID_GFN )
>> +                put_gfn(dp, gfn);
>>               break;
>> +        }
>>   
>>           va = map_domain_page(mfn);
>>           va = va + (addr & (PAGE_SIZE-1));