[BUGFIX][PATCH 0/4] gdbsx: fix 3 bugs

[BUGFIX][PATCH 0/4] gdbsx: fix 3 bugs

[Don Slutz]

Release manager requests:
  patch 1 should be in 4.4.0
  patch 3 and 4 would be good to be in 4.4.0
  patch 2 is optional.

While tracking down a bug in seabios/grub I found the bug in patch
1.

There are 2 ways that gfn will not be INVALID_GFN and yet mfn will
be INVALID_MFN.

  1) p2m_is_readonly(gfntype) and writing memory.
  2) the requested vaddr does not exist.

This may only be an issue for a HVM guest that is in real mode
(I.E. no page tables).

Patch 2 is debug logging that was used to find the 2nd way.

Patch 3 and 4 are more of a cleanup bug fix.

Don Slutz (4):
  dbg_rw_guest_mem: need to call put_gfn in error path.
  dbg_rw_guest_mem: Enable debug log output
  xg_read_mem: Report on error.
  XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

 tools/debugger/gdbsx/xg/xg_main.c |  6 ++++--
 xen/arch/x86/debug.c              | 44 ++++++++++++++++++++++++++++++---------
 xen/arch/x86/domctl.c             |  3 +--
 3 files changed, 39 insertions(+), 14 deletions(-)

-- 
1.8.4

[BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call put_gfn in error path.

[Don Slutz]

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 ) {
+            if ( gfn != INVALID_GFN )
+                put_gfn(dp, gfn);
             break;
+        }
 
         va = map_domain_page(mfn);
         va = va + (addr & (PAGE_SIZE-1));
-- 
1.8.4

Re: [BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call put_gfn in error path.

[Andrew Cooper]

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.

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

Re: [BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call put_gfn in error path.

[Don Slutz]

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

Re: [BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call put_gfn in error path.

[Andrew Cooper]

On 06/01/14 12:43, Don Slutz wrote:
> 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

Size really doesn't matter.

What does matter is that the caller of dbg_hvm_va2mfn() should not have
to cleanup a reference taken when it returns an error.

Anyway, "the requested vaddr does not exist" is covered by
paging_gva_to_gfn(), which will exit before taking the ref on the gfn.

The following (completely untested) patch ought to do:

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 3e21ca8..3372adb 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;
 }

Re: [BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call put_gfn in error path.

[Don Slutz]

On 01/06/14 08:13, Andrew Cooper wrote:
> On 06/01/14 12:43, Don Slutz wrote:
>> 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
> Size really doesn't matter.
>
> What does matter is that the caller of dbg_hvm_va2mfn() should not have
> to cleanup a reference taken when it returns an error.
>
> Anyway, "the requested vaddr does not exist" is covered by
> paging_gva_to_gfn(), which will exit before taking the ref on the gfn.

This is a false statement.  A valid GFN is returned in this case. The lock is taken by get_gfn().

>
> The following (completely untested) patch ought to do:
>
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index 3e21ca8..3372adb 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;
>   }
>

However this patch (which I have now tested) works fine.  I will post it soon as part of v2 of this patch set.

    -Don Slutz

Re: [BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call put_gfn in error path.

[Mukesh Rathor]

On Mon, 06 Jan 2014 14:50:58 -0500
Don Slutz <dslutz@verizon.com> wrote:

> On 01/06/14 08:13, Andrew Cooper wrote:
> > On 06/01/14 12:43, Don Slutz wrote:
> >> 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:
> >>>>
......
> >>> 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.

Right, that is similar to what I had done when I had noticed this early this
year. I had just changed to nolock query since the domain is paused during
this call, but in hindsight it's better to do locked query. So I prefer
doing in dbg_hvm_va2mfn() too.

http://lists.xen.org/archives/html/xen-devel/2013-01/msg00781.html

Will wait for your v2 you said you will be submitting.

thanks,
mukesh

[PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Don Slutz]

This also fixes the old debug output to compile and work if DBGP1
and DBGP2 are defined like DBGP3.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/debug.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index eceb805..2e0a05a 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -41,6 +41,12 @@
 #define DBGP2(...) ((void)0)
 #endif
 
+#ifdef XEN_GDBSX_DEBUG3
+#define DBGP3(...) gdprintk(XENLOG_DEBUG, __VA_ARGS__)
+#else
+#define DBGP3(...) ((void)0)
+#endif
+
 /* Returns: mfn for the given (hvm guest) vaddr */
 static unsigned long 
 dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
@@ -60,6 +66,7 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
     }
 
     mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); 
+    DBGP3("L: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
     if ( p2m_is_readonly(gfntype) && toaddr )
     {
         DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
@@ -102,8 +109,8 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val)
         l4e = l4t[l4_table_offset(vaddr)];
         unmap_domain_page(l4t);
         mfn = l4e_get_pfn(l4e);
-        DBGP2("l4t:%p l4to:%lx l4e:%lx mfn:%lx\n", l4t, 
-              l4_table_offset(vaddr), l4e, mfn);
+        DBGP2("l4t:%p l4to:%lx l4e:%" PRIpte " mfn:%lx\n",
+              l4t, l4_table_offset(vaddr), l4e_get_intpte(l4e), mfn);
         if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
         {
             DBGP1("l4 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
@@ -114,8 +121,8 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val)
         l3e = l3t[l3_table_offset(vaddr)];
         unmap_domain_page(l3t);
         mfn = l3e_get_pfn(l3e);
-        DBGP2("l3t:%p l3to:%lx l3e:%lx mfn:%lx\n", l3t, 
-              l3_table_offset(vaddr), l3e, mfn);
+        DBGP2("l3t:%p l3to:%lx l3e:%" PRIpte " mfn:%lx\n",
+              l3t, l3_table_offset(vaddr), l3e_get_intpte(l3e), mfn);
         if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) ||
              (l3e_get_flags(l3e) & _PAGE_PSE) )
         {
@@ -128,8 +135,8 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val)
     l2e = l2t[l2_table_offset(vaddr)];
     unmap_domain_page(l2t);
     mfn = l2e_get_pfn(l2e);
-    DBGP2("l2t:%p l2to:%lx l2e:%lx mfn:%lx\n", l2t, l2_table_offset(vaddr),
-          l2e, mfn);
+    DBGP2("l2t:%p l2to:%lx l2e:%" PRIpte " mfn:%lx\n",
+          l2t, l2_table_offset(vaddr), l2e_get_intpte(l2e), mfn);
     if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) ||
          (l2e_get_flags(l2e) & _PAGE_PSE) )
     {
@@ -140,8 +147,8 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val)
     l1e = l1t[l1_table_offset(vaddr)];
     unmap_domain_page(l1t);
     mfn = l1e_get_pfn(l1e);
-    DBGP2("l1t:%p l1to:%lx l1e:%lx mfn:%lx\n", l1t, l1_table_offset(vaddr),
-          l1e, mfn);
+    DBGP2("l1t:%p l1to:%lx l1e:%" PRIpte " mfn:%lx\n",
+          l1t, l1_table_offset(vaddr), l1e_get_intpte(l1e), mfn);
 
     return mfn_valid(mfn) ? mfn : INVALID_MFN;
 }
@@ -162,8 +169,11 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len, struct domain *dp,
                ? dbg_hvm_va2mfn(addr, dp, toaddr, &gfn)
                : dbg_pv_va2mfn(addr, dp, pgd3));
         if ( mfn == INVALID_MFN ) {
-            if ( gfn != INVALID_GFN )
+            if ( gfn != INVALID_GFN ) {
                 put_gfn(dp, gfn);
+                DBGP3("R: addr:%lx pagecnt=%ld domid:%d mfn:%lx\n",
+                      addr, pagecnt, dp->domain_id, mfn);
+            }
             break;
         }
 
@@ -181,8 +191,11 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len, struct domain *dp,
         }
 
         unmap_domain_page(va);
-        if ( gfn != INVALID_GFN )
+        if ( gfn != INVALID_GFN ) {
             put_gfn(dp, gfn);
+            DBGP3("R: addr:%lx pagecnt=%ld domid:%d mfn:%lx\n",
+                  addr, pagecnt, dp->domain_id, mfn);
+        }
 
         addr += pagecnt;
         buf += pagecnt;
@@ -226,3 +239,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
     return len;
 }
 
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.8.4

Re: [PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Konrad Rzeszutek Wilk]

On Sat, Jan 04, 2014 at 12:52:14PM -0500, Don Slutz wrote:
> This also fixes the old debug output to compile and work if DBGP1
> and DBGP2 are defined like DBGP3.
> 
> @@ -226,3 +239,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
>      return len;
>  }
>  
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

??

Re: [PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Don Slutz]

[-- Attachment #1.1: Type: text/plain, Size: 978 bytes --]

On 01/04/14 16:13, Konrad Rzeszutek Wilk wrote:
> On Sat, Jan 04, 2014 at 12:52:14PM -0500, Don Slutz wrote:
>> This also fixes the old debug output to compile and work if DBGP1
>> and DBGP2 are defined like DBGP3.
>>
>> @@ -226,3 +239,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
>>       return len;
>>   }
>>   
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> ??

I take that as "Why add this"?

Dealing with many different styles of code, I find it helps to have this 
emacs add.  From CODING_STYLE:

    Emacs local variables
    ---------------------

    A comment block containing local variables for emacs is permitted at
    the end of files.  It should be:

    /*
      * Local variables:
      * mode: C
      * c-file-style: "BSD"
      * c-basic-offset: 4
      * indent-tabs-mode: nil
      * End:
      */

    -Don Slutz

[-- Attachment #1.2: Type: text/html, Size: 6070 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Konrad Rzeszutek Wilk]

Don Slutz <dslutz@verizon.com> wrote:
>On 01/04/14 16:13, Konrad Rzeszutek Wilk wrote:
>> On Sat, Jan 04, 2014 at 12:52:14PM -0500, Don Slutz wrote:
>>> This also fixes the old debug output to compile and work if DBGP1
>>> and DBGP2 are defined like DBGP3.
>>>
>>> @@ -226,3 +239,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int
>len, domid_t domid, int toaddr,
>>>       return len;
>>>   }
>>>   
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>> ??
>
>I take that as "Why add this"?

>
>Dealing with many different styles of code, I find it helps to have
>this 
>emacs add.  From CODING_STYLE:

Sure. But then this should be a separate patch. The way both in Xen and Linux is to have one logical change per patch.

I would recommend you split this out from this patch.
>
>    Emacs local variables
>    ---------------------
>
>   A comment block containing local variables for emacs is permitted at
>    the end of files.  It should be:
>
>    /*
>      * Local variables:
>      * mode: C
>      * c-file-style: "BSD"
>      * c-basic-offset: 4
>      * indent-tabs-mode: nil
>      * End:
>      */
>
>    -Don Slutz

Re: [PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Ian Campbell]

On Sat, 2014-01-04 at 16:24 -0500, Don Slutz wrote:
> On 01/04/14 16:13, Konrad Rzeszutek Wilk wrote:
> 
> > On Sat, Jan 04, 2014 at 12:52:14PM -0500, Don Slutz wrote:
> > > This also fixes the old debug output to compile and work if DBGP1
> > > and DBGP2 are defined like DBGP3.
> > > 
> > > @@ -226,3 +239,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
> > >      return len;
> > >  }
> > >  
> > > +/*
> > > + * Local variables:
> > > + * mode: C
> > > + * c-file-style: "BSD"
> > > + * c-basic-offset: 4
> > > + * indent-tabs-mode: nil
> > > + * End:
> > > + */
> > ??
> 
> I take that as "Why add this"?
> 
> Dealing with many different styles of code, I find it helps to have
> this emacs add.

Yes, but why is it in the optional debug patch?

Re: [PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Don Slutz]

On 01/06/14 10:28, Ian Campbell wrote:
> On Sat, 2014-01-04 at 16:24 -0500, Don Slutz wrote:
>> On 01/04/14 16:13, Konrad Rzeszutek Wilk wrote:
>>
>>> On Sat, Jan 04, 2014 at 12:52:14PM -0500, Don Slutz wrote:
>>>> This also fixes the old debug output to compile and work if DBGP1
>>>> and DBGP2 are defined like DBGP3.
>>>>
>>>> @@ -226,3 +239,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
>>>>       return len;
>>>>   }
>>>>   
>>>> +/*
>>>> + * Local variables:
>>>> + * mode: C
>>>> + * c-file-style: "BSD"
>>>> + * c-basic-offset: 4
>>>> + * indent-tabs-mode: nil
>>>> + * End:
>>>> + */
>>> ??
>> I take that as "Why add this"?
>>
>> Dealing with many different styles of code, I find it helps to have
>> this emacs add.
> Yes, but why is it in the optional debug patch?
>
>

Because I did not think that it needs to be in 4.4.0.  I have no issue with moving it into the 1st patch.  Will wait for guidance.

    -Don Slutz

Re: [PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Ian Campbell]

On Mon, 2014-01-06 at 11:41 -0500, Don Slutz wrote:
> Because I did not think that it needs to be in 4.4.0.

Oh, did you mean it was optional in the sense of could be deferred from
4.4.0 to 4.5.0 versus being entirely discardable?

>   I have no issue with moving it into the 1st patch.

As usual a separate patch for a separate change would be best.

Ian.

Re: [PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Don Slutz]

On 01/06/14 11:44, Ian Campbell wrote:
> On Mon, 2014-01-06 at 11:41 -0500, Don Slutz wrote:
>> Because I did not think that it needs to be in 4.4.0.
> Oh, did you mean it was optional in the sense of could be deferred from
> 4.4.0 to 4.5.0 versus being entirely discardable?

Yes, that is what I was trying to say.

>
>>    I have no issue with moving it into the 1st patch.

Will make this change it's own patch.

    -Don Slutz

> As usual a separate patch for a separate change would be best.
>
> Ian.
>

Re: [PATCH 2/4] dbg_rw_guest_mem: Enable debug log output

[Mukesh Rathor]

On Sat,  4 Jan 2014 12:52:14 -0500
Don Slutz <dslutz@verizon.com> wrote:

> This also fixes the old debug output to compile and work if DBGP1
> and DBGP2 are defined like DBGP3.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  xen/arch/x86/debug.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index eceb805..2e0a05a 100644
> --- a/xen/arch/x86/debug.c
> +++ b/xen/arch/x86/debug.c
> @@ -41,6 +41,12 @@
>  #define DBGP2(...) ((void)0)
>  #endif
>  
> +#ifdef XEN_GDBSX_DEBUG3
> +#define DBGP3(...) gdprintk(XENLOG_DEBUG, __VA_ARGS__)
> +#else
> +#define DBGP3(...) ((void)0)
> +#endif
> +

Umm... some hostorical perspective... this file is shared by both
gdbsx, and kdb, and possibly any future debug type tools. While gdbsx
got merged, kdb did not. So how about, just define say dbg_debug to replace
kdbdbg, (please don't call it "debug" - I hate using words that grep 
million results), and then just change DBGP*. Also looks like DBGP is 
not used anywhere, so we only need two... 

static volatile int dbg_debug;
#define DBGP(...) {(dbg_debug) ? printk(__VA_ARGS__) : 0;}
#define DBGP1(...) {(dbg_debug > 1) ? printk(__VA_ARGS__) : 0;}

This allows us to not need XEN_GDBSX_DEBUG3, and also the debug
can be enabled dynamically without recompile (in my case, via kdb).

thanks,
mukesh

[BUGFIX][PATCH 3/4] xg_read_mem: Report on error.

[Don Slutz]

I had coded this with XGERR, but gdb will try to read memory without
a direct request from the user.  So the error message can be confusing.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/debugger/gdbsx/xg/xg_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
index 5736b86..a192478 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -775,7 +775,7 @@ xg_read_mem(uint64_t guestva, char *tobuf, int tobuf_len, uint64_t pgd3val)
 {
     struct xen_domctl_gdbsx_memio *iop = &domctl.u.gdbsx_guest_memio;
     union {uint64_t llbuf8; char buf8[8];} u = {0};
-    int i;
+    int i, rc;
 
     XGTRC("E:gva:%llx tobuf:%lx len:%d\n", guestva, tobuf, tobuf_len);
 
@@ -786,7 +786,9 @@ xg_read_mem(uint64_t guestva, char *tobuf, int tobuf_len, uint64_t pgd3val)
     iop->len = tobuf_len;
     iop->gwr = 0;       /* not writing to guest */
 
-    _domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, tobuf, tobuf_len);
+    if ( (rc = _domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, tobuf, tobuf_len)) )
+        XGTRC("ERROR: failed to read %d bytes. errno:%d rc:%d\n",
+              iop->remain, errno, rc);
 
     for(i=0; i < XGMIN(8, tobuf_len); u.buf8[i]=tobuf[i], i++);
     XGTRC("X:remain:%d buf8:0x%llx\n", iop->remain, u.llbuf8);
-- 
1.8.4

Re: [BUGFIX][PATCH 3/4] xg_read_mem: Report on error.

[Mukesh Rathor]

On Sat,  4 Jan 2014 12:52:15 -0500
Don Slutz <dslutz@verizon.com> wrote:

> I had coded this with XGERR, but gdb will try to read memory without
> a direct request from the user.  So the error message can be
> confusing.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Acked-by: Mukesh Rathor <mukesh.rathor@oracle.com>

> ---
>  tools/debugger/gdbsx/xg/xg_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/debugger/gdbsx/xg/xg_main.c
> b/tools/debugger/gdbsx/xg/xg_main.c index 5736b86..a192478 100644
> --- a/tools/debugger/gdbsx/xg/xg_main.c
> +++ b/tools/debugger/gdbsx/xg/xg_main.c
> @@ -775,7 +775,7 @@ xg_read_mem(uint64_t guestva, char *tobuf, int
> tobuf_len, uint64_t pgd3val) {
>      struct xen_domctl_gdbsx_memio *iop = &domctl.u.gdbsx_guest_memio;
>      union {uint64_t llbuf8; char buf8[8];} u = {0};
> -    int i;
> +    int i, rc;
>  
>      XGTRC("E:gva:%llx tobuf:%lx len:%d\n", guestva, tobuf,
> tobuf_len); 
> @@ -786,7 +786,9 @@ xg_read_mem(uint64_t guestva, char *tobuf, int
> tobuf_len, uint64_t pgd3val) iop->len = tobuf_len;
>      iop->gwr = 0;       /* not writing to guest */
>  
> -    _domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, tobuf, tobuf_len);
> +    if ( (rc = _domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, tobuf,
> tobuf_len)) )
> +        XGTRC("ERROR: failed to read %d bytes. errno:%d rc:%d\n",
> +              iop->remain, errno, rc);
>  
>      for(i=0; i < XGMIN(8, tobuf_len); u.buf8[i]=tobuf[i], i++);
>      XGTRC("X:remain:%d buf8:0x%llx\n", iop->remain, u.llbuf8);

[BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Don Slutz]

The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
returned.

Without this gdb does not report an error.

With this patch and using a 1G hvm domU:

(gdb) x/1xh 0x6ae9168b
0x6ae9168b:     Cannot access memory at address 0x6ae9168b

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/domctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index ef6c140..4aa751f 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -997,8 +997,7 @@ long arch_do_domctl(
             domctl->u.gdbsx_guest_memio.len;
 
         ret = gdbsx_guest_mem_io(domctl->domain, &domctl->u.gdbsx_guest_memio);
-        if ( !ret )
-           copyback = 1;
+        copyback = 1;
     }
     break;
 
-- 
1.8.4

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Andrew Cooper]

On 04/01/2014 17:52, Don Slutz wrote:
> The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
> returned.
>
> Without this gdb does not report an error.
>
> With this patch and using a 1G hvm domU:
>
> (gdb) x/1xh 0x6ae9168b
> 0x6ae9168b:     Cannot access memory at address 0x6ae9168b
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  xen/arch/x86/domctl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index ef6c140..4aa751f 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -997,8 +997,7 @@ long arch_do_domctl(
>              domctl->u.gdbsx_guest_memio.len;
>  
>          ret = gdbsx_guest_mem_io(domctl->domain, &domctl->u.gdbsx_guest_memio);
> -        if ( !ret )
> -           copyback = 1;
> +        copyback = 1;

This whole hypercall implementation looks suspect.

gdbsx_guest_mem_io() itself writes to the 'remain' field, making the
statement crossing the top of this context redundant.

Furthermore, ret is strictly 0 in the case that 'remain' is 0, or
-EFAULT if 'remain' is non-0.

While this does change the ABI of a hypercall, I think it is reasonable
to do, as domctl.h does imply that 'remain' is an output parameter,
without specifying anything about its behaviour in the case of an error.

~Andrew

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Ian Campbell]

On Sun, 2014-01-05 at 23:35 +0000, Andrew Cooper wrote:
> While this does change the ABI of a hypercall, I think it is
> reasonable to do, as domctl.h does imply that 'remain' is an output
> parameter, without specifying anything about its behaviour in the case
> of an error. 

The domctl ABI is subject to change anyway.

We don't seem to have had any cause to bump XEN_DOMCTL_INTERFACE_VERSION
during the 4.4 dev cycle yet. I don't think this change warrants it
either.

Ian.

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Mukesh Rathor]

On Sat,  4 Jan 2014 12:52:16 -0500
Don Slutz <dslutz@verizon.com> wrote:

> The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
> returned.
> 
> Without this gdb does not report an error.
> 
> With this patch and using a 1G hvm domU:
> 
> (gdb) x/1xh 0x6ae9168b
> 0x6ae9168b:     Cannot access memory at address 0x6ae9168b
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  xen/arch/x86/domctl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index ef6c140..4aa751f 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -997,8 +997,7 @@ long arch_do_domctl(
>              domctl->u.gdbsx_guest_memio.len;
>  
>          ret = gdbsx_guest_mem_io(domctl->domain,
> &domctl->u.gdbsx_guest_memio);
> -        if ( !ret )
> -           copyback = 1;
> +        copyback = 1;
>      }
>      break;
>  

Ooopsy... my thought was that an application should not even look at
remain if the hcall/syscall failed, but forgot when writing the
gdbsx itself :). Think of it this way, if the call didn't even make it to 
xen, and some reason the ioctl returned non-zero rc, then remain would 
still be zero. So I think we should fix gdbsx instead of here:

xg_write_mem():
    if ((rc=_domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, frombuf, buflen)))
    {
        XGERR("ERROR: failed to write %d bytes. errno:%d rc:%d\n",
               iop->remain, errno, rc);
        return iop->len;
    }    

Similarly in xg_read_mem().

Hope that makes sense. Don't mean to create work for you for my mistake,
so if you don't have time, I can submit a patch for this too.

thanks
Mukesh

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Ian Campbell]

On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
> On Sat,  4 Jan 2014 12:52:16 -0500
> Don Slutz <dslutz@verizon.com> wrote:
> 
> > The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
> > returned.
> > 
> > Without this gdb does not report an error.
> > 
> > With this patch and using a 1G hvm domU:
> > 
> > (gdb) x/1xh 0x6ae9168b
> > 0x6ae9168b:     Cannot access memory at address 0x6ae9168b
> > 
> > Signed-off-by: Don Slutz <dslutz@verizon.com>
> > ---
> >  xen/arch/x86/domctl.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > index ef6c140..4aa751f 100644
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -997,8 +997,7 @@ long arch_do_domctl(
> >              domctl->u.gdbsx_guest_memio.len;
> >  
> >          ret = gdbsx_guest_mem_io(domctl->domain,
> > &domctl->u.gdbsx_guest_memio);
> > -        if ( !ret )
> > -           copyback = 1;
> > +        copyback = 1;
> >      }
> >      break;
> >  
> 
> Ooopsy... my thought was that an application should not even look at
> remain if the hcall/syscall failed, but forgot when writing the
> gdbsx itself :). Think of it this way, if the call didn't even make it to 
> xen, and some reason the ioctl returned non-zero rc, then remain would 
> still be zero. So I think we should fix gdbsx instead of here:
> 
> xg_write_mem():
>     if ((rc=_domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, frombuf, buflen)))
>     {
>         XGERR("ERROR: failed to write %d bytes. errno:%d rc:%d\n",
>                iop->remain, errno, rc);

Isn't this still using iop->remain on failure which is what you say
shouldn't be done?

>         return iop->len;
>     }    
> 
> Similarly in xg_read_mem().
> 
> Hope that makes sense. Don't mean to create work for you for my mistake,
> so if you don't have time, I can submit a patch for this too.
> 
> thanks
> Mukesh

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Ian Campbell]

On Tue, 2014-01-07 at 10:00 +0000, Ian Campbell wrote:
> On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
> > On Sat,  4 Jan 2014 12:52:16 -0500
> > Don Slutz <dslutz@verizon.com> wrote:
> > 
> > > The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
> > > returned.
> > > 
> > > Without this gdb does not report an error.
> > > 
> > > With this patch and using a 1G hvm domU:
> > > 
> > > (gdb) x/1xh 0x6ae9168b
> > > 0x6ae9168b:     Cannot access memory at address 0x6ae9168b
> > > 
> > > Signed-off-by: Don Slutz <dslutz@verizon.com>
> > > ---
> > >  xen/arch/x86/domctl.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > > index ef6c140..4aa751f 100644
> > > --- a/xen/arch/x86/domctl.c
> > > +++ b/xen/arch/x86/domctl.c
> > > @@ -997,8 +997,7 @@ long arch_do_domctl(
> > >              domctl->u.gdbsx_guest_memio.len;
> > >  
> > >          ret = gdbsx_guest_mem_io(domctl->domain,
> > > &domctl->u.gdbsx_guest_memio);
> > > -        if ( !ret )
> > > -           copyback = 1;
> > > +        copyback = 1;
> > >      }
> > >      break;
> > >  
> > 
> > Ooopsy... my thought was that an application should not even look at
> > remain if the hcall/syscall failed, but forgot when writing the
> > gdbsx itself :). Think of it this way, if the call didn't even make it to 
> > xen, and some reason the ioctl returned non-zero rc, then remain would 
> > still be zero.

Thinking about this for a second longer -- how does this interface deal
with partial success?

It seems natural to me for it to return an error and also update
->remain, but perhaps you have a different scheme in mind?

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Don Slutz]

On 01/07/14 05:02, Ian Campbell wrote:
> On Tue, 2014-01-07 at 10:00 +0000, Ian Campbell wrote:
>> On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
>>> On Sat,  4 Jan 2014 12:52:16 -0500
>>> Don Slutz <dslutz@verizon.com> wrote:
>>>
>>>> The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
>>>> returned.
>>>>
>>>> Without this gdb does not report an error.
>>>>
>>>> With this patch and using a 1G hvm domU:
>>>>
>>>> (gdb) x/1xh 0x6ae9168b
>>>> 0x6ae9168b:     Cannot access memory at address 0x6ae9168b
>>>>
>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>> ---
>>>>   xen/arch/x86/domctl.c | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>>> index ef6c140..4aa751f 100644
>>>> --- a/xen/arch/x86/domctl.c
>>>> +++ b/xen/arch/x86/domctl.c
>>>> @@ -997,8 +997,7 @@ long arch_do_domctl(
>>>>               domctl->u.gdbsx_guest_memio.len;
>>>>   
>>>>           ret = gdbsx_guest_mem_io(domctl->domain,
>>>> &domctl->u.gdbsx_guest_memio);
>>>> -        if ( !ret )
>>>> -           copyback = 1;
>>>> +        copyback = 1;
>>>>       }
>>>>       break;
>>>>   
>>> Ooopsy... my thought was that an application should not even look at
>>> remain if the hcall/syscall failed, but forgot when writing the
>>> gdbsx itself :). Think of it this way, if the call didn't even make it to
>>> xen, and some reason the ioctl returned non-zero rc, then remain would
>>> still be zero.
> Thinking about this for a second longer -- how does this interface deal
> with partial success?
>
> It seems natural to me for it to return an error and also update
> ->remain, but perhaps you have a different scheme in mind?
>
>

I had assumed that this patch (which is not need to "fix" the bugs I found), was to be dropped in v2.  However I will agree that currently there is no way to know about partial success.  The untested change:

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index ef6c140..0add07e 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -43,7 +43,7 @@ static int gdbsx_guest_mem_io(
      iop->remain = dbg_rw_mem(
          (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
          iop->gwr, iop->pgd3val);
-    return (iop->remain ? -EFAULT : 0);
+    return 0;
  }

  long arch_do_domctl(


Would appear to allow partial success to be reported and also meet with remain not to be looked at with an error.

    -Don Slutz

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Mukesh Rathor]

On Tue, 07 Jan 2014 11:24:15 -0500
Don Slutz <dslutz@verizon.com> wrote:

> On 01/07/14 05:02, Ian Campbell wrote:
> > On Tue, 2014-01-07 at 10:00 +0000, Ian Campbell wrote:
> >> On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
> >>> On Sat,  4 Jan 2014 12:52:16 -0500
> >>> Don Slutz <dslutz@verizon.com> wrote:
> >>>
> >>>> The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
> >>>> returned.
> >>>>

..... 

> I had assumed that this patch (which is not need to "fix" the bugs I
> found), was to be dropped in v2.  However I will agree that currently
> there is no way to know about partial success.  The untested change:
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index ef6c140..0add07e 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -43,7 +43,7 @@ static int gdbsx_guest_mem_io(
>       iop->remain = dbg_rw_mem(
>           (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
>           iop->gwr, iop->pgd3val);
> -    return (iop->remain ? -EFAULT : 0);
> +    return 0;
>   }
> 
>   long arch_do_domctl(
> 
> 
> Would appear to allow partial success to be reported and also meet
> with remain not to be looked at with an error.

No, the partial success is relevant in other cases, like EAGAIN,
but not EFAULT. If we make it pre-emptible in future to return
EAGAIN, we'd need to make sure remain was honored. Again, think of it
this way, if the first copyin failed, remain would have not been 
initialized.

So, because now the only cause of unfinished copy from dbg_rw_mem is 
EFAULT, we should leave above xen code alone, and just change gdbsx.

thanks
mukesh

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Don Slutz]

On 01/07/14 18:01, Mukesh Rathor wrote:
> On Tue, 07 Jan 2014 11:24:15 -0500
> Don Slutz <dslutz@verizon.com> wrote:
>
>> On 01/07/14 05:02, Ian Campbell wrote:
>>> On Tue, 2014-01-07 at 10:00 +0000, Ian Campbell wrote:
>>>> On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
>>>>> On Sat,  4 Jan 2014 12:52:16 -0500
>>>>> Don Slutz <dslutz@verizon.com> wrote:
>>>>>
>>>>>> The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
>>>>>> returned.
>>>>>>
> .....
>
>> I had assumed that this patch (which is not need to "fix" the bugs I
>> found), was to be dropped in v2.  However I will agree that currently
>> there is no way to know about partial success.  The untested change:
>>
>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>> index ef6c140..0add07e 100644
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -43,7 +43,7 @@ static int gdbsx_guest_mem_io(
>>        iop->remain = dbg_rw_mem(
>>            (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
>>            iop->gwr, iop->pgd3val);
>> -    return (iop->remain ? -EFAULT : 0);
>> +    return 0;
>>    }
>>
>>    long arch_do_domctl(
>>
>>
>> Would appear to allow partial success to be reported and also meet
>> with remain not to be looked at with an error.
> No, the partial success is relevant in other cases, like EAGAIN,
> but not EFAULT. If we make it pre-emptible in future to return
> EAGAIN, we'd need to make sure remain was honored. Again, think of it
> this way, if the first copyin failed, remain would have not been
> initialized.
>
> So, because now the only cause of unfinished copy from dbg_rw_mem is
> EFAULT, we should leave above xen code alone, and just change gdbsx.
>
> thanks
> mukesh

Since it did not look like we would get to an agreement soon on this, I have sent out v2 series.

Not at all clear this patch should be in 4.4.0 (domctl api change, high risk, no clear use case (as far as I know gdb does not do anything with partial success)).

On this topic; you seem to be overlooking the page crossing case.

Using the info that page 1f is good and 20 is bad, a domctl request for 1ffff for 2 bytes would call on dbg_rw_mem(), dbg_rw_guest_mem() which calculate pagecnt == 1, get a valid mfn and return that byte. The 2nd time pagecnt is also 1, but we get INVALID_MFN, so dbg_rw_guest_mem(0 returns 1.  dbg_rw_mem(0 also returns 1. gdbsx_guest_mem_io() returns -EFAULT so no copyback.

At this point of the 2 requested byte, 1 byte is valid and 1 is not.  Since copyback is not done, remain is 0.  So the caller get the error and does not have this "partial success" information.

The 1st version of this patch I proposed, the copyback is done, so the caller gets remain == 1 and the valid byte and an error.

The 2nd version of this patch (which has now been tested) sets remian == 1, the valid byte, and no error.

Hope this helps.

    -Don Slutz

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Mukesh Rathor]

On Tue, 07 Jan 2014 20:02:41 -0500
Don Slutz <dslutz@verizon.com> wrote:

> On 01/07/14 18:01, Mukesh Rathor wrote:
> > On Tue, 07 Jan 2014 11:24:15 -0500
> > Don Slutz <dslutz@verizon.com> wrote:
....

> Using the info that page 1f is good and 20 is bad, a domctl request
> for 1ffff for 2 bytes would call on dbg_rw_mem(), dbg_rw_guest_mem()
> which calculate pagecnt == 1, get a valid mfn and return that byte.
> The 2nd time pagecnt is also 1, but we get INVALID_MFN, so
> dbg_rw_guest_mem(0 returns 1.  dbg_rw_mem(0 also returns 1.
> gdbsx_guest_mem_io() returns -EFAULT so no copyback.
> 
> At this point of the 2 requested byte, 1 byte is valid and 1 is not.
> Since copyback is not done, remain is 0.  So the caller get the error
> and does not have this "partial success" information.

Again, the application cannot and should not rely on the validity 
of the field in case of failing hcall/syscall, since the failure point
is not known to the application, unless the failure is EAGAIN.

Hope that makes sense.

Mukesh

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Don Slutz]

On 01/07/14 05:00, Ian Campbell wrote:
> On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
>> On Sat,  4 Jan 2014 12:52:16 -0500
>> Don Slutz <dslutz@verizon.com> wrote:
>>
>>> The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
>>> returned.
>>>
>>> Without this gdb does not report an error.
>>>
>>> With this patch and using a 1G hvm domU:
>>>
>>> (gdb) x/1xh 0x6ae9168b
>>> 0x6ae9168b:     Cannot access memory at address 0x6ae9168b
>>>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>> ---
>>>   xen/arch/x86/domctl.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>> index ef6c140..4aa751f 100644
>>> --- a/xen/arch/x86/domctl.c
>>> +++ b/xen/arch/x86/domctl.c
>>> @@ -997,8 +997,7 @@ long arch_do_domctl(
>>>               domctl->u.gdbsx_guest_memio.len;
>>>   
>>>           ret = gdbsx_guest_mem_io(domctl->domain,
>>> &domctl->u.gdbsx_guest_memio);
>>> -        if ( !ret )
>>> -           copyback = 1;
>>> +        copyback = 1;
>>>       }
>>>       break;
>>>   
>> Ooopsy... my thought was that an application should not even look at
>> remain if the hcall/syscall failed, but forgot when writing the
>> gdbsx itself :). Think of it this way, if the call didn't even make it to
>> xen, and some reason the ioctl returned non-zero rc, then remain would
>> still be zero. So I think we should fix gdbsx instead of here:
>>
>> xg_write_mem():
>>      if ((rc=_domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, frombuf, buflen)))
>>      {
>>          XGERR("ERROR: failed to write %d bytes. errno:%d rc:%d\n",
>>                 iop->remain, errno, rc);
> Isn't this still using iop->remain on failure which is what you say
> shouldn't be done?

I agree, which is why I took it as a guide line.  What I have tested with is:

diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
index 3b2a285..53a0ce1 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -787,8 +787,11 @@ xg_read_mem(uint64_t guestva, char *tobuf, int tobuf_len, uint64_t pgd3val)
      iop->gwr = 0;       /* not writing to guest */

      if ( (rc = _domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, tobuf, tobuf_len)) )
-        XGTRC("ERROR: failed to read %d bytes. errno:%d rc:%d\n",
-              iop->remain, errno, rc);
+    {
+        XGTRC("ERROR: failed to read bytes. errno:%d rc:%d\n",
+              errno, rc);
+        return tobuf_len;
+    }

      for(i=0; i < XGMIN(8, tobuf_len); u.buf8[i]=tobuf[i], i++);
      XGTRC("X:remain:%d buf8:0x%llx\n", iop->remain, u.llbuf8);
@@ -818,8 +821,11 @@ xg_write_mem(uint64_t guestva, char *frombuf, int buflen, uint64_t pgd3val)
      iop->gwr = 1;       /* writing to guest */

      if ((rc=_domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, frombuf, buflen)))
-        XGERR("ERROR: failed to write %d bytes. errno:%d rc:%d\n",
-              iop->remain, errno, rc);
+    {
+        XGERR("ERROR: failed to write bytes to %llx. errno:%d rc:%d\n",
+              guestva, errno, rc);
+        return buflen;
+    }
      return iop->remain;
  }


works fine and I plan it to be part of v2.
     -Don


>>          return iop->len;
>>      }
>>
>> Similarly in xg_read_mem().
>>
>> Hope that makes sense. Don't mean to create work for you for my mistake,
>> so if you don't have time, I can submit a patch for this too.
>>
>> thanks
>> Mukesh
>

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Mukesh Rathor]

On Tue, 07 Jan 2014 11:26:36 -0500
Don Slutz <dslutz@verizon.com> wrote:

> On 01/07/14 05:00, Ian Campbell wrote:
> > On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
> >> On Sat,  4 Jan 2014 12:52:16 -0500
> >> Don Slutz <dslutz@verizon.com> wrote:
.....
> +++ b/tools/debugger/gdbsx/xg/xg_main.c
> @@ -787,8 +787,11 @@ xg_read_mem(uint64_t guestva, char *tobuf, int
> tobuf_len, uint64_t pgd3val) iop->gwr = 0;       /* not writing to
> guest */
> 
>       if ( (rc = _domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, tobuf,
> tobuf_len)) )
> -        XGTRC("ERROR: failed to read %d bytes. errno:%d rc:%d\n",
> -              iop->remain, errno, rc);
> +    {
> +        XGTRC("ERROR: failed to read bytes. errno:%d rc:%d\n",
> +              errno, rc);


Probably would fit in just one line. 
           XGTRC("ERROR: failed to read bytes. errno:%d rc:%d\n", errno, rc);


> +        return tobuf_len;
> +    }
> 
>       for(i=0; i < XGMIN(8, tobuf_len); u.buf8[i]=tobuf[i], i++);
>       XGTRC("X:remain:%d buf8:0x%llx\n", iop->remain, u.llbuf8);
> @@ -818,8 +821,11 @@ xg_write_mem(uint64_t guestva, char *frombuf,
> int buflen, uint64_t pgd3val) iop->gwr = 1;       /* writing to guest
> */
> 
>       if ((rc=_domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, frombuf,
> buflen)))
> -        XGERR("ERROR: failed to write %d bytes. errno:%d rc:%d\n",
> -              iop->remain, errno, rc);
> +    {
> +        XGERR("ERROR: failed to write bytes to %llx. errno:%d
> rc:%d\n",
> +              guestva, errno, rc);
> +        return buflen;
> +    }
>       return iop->remain;
>   }
> 
> 
> works fine and I plan it to be part of v2.

Yes, this is it.

thanks
Mukesh

Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Mukesh Rathor]

On Tue, 7 Jan 2014 10:00:24 +0000
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
> > On Sat,  4 Jan 2014 12:52:16 -0500
> > Don Slutz <dslutz@verizon.com> wrote:
> > 
> > > The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
> > > returned.
> > > 
> > > Without this gdb does not report an error.
> > > 
> > > With this patch and using a 1G hvm domU:
> > > 
> > > (gdb) x/1xh 0x6ae9168b
> > > 0x6ae9168b:     Cannot access memory at address 0x6ae9168b
> > > 
> > > Signed-off-by: Don Slutz <dslutz@verizon.com>
> > > ---
> > >  xen/arch/x86/domctl.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > > index ef6c140..4aa751f 100644
> > > --- a/xen/arch/x86/domctl.c
> > > +++ b/xen/arch/x86/domctl.c
> > > @@ -997,8 +997,7 @@ long arch_do_domctl(
> > >              domctl->u.gdbsx_guest_memio.len;
> > >  
> > >          ret = gdbsx_guest_mem_io(domctl->domain,
> > > &domctl->u.gdbsx_guest_memio);
> > > -        if ( !ret )
> > > -           copyback = 1;
> > > +        copyback = 1;
> > >      }
> > >      break;
> > >  
> > 
> > Ooopsy... my thought was that an application should not even look at
> > remain if the hcall/syscall failed, but forgot when writing the
> > gdbsx itself :). Think of it this way, if the call didn't even make
> > it to xen, and some reason the ioctl returned non-zero rc, then
> > remain would still be zero. So I think we should fix gdbsx instead
> > of here:
> > 
> > xg_write_mem():
> >     if ((rc=_domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, frombuf,
> > buflen))) {
> >         XGERR("ERROR: failed to write %d bytes. errno:%d rc:%d\n",
> >                iop->remain, errno, rc);
> 
> Isn't this still using iop->remain on failure which is what you say
> shouldn't be done?

Right, it was just a guidline code with bad cut and paste of XGERR()..
so picky :) :)... 

Mukesh

Re: [BUGFIX][PATCH 0/4] gdbsx: fix 3 bugs

[Ian Campbell]

On Sat, 2014-01-04 at 12:52 -0500, Don Slutz wrote:
> Release manager requests:
>   patch 1 should be in 4.4.0
>   patch 3 and 4 would be good to be in 4.4.0

(as acting RM in George's absence):

In all three cases what is missing is the "why" and the appropriate
analysis from
http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_code_freeze 
i.e. what is the impact of the bug (i..e what are the advantages of the
fix) and what are the risks of this changing causing further breakage?
I'm not really in a position to evaluate the risk of a change in gdbsx,
so someone needs to tell me.

I think given that gdbsx is a somewhat "peripheral" bit of code and that
it is targeted at developers (who might be better able to tolerate any
resulting issues and more able to apply subsequent fixups than regular
users) we can accept a larger risk than we would with a change to the
hypervisor itself etc (that's assuming that all of these changes cannot
potentially impact non-debugger usecases which I expect is the case from
the function names but I would like to see confirmed).

Apart from a release ack 4 of these would need an Ack from Mukesh IMHO.
TBH if he is happy with them then I see no reason not to give a release
ack as well.

>   patch 2 is optional.
> 
> While tracking down a bug in seabios/grub I found the bug in patch
> 1.
> 
> There are 2 ways that gfn will not be INVALID_GFN and yet mfn will
> be INVALID_MFN.
> 
>   1) p2m_is_readonly(gfntype) and writing memory.
>   2) the requested vaddr does not exist.
> 
> This may only be an issue for a HVM guest that is in real mode
> (I.E. no page tables).
> 
> Patch 2 is debug logging that was used to find the 2nd way.
> 
> Patch 3 and 4 are more of a cleanup bug fix.
> 
> Don Slutz (4):
>   dbg_rw_guest_mem: need to call put_gfn in error path.
>   dbg_rw_guest_mem: Enable debug log output
>   xg_read_mem: Report on error.
>   XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.
> 
>  tools/debugger/gdbsx/xg/xg_main.c |  6 ++++--
>  xen/arch/x86/debug.c              | 44 ++++++++++++++++++++++++++++++---------
>  xen/arch/x86/domctl.c             |  3 +--
>  3 files changed, 39 insertions(+), 14 deletions(-)
> 

Re: [BUGFIX][PATCH 0/4] gdbsx: fix 3 bugs

[Mukesh Rathor]

On Mon, 6 Jan 2014 15:45:48 +0000
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Sat, 2014-01-04 at 12:52 -0500, Don Slutz wrote:
> > Release manager requests:
> >   patch 1 should be in 4.4.0
> >   patch 3 and 4 would be good to be in 4.4.0
> 
> (as acting RM in George's absence):
> 
> In all three cases what is missing is the "why" and the appropriate
> analysis from
> http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_code_freeze 
> i.e. what is the impact of the bug (i..e what are the advantages of
> the fix) and what are the risks of this changing causing further
> breakage? I'm not really in a position to evaluate the risk of a
> change in gdbsx, so someone needs to tell me.
> 
> I think given that gdbsx is a somewhat "peripheral" bit of code and
> that it is targeted at developers (who might be better able to
> tolerate any resulting issues and more able to apply subsequent
> fixups than regular users) we can accept a larger risk than we would
> with a change to the hypervisor itself etc (that's assuming that all
> of these changes cannot potentially impact non-debugger usecases
> which I expect is the case from the function names but I would like
> to see confirmed).
> 
> Apart from a release ack 4 of these would need an Ack from Mukesh
> IMHO. TBH if he is happy with them then I see no reason not to give a
> release ack as well.

Right Ian, at present I believe the only user of this file is gdbsx
so 4.4 would be very low risk. The issue is in a rare use case, so it 
could wait till 4.5 too if you'd rather not. Ok either way... :)..

thanks
mukesh