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

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

[Don Slutz]

Changes v3 to v4:
    Drop patch 1 -- already commited
    Drop patch 3 -- Does not need to be in 4.4 as far as I know
    Added "Acked-by: Mukesh Rathor <mukesh.rathor@oracle.com>" to all 3.

Changes v2 to v3:
  From Jan Beulich:
    Add else to keep debug log the same.

Changes v1 to v2:
  From Konrad Rzeszutek Wilk and Ian Campbell:

    ??

  Split out emacs local variables add to it's own new patch (number 1).

  From Andrew Cooper:

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

  So use his version of the change.

  From Ian Campbell:

    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).
 
  My take on this below.

  From Mukesh Rathor:

    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:

  Dropped old patch 4, Added new patch 5.


Freeze:

  The benefit of this series is that the hypervisor stops calling
  panic (debug=y) or hanging (debug=n).  Also a person using gdbsx
  is not seeing random heap data of gdbsx's heap instead of guest
  data.

  The risk is that gdbsx does something new wrong.

  My understanding is that all the changes here only effect gdbsx
  and so very limited in scope.

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

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

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 3 is debug logging that was used to find the 2nd way.


Andrew Cooper (1):
  dbg_rw_guest_mem: need to call put_gfn in error path.

Don Slutz (2):
  xg_read_mem: Report on error.
  xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error.

 tools/debugger/gdbsx/xg/xg_main.c | 15 +++++++++++----
 xen/arch/x86/debug.c              | 11 +++++++++--
 2 files changed, 20 insertions(+), 6 deletions(-)

-- 
1.8.4

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

[Don Slutz]

From: Andrew Cooper <andrew.cooper3@citrix.com>

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: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Don Slutz <dslutz@verizon.com>
Tested-by: Don Slutz <dslutz@verizon.com>
Acked-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/debug.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index a67a192..435bd40 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;
+    }
+    else
+        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;
     }
 
-    DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
     return mfn;
 }
 
-- 
1.8.4

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

[Jan Beulich]

>>> On 10.01.14 at 22:56, Don Slutz <dslutz@verizon.com> wrote:

I don't see why you included this in the resend - it got applied
earlier on Friday.

Jan

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

[Don Slutz]

On 01/13/14 06:39, Jan Beulich wrote:
>>>> On 10.01.14 at 22:56, Don Slutz <dslutz@verizon.com> wrote:
> I don't see why you included this in the resend - it got applied
> earlier on Friday.
>
> Jan
>


Opps, My mistake.  In the rush to get v4 in share I forgot to check it 
vs staging also.

Sorry for the mistake.

     -Don Slutz

[BUGFIX][PATCH v4 2/3] 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>
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 0622ebd..3b2a285 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

[BUGFIX][PATCH v4 3/3] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error.

[Don Slutz]

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

Drop output of iop->remain because it most likely will be zero.
This leads to a strange message:

ERROR: failed to read 0 bytes. errno:14 rc:-1

Add address to write error because it may be the only message
displayed.

Note: currently XEN_DOMCTL_gdbsx_guestmemio does not change 'iop' on
error and so iop->remain will be zero.

Signed-off-by: Don Slutz <dslutz@verizon.com>
Acked-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 tools/debugger/gdbsx/xg/xg_main.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
index 3b2a285..0fc3f82 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -787,8 +787,10 @@ 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 +820,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;
 }
 
-- 
1.8.4

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

[Ian Campbell]

On Fri, 2014-01-10 at 16:56 -0500, Don Slutz wrote:
> Changes v3 to v4:
>     Drop patch 1 -- already commited
>     Drop patch 3 -- Does not need to be in 4.4 as far as I know
>     Added "Acked-by: Mukesh Rathor <mukesh.rathor@oracle.com>" to all 3.

> Release manager requests:
>   patch 1 and 3 are optional for 4.4.0.
>   patch 2 should be in 4.4.0
>   patch 4 and 5 would be good to be in 4.4.0

This is all totally inconsistent.

The first quoted para talks about patch numbers without names without
realising that the reader has no context for them (especially given that
the request for this resend was due to the confusion surrounding what
was what in the previous iteration).

The second quoted para hasn't been true for at least one iteration.

So, I was just about to apply:
>   xg_read_mem: Report on error.
>   xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error.

but now I've no idea whether I really should or not.

I'm going to apply anyway, since I suspect that was what was intended,
but in the future please work on the clarity of your communications, 100
lines of quotations and analysis is no good if it doesn't actually make
sense.

If I wasn't supposed to apply please shout and I will revert.

Ian.