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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/01/2014 01:06, Don Slutz wrote:
> 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 = {<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>
>> Technically this should include by Signed-off-by: Andrew Cooper
>> <andrew.cooper3@citrix.com> 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.

That is fair enough - it was my mistake to start with so no worries.

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

Depends whether the committers are happy accumulating tags and whether
there is further comment/changes required for the patch.

As a rule of thumb, I would say "no for now" with a "accumulate if a new
v3 is needed" or "a committer asks you to accumulate". 

~Andrew