[BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs

[BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs

[Don Slutz]

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.


Don Slutz (5):
  Add Emacs local variables to source files.
  dbg_rw_guest_mem: need to call put_gfn in error path.
  dbg_rw_guest_mem: Conditionally enable debug log output
  xg_read_mem: Report on error.
  xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error.

 tools/debugger/gdbsx/xg/xg_main.c | 23 ++++++++++---
 xen/arch/x86/debug.c              | 71 +++++++++++++++++++++++----------------
 2 files changed, 61 insertions(+), 33 deletions(-)

-- 
1.8.4

[PATCH v2 1/5] Add Emacs local variables to source files.

[Don Slutz]

These 2 files are changed in this patch set.  So add the allowed
"Emacs local variables" from CODING_STYLE.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/debugger/gdbsx/xg/xg_main.c | 8 ++++++++
 xen/arch/x86/debug.c              | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
index 5736b86..0622ebd 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -821,3 +821,11 @@ xg_write_mem(uint64_t guestva, char *frombuf, int buflen, uint64_t pgd3val)
     return iop->remain;
 }
 
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 3e21ca8..a67a192 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -223,3 +223,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

[BUGFIX][PATCH v2 2/5] 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index a67a192..ba6a64d 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;
 }
 
-- 
1.8.4

[PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Don Slutz]

If dbg_debug is non-zero, output debug.

Include put_gfn debug logging.

Here is a smaple output at dbg_debug == 2:

(XEN) [2014-01-07 03:20:09] gmem:addr:8f56 buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
(XEN) [2014-01-07 03:20:09] vaddr:8f56 domid:1
(XEN) [2014-01-07 03:20:09] X: vaddr:8f56 domid:1 mfn:64331a
(XEN) [2014-01-07 03:20:09] R: addr:8f56 pagecnt=1 domid:1 gfn:8
(XEN) [2014-01-07 03:20:09] gmem:exit:len:$0
(XEN) [2014-01-07 03:20:09] gmem:addr:8f57 buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
(XEN) [2014-01-07 03:20:09] vaddr:8f57 domid:1
(XEN) [2014-01-07 03:20:09] X: vaddr:8f57 domid:1 mfn:64331a
(XEN) [2014-01-07 03:20:09] R: addr:8f57 pagecnt=1 domid:1 gfn:8
(XEN) [2014-01-07 03:20:09] gmem:exit:len:$0
(XEN) [2014-01-07 03:20:09] gmem:addr:6ae9168b buf:00000000006e2020 len:$2 domid:1 toaddr:0 dp:ffff83083e5fe000
(XEN) [2014-01-07 03:20:09] vaddr:6ae9168b domid:1
(XEN) [2014-01-07 03:20:09] X: vaddr:6ae9168b domid:1 mfn:ffffffffffffffff
(XEN) [2014-01-07 03:20:09] R: domid:1 gfn:6ae91
(XEN) [2014-01-07 03:20:09] gmem:exit:len:$2

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

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index ba6a64d..777e5ba 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -30,16 +30,9 @@
  * gdbsx, etc..
  */
 
-#ifdef XEN_KDB_CONFIG
-#include "../kdb/include/kdbdefs.h"
-#include "../kdb/include/kdbproto.h"
-#define DBGP(...) {(kdbdbg) ? kdbp(__VA_ARGS__):0;}
-#define DBGP1(...) {(kdbdbg>1) ? kdbp(__VA_ARGS__):0;}
-#define DBGP2(...) {(kdbdbg>2) ? kdbp(__VA_ARGS__):0;}
-#else
-#define DBGP1(...) ((void)0)
-#define DBGP2(...) ((void)0)
-#endif
+static volatile int dbg_debug;
+#define DBGP(...) {(dbg_debug) ? printk(__VA_ARGS__) : 0;}
+#define DBGP1(...) {(dbg_debug > 1) ? printk(__VA_ARGS__) : 0;}
 
 /* Returns: mfn for the given (hvm guest) vaddr */
 static unsigned long 
@@ -50,27 +43,28 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
     uint32_t pfec = PFEC_page_present;
     p2m_type_t gfntype;
 
-    DBGP2("vaddr:%lx domid:%d\n", vaddr, dp->domain_id);
+    DBGP1("vaddr:%lx domid:%d\n", vaddr, dp->domain_id);
 
     *gfn = paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec);
     if ( *gfn == INVALID_GFN )
     {
-        DBGP2("kdb:bad gfn from gva_to_gfn\n");
+        DBGP1("kdb:bad gfn from gva_to_gfn\n");
         return INVALID_MFN;
     }
 
     mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); 
     if ( p2m_is_readonly(gfntype) && toaddr )
     {
-        DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
+        DBGP1("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
         mfn = INVALID_MFN;
     }
 
-    DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
+    DBGP1("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
 
     if ( mfn == INVALID_MFN )
     {
         put_gfn(dp, *gfn);
+        DBGP1("R: domid:%d gfn:%lx\n", dp->domain_id, *gfn);
         *gfn = INVALID_GFN;
     }
 
@@ -100,7 +94,7 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val)
     unsigned long cr3 = (pgd3val ? pgd3val : dp->vcpu[0]->arch.cr3);
     unsigned long mfn = cr3 >> PAGE_SHIFT;
 
-    DBGP2("vaddr:%lx domid:%d cr3:%lx pgd3:%lx\n", vaddr, dp->domain_id, 
+    DBGP1("vaddr:%lx domid:%d cr3:%lx pgd3:%lx\n", vaddr, dp->domain_id,
           cr3, pgd3val);
 
     if ( pgd3val == 0 )
@@ -109,11 +103,11 @@ 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);
+        DBGP1("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);
+            DBGP("l4 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
             return INVALID_MFN;
         }
 
@@ -121,12 +115,12 @@ 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);
+        DBGP1("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) )
         {
-            DBGP1("l3 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
+            DBGP("l3 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
             return INVALID_MFN;
         }
     }
@@ -135,20 +129,20 @@ 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);
+    DBGP1("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) )
     {
-        DBGP1("l2 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
+        DBGP("l2 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
         return INVALID_MFN;
     }
     l1t = map_domain_page(mfn);
     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);
+    DBGP1("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;
 }
@@ -186,7 +180,11 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len, struct domain *dp,
 
         unmap_domain_page(va);
         if ( gfn != INVALID_GFN )
+        {
             put_gfn(dp, gfn);
+            DBGP1("R: addr:%lx pagecnt=%ld domid:%d gfn:%lx\n",
+                  addr, pagecnt, dp->domain_id, gfn);
+        }
 
         addr += pagecnt;
         buf += pagecnt;
@@ -210,7 +208,7 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
     struct domain *dp = get_domain_by_id(domid);
     int hyp = (domid == DOMID_IDLE);
 
-    DBGP2("gmem:addr:%lx buf:%p len:$%d domid:%x toaddr:%x dp:%p\n", 
+    DBGP1("gmem:addr:%lx buf:%p len:$%d domid:%x toaddr:%x dp:%p\n",
           addr, buf, len, domid, toaddr, dp);
     if ( hyp )
     {
@@ -226,7 +224,7 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
         put_domain(dp);
     }
 
-    DBGP2("gmem:exit:len:$%d\n", len);
+    DBGP1("gmem:exit:len:$%d\n", len);
     return len;
 }
 
-- 
1.8.4

[BUGFIX][PATCH v2 4/5] 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.

Acked-by: Mukesh Rathor <mukesh.rathor@oracle.com>
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 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 v2 5/5] 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>
---
 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 v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path.

[Andrew Cooper]

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.

It should probably also include your Tested-by: tag

Ian (with RM hat on):
  This is a hypervisor reference counting error on a toolstack hypercall
path.  Irrespective of any of the other patches in this series, I think
this should be included ASAP (although probably subject to review from a
third person), which will fix the hypervisor crashes from gdbsx usage.

~Andrew

> ---
>  xen/arch/x86/debug.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index a67a192..ba6a64d 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 v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path.

[Don Slutz]

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.


> 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

>
> Ian (with RM hat on):
>    This is a hypervisor reference counting error on a toolstack hypercall
> path.  Irrespective of any of the other patches in this series, I think
> this should be included ASAP (although probably subject to review from a
> third person), which will fix the hypervisor crashes from gdbsx usage.
>
> ~Andrew
>
>> ---
>>   xen/arch/x86/debug.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
>> index a67a192..ba6a64d 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 v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error.

[Mukesh Rathor]

On Tue,  7 Jan 2014 19:25:48 -0500
Don Slutz <dslutz@verizon.com> wrote:

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

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

[Mukesh Rathor]

On Wed, 8 Jan 2014 00:55:32 +0000
Andrew Cooper <andrew.cooper3@citrix.com> 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.
> 
> It should probably also include your Tested-by: tag

After above changes:

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

> 
> Ian (with RM hat on):
>   This is a hypervisor reference counting error on a toolstack
> hypercall path.  Irrespective of any of the other patches in this
> series, I think this should be included ASAP (although probably
> subject to review from a third person), which will fix the hypervisor
> crashes from gdbsx usage. 
> ~Andrew
> 
> > ---
> >  xen/arch/x86/debug.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> > index a67a192..ba6a64d 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;
> >  }
> >  
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Andrew Cooper]

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

Re: [PATCH v2 1/5] Add Emacs local variables to source files.

[Mukesh Rathor]

On Tue,  7 Jan 2014 19:25:44 -0500
Don Slutz <dslutz@verizon.com> wrote:

> These 2 files are changed in this patch set.  So add the allowed
> "Emacs local variables" from CODING_STYLE.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Will let some emacs user ack these... "vim" rules!!

Mukesh

> ---
>  tools/debugger/gdbsx/xg/xg_main.c | 8 ++++++++
>  xen/arch/x86/debug.c              | 8 ++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/tools/debugger/gdbsx/xg/xg_main.c
> b/tools/debugger/gdbsx/xg/xg_main.c index 5736b86..0622ebd 100644
> --- a/tools/debugger/gdbsx/xg/xg_main.c
> +++ b/tools/debugger/gdbsx/xg/xg_main.c
> @@ -821,3 +821,11 @@ xg_write_mem(uint64_t guestva, char *frombuf,
> int buflen, uint64_t pgd3val) return iop->remain;
>  }
>  
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index 3e21ca8..a67a192 100644
> --- a/xen/arch/x86/debug.c
> +++ b/xen/arch/x86/debug.c
> @@ -223,3 +223,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: [BUGFIX][PATCH v2 4/5] xg_read_mem: Report on error.

[Mukesh Rathor]

On Tue,  7 Jan 2014 19:25:47 -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.
> 
> Acked-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> Signed-off-by: Don Slutz <dslutz@verizon.com>

I was told the acked line must come after the sob.  jfyi.

Mukesh

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

Re: [PATCH v2 1/5] Add Emacs local variables to source files.

[Andrew Cooper]

On 08/01/2014 01:16, Mukesh Rathor wrote:
> On Tue,  7 Jan 2014 19:25:44 -0500
> Don Slutz <dslutz@verizon.com> wrote:
>
>> These 2 files are changed in this patch set.  So add the allowed
>> "Emacs local variables" from CODING_STYLE.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> Will let some emacs user ack these... "vim" rules!!
>
> Mukesh

Wasn't there a patch a little while back to add equivalent vim rules to
each of the files (or at least a thread suggesting a patch)?  In the
interest of cooperation, it is only fair.

As a fellow emacsian,

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

As for 4.4-ness:  If the rest of the series is deemed ok for a
release-ack, then this should go in for completeness.  If part of the
series is decided to be deferred, then this warrants deferring as well.

~Andrew

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Mukesh Rathor]

On Tue,  7 Jan 2014 19:25:46 -0500
Don Slutz <dslutz@verizon.com> wrote:

> If dbg_debug is non-zero, output debug.
> 
> Include put_gfn debug logging.
> 
> Here is a smaple output at dbg_debug == 2:
> 
> (XEN) [2014-01-07 03:20:09] gmem:addr:8f56 buf:00000000006e2020
> len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000 (XEN) [2014-01-07
> 03:20:09] vaddr:8f56 domid:1 (XEN) [2014-01-07 03:20:09] X:
> vaddr:8f56 domid:1 mfn:64331a (XEN) [2014-01-07 03:20:09] R:
> addr:8f56 pagecnt=1 domid:1 gfn:8 (XEN) [2014-01-07 03:20:09]
> gmem:exit:len:$0 (XEN) [2014-01-07 03:20:09] gmem:addr:8f57
> buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
> (XEN) [2014-01-07 03:20:09] vaddr:8f57 domid:1 (XEN) [2014-01-07
> 03:20:09] X: vaddr:8f57 domid:1 mfn:64331a (XEN) [2014-01-07
> 03:20:09] R: addr:8f57 pagecnt=1 domid:1 gfn:8 (XEN) [2014-01-07
> 03:20:09] gmem:exit:len:$0 (XEN) [2014-01-07 03:20:09]
> gmem:addr:6ae9168b buf:00000000006e2020 len:$2 domid:1 toaddr:0
> dp:ffff83083e5fe000 (XEN) [2014-01-07 03:20:09] vaddr:6ae9168b
> domid:1 (XEN) [2014-01-07 03:20:09] X: vaddr:6ae9168b domid:1
> mfn:ffffffffffffffff (XEN) [2014-01-07 03:20:09] R: domid:1 gfn:6ae91
> (XEN) [2014-01-07 03:20:09] gmem:exit:len:$2
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

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

> ---
>  xen/arch/x86/debug.c | 54
> +++++++++++++++++++++++++--------------------------- 1 file changed,
> 26 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index ba6a64d..777e5ba 100644
> --- a/xen/arch/x86/debug.c
> +++ b/xen/arch/x86/debug.c
> @@ -30,16 +30,9 @@
>   * gdbsx, etc..
>   */
>  
> -#ifdef XEN_KDB_CONFIG
> -#include "../kdb/include/kdbdefs.h"
> -#include "../kdb/include/kdbproto.h"
> -#define DBGP(...) {(kdbdbg) ? kdbp(__VA_ARGS__):0;}
> -#define DBGP1(...) {(kdbdbg>1) ? kdbp(__VA_ARGS__):0;}
> -#define DBGP2(...) {(kdbdbg>2) ? kdbp(__VA_ARGS__):0;}
> -#else
> -#define DBGP1(...) ((void)0)
> -#define DBGP2(...) ((void)0)
> -#endif
> +static volatile int dbg_debug;
> +#define DBGP(...) {(dbg_debug) ? printk(__VA_ARGS__) : 0;}
> +#define DBGP1(...) {(dbg_debug > 1) ? printk(__VA_ARGS__) : 0;}
>  
>  /* Returns: mfn for the given (hvm guest) vaddr */
>  static unsigned long 
> @@ -50,27 +43,28 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp,
> int toaddr, uint32_t pfec = PFEC_page_present;
>      p2m_type_t gfntype;
>  
> -    DBGP2("vaddr:%lx domid:%d\n", vaddr, dp->domain_id);
> +    DBGP1("vaddr:%lx domid:%d\n", vaddr, dp->domain_id);
>  
>      *gfn = paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec);
>      if ( *gfn == INVALID_GFN )
>      {
> -        DBGP2("kdb:bad gfn from gva_to_gfn\n");
> +        DBGP1("kdb:bad gfn from gva_to_gfn\n");
>          return INVALID_MFN;
>      }
>  
>      mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); 
>      if ( p2m_is_readonly(gfntype) && toaddr )
>      {
> -        DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
> +        DBGP1("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
>          mfn = INVALID_MFN;
>      }
>  
> -    DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id,
> mfn);
> +    DBGP1("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id,
> mfn); 
>      if ( mfn == INVALID_MFN )
>      {
>          put_gfn(dp, *gfn);
> +        DBGP1("R: domid:%d gfn:%lx\n", dp->domain_id, *gfn);
>          *gfn = INVALID_GFN;
>      }
>  
> @@ -100,7 +94,7 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp,
> uint64_t pgd3val) unsigned long cr3 = (pgd3val ? pgd3val :
> dp->vcpu[0]->arch.cr3); unsigned long mfn = cr3 >> PAGE_SHIFT;
>  
> -    DBGP2("vaddr:%lx domid:%d cr3:%lx pgd3:%lx\n", vaddr,
> dp->domain_id, 
> +    DBGP1("vaddr:%lx domid:%d cr3:%lx pgd3:%lx\n", vaddr,
> dp->domain_id, cr3, pgd3val);
>  
>      if ( pgd3val == 0 )
> @@ -109,11 +103,11 @@ 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);
> +        DBGP1("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);
> +            DBGP("l4 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr,
> cr3); return INVALID_MFN;
>          }
>  
> @@ -121,12 +115,12 @@ 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);
> +        DBGP1("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) )
>          {
> -            DBGP1("l3 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr,
> cr3);
> +            DBGP("l3 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr,
> cr3); return INVALID_MFN;
>          }
>      }
> @@ -135,20 +129,20 @@ 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);
> +    DBGP1("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) )
>      {
> -        DBGP1("l2 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr,
> cr3);
> +        DBGP("l2 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
>          return INVALID_MFN;
>      }
>      l1t = map_domain_page(mfn);
>      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);
> +    DBGP1("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;
>  }
> @@ -186,7 +180,11 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf,
> int len, struct domain *dp, 
>          unmap_domain_page(va);
>          if ( gfn != INVALID_GFN )
> +        {
>              put_gfn(dp, gfn);
> +            DBGP1("R: addr:%lx pagecnt=%ld domid:%d gfn:%lx\n",
> +                  addr, pagecnt, dp->domain_id, gfn);
> +        }
>  
>          addr += pagecnt;
>          buf += pagecnt;
> @@ -210,7 +208,7 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len,
> domid_t domid, int toaddr, struct domain *dp =
> get_domain_by_id(domid); int hyp = (domid == DOMID_IDLE);
>  
> -    DBGP2("gmem:addr:%lx buf:%p len:$%d domid:%x toaddr:%x dp:%p\n", 
> +    DBGP1("gmem:addr:%lx buf:%p len:$%d domid:%x toaddr:%x dp:%p\n",
>            addr, buf, len, domid, toaddr, dp);
>      if ( hyp )
>      {
> @@ -226,7 +224,7 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len,
> domid_t domid, int toaddr, put_domain(dp);
>      }
>  
> -    DBGP2("gmem:exit:len:$%d\n", len);
> +    DBGP1("gmem:exit:len:$%d\n", len);
>      return len;
>  }
>  

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

[Mukesh Rathor]

On Wed, 8 Jan 2014 00:55:32 +0000
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 08/01/2014 00:25, Don Slutz wrote:
> > Using a 1G hvm domU (in grub) and gdbsx:
> >
..... 

> 
> Ian (with RM hat on):
>   This is a hypervisor reference counting error on a toolstack
> hypercall path.  Irrespective of any of the other patches in this
> series, I think this should be included ASAP (although probably
> subject to review from a third person), which will fix the hypervisor
> crashes from gdbsx usage.

I remember long ago mentioning to our packaing team to make gdbsx
root executible only. 

What would be a good place to document that gdbsx should be removed from
production systems, and/or be made root executible only?

thanks
mukesh

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

[Andrew Cooper]

On 08/01/2014 01:44, Mukesh Rathor wrote:
> On Wed, 8 Jan 2014 00:55:32 +0000
> Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
>> On 08/01/2014 00:25, Don Slutz wrote:
>>> Using a 1G hvm domU (in grub) and gdbsx:
>>>
> ..... 
>
>> Ian (with RM hat on):
>>   This is a hypervisor reference counting error on a toolstack
>> hypercall path.  Irrespective of any of the other patches in this
>> series, I think this should be included ASAP (although probably
>> subject to review from a third person), which will fix the hypervisor
>> crashes from gdbsx usage.
> I remember long ago mentioning to our packaing team to make gdbsx
> root executible only. 
>
> What would be a good place to document that gdbsx should be removed from
> production systems, and/or be made root executible only?
>
> thanks
> mukesh
>
>

[root@idol ~]# ls -la /dev/xen/privcmd
crw-rw---- 1 root root 10, 57 Jan  7 11:48 /dev/xen/privcmd

As currently stands (Linux 3.10), only root can open privcmd and issue
ioctls, so a non-root gdbsx process would presumably not function at
all.  I am not sure that any documentation needs updating.

Having said that, with my "future ventures into reducing required dom0
priveleges" hat on, it would be very nice for a subset of hypercalls to
be available in a non-privileged, read-only form.  This would allow
read-only information from xl (and xentop and suchlike) to be available
to non-root users in dom0.

On the other hand, anyone with shell access in dom0 is likely a system
administrator anyway, so will almost certainly be running with sudo
privileges (or as root) anyway.

~Andrew

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

[Mukesh Rathor]

On Wed, 8 Jan 2014 02:30:24 +0000
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 08/01/2014 01:44, Mukesh Rathor wrote:
> > On Wed, 8 Jan 2014 00:55:32 +0000
> > Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> >> On 08/01/2014 00:25, Don Slutz wrote:
> >>> Using a 1G hvm domU (in grub) and gdbsx:
> >>>
> > ..... 
> >
> >> Ian (with RM hat on):
> >>   This is a hypervisor reference counting error on a toolstack
> >> hypercall path.  Irrespective of any of the other patches in this
> >> series, I think this should be included ASAP (although probably
> >> subject to review from a third person), which will fix the
> >> hypervisor crashes from gdbsx usage.
> > I remember long ago mentioning to our packaing team to make gdbsx
> > root executible only. 
> >
> > What would be a good place to document that gdbsx should be removed
> > from production systems, and/or be made root executible only?
> >
> > thanks
> > mukesh
> >
> >
> 
> [root@idol ~]# ls -la /dev/xen/privcmd
> crw-rw---- 1 root root 10, 57 Jan  7 11:48 /dev/xen/privcmd
> 
> As currently stands (Linux 3.10), only root can open privcmd and issue
> ioctls, so a non-root gdbsx process would presumably not function at
> all.  I am not sure that any documentation needs updating.

Ah, right. I remember now...  thats much better. At least, currently its
not compromised with anyone able to run it.

thanks
Mukesh

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

[Jan Beulich]

>>> On 08.01.14 at 01:25, Don Slutz <dslutz@verizon.com> wrote:
> 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

Which clearly shows that the series is badly ordered: You shouldn't
expect committers to know (or even have to guess) that applying
later patches without earlier ones is okay. I.e. if you think that
leaving out part of the series for 4.4 is fine, you should place the
required ones first, the optional ones second, and the 4.5 ones
last. Or, if the patches are in fact independent, another option
would be to not send the patches as a series in the first place.

Jan

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

[Jan Beulich]

>>> On 08.01.14 at 01:25, Don Slutz <dslutz@verizon.com> wrote:
> --- 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);

With the flow change above, this should be moved into an "else"
to the earlier "if".

Jan

> +
> +    if ( mfn == INVALID_MFN )
> +    {
> +        put_gfn(dp, *gfn);
> +        *gfn = INVALID_GFN;
> +    }
> +
>      return mfn;
>  }

Re: [PATCH v2 1/5] Add Emacs local variables to source files.

[Ian Campbell]

On Wed, 2014-01-08 at 01:27 +0000, Andrew Cooper wrote:
> On 08/01/2014 01:16, Mukesh Rathor wrote:
> > On Tue,  7 Jan 2014 19:25:44 -0500
> > Don Slutz <dslutz@verizon.com> wrote:
> >
> >> These 2 files are changed in this patch set.  So add the allowed
> >> "Emacs local variables" from CODING_STYLE.
> >>
> >> Signed-off-by: Don Slutz <dslutz@verizon.com>
> > Will let some emacs user ack these... "vim" rules!!
> >
> > Mukesh
> 
> Wasn't there a patch a little while back to add equivalent vim rules to
> each of the files (or at least a thread suggesting a patch)?  In the
> interest of cooperation, it is only fair.

IIRC it had some shortcoming, like requiring vim users to always invoke
vim from the top-level xen source directory?

For my part I would be more than happy to accept vim magic runes into
anything which I'm the maintainer for (despite being an emacs type!).

> 
> As a fellow emacsian,

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> As for 4.4-ness:  If the rest of the series is deemed ok for a
> release-ack, then this should go in for completeness.  If part of the
> series is decided to be deferred, then this warrants deferring as well.

I don't think there is any reason to hold off on a change like this
irrespective of what happens to the rest of the series.

Release-acked-by: Ian Campbell

Ian.

Re: [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error.

[Ian Campbell]

On Tue, 2014-01-07 at 19:25 -0500, Don Slutz wrote:
> 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>
> ---
>  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);

Is it worth printing the expect number (i.e. the input) of bytes? Is
that buflen here?

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

Same here.

> +        return buflen;
> +    }
>      return iop->remain;
>  }
>  

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Ian Campbell]

On Tue, 2014-01-07 at 19:25 -0500, Don Slutz wrote:
> If dbg_debug is non-zero, output debug.
> 
> Include put_gfn debug logging.
> 
> Here is a smaple output at dbg_debug == 2:
> 
> (XEN) [2014-01-07 03:20:09] gmem:addr:8f56 buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
> (XEN) [2014-01-07 03:20:09] vaddr:8f56 domid:1
> (XEN) [2014-01-07 03:20:09] X: vaddr:8f56 domid:1 mfn:64331a
> (XEN) [2014-01-07 03:20:09] R: addr:8f56 pagecnt=1 domid:1 gfn:8
> (XEN) [2014-01-07 03:20:09] gmem:exit:len:$0
> (XEN) [2014-01-07 03:20:09] gmem:addr:8f57 buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
> (XEN) [2014-01-07 03:20:09] vaddr:8f57 domid:1
> (XEN) [2014-01-07 03:20:09] X: vaddr:8f57 domid:1 mfn:64331a
> (XEN) [2014-01-07 03:20:09] R: addr:8f57 pagecnt=1 domid:1 gfn:8
> (XEN) [2014-01-07 03:20:09] gmem:exit:len:$0
> (XEN) [2014-01-07 03:20:09] gmem:addr:6ae9168b buf:00000000006e2020 len:$2 domid:1 toaddr:0 dp:ffff83083e5fe000
> (XEN) [2014-01-07 03:20:09] vaddr:6ae9168b domid:1
> (XEN) [2014-01-07 03:20:09] X: vaddr:6ae9168b domid:1 mfn:ffffffffffffffff
> (XEN) [2014-01-07 03:20:09] R: domid:1 gfn:6ae91
> (XEN) [2014-01-07 03:20:09] gmem:exit:len:$2
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  xen/arch/x86/debug.c | 54 +++++++++++++++++++++++++---------------------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index ba6a64d..777e5ba 100644
> --- a/xen/arch/x86/debug.c
> +++ b/xen/arch/x86/debug.c
> @@ -30,16 +30,9 @@
>   * gdbsx, etc..
>   */
>  
> -#ifdef XEN_KDB_CONFIG
> -#include "../kdb/include/kdbdefs.h"
> -#include "../kdb/include/kdbproto.h"
> -#define DBGP(...) {(kdbdbg) ? kdbp(__VA_ARGS__):0;}
> -#define DBGP1(...) {(kdbdbg>1) ? kdbp(__VA_ARGS__):0;}
> -#define DBGP2(...) {(kdbdbg>2) ? kdbp(__VA_ARGS__):0;}
> -#else
> -#define DBGP1(...) ((void)0)
> -#define DBGP2(...) ((void)0)
> -#endif
> +static volatile int dbg_debug;

Using volatile is almost always wrong. Why do you think it is needed
here?

If anything this variable is exactly the opposite, i..e __read_mostly or
even const (given that I can't see anything which writes it I suppose
this is a compile time setting?)

Ian.

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

[Ian Campbell]

On Wed, 2014-01-08 at 00:55 +0000, Andrew Cooper wrote:

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

If you are the author then the first line of the mail should also be

From: Andrew Cooper <andre...@citrix.com>

Don, if you git commit --author='....' (or --amend to fixup an existing
commit) then git send-email will do the right thing.

> Ian (with RM hat on):
>   This is a hypervisor reference counting error on a toolstack hypercall
> path.  Irrespective of any of the other patches in this series, I think
> this should be included ASAP (although probably subject to review from a
> third person), which will fix the hypervisor crashes from gdbsx usage.

I've already given this stuff a release Ack, subject to Muckesh
approving it and someone saying for sure that this patch can only affect
debuggers.

Ian.

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

[Don Slutz]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

On 01/08/14 03:36, Jan Beulich wrote:
>>>> On 08.01.14 at 01:25, Don Slutz <dslutz@verizon.com> wrote:
>> --- 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);
> With the flow change above, this should be moved into an "else"
> to the earlier "if".

Ok.  I have done this and tested it.  (v3 attached).

Note: this means that patch v2 3/5 will not cleanly apply.  I am in the process of fixing and testing v3 of that.  Andrew Cooper in

http://lists.xen.org/archives/html/xen-devel/2014-01/msg00595.html

Says that this one patch "should be included ASAP"; which is why I am sending this 1st.

I did not add:

Acked-by: Mukesh Rathor ...

Even though my understanding is that I could have (please let me know if this is wrong) based on:

http://lists.xen.org/archives/html/xen-devel/2014-01/msg00601.html

since the v3 change is very minor (like a comment change).  This is because DBGP2:

#define DBGP2(...) ((void)0)

does nothing.  And if changed to do something, the file fails to compile without more changes (I.E. patch 3/5).  Even though I knew this does nothing I has compiled and run this code.


I did change the author to "Andrew Cooper <andrew.cooper3@citrix.com>" since he provided most of this change. (see

http://lists.xen.org/archives/html/xen-devel/2014-01/msg00631.html

)  Not sure that is correct since we have now both made changes.

    -Don Slutz



> Jan
>
>> +
>> +    if ( mfn == INVALID_MFN )
>> +    {
>> +        put_gfn(dp, *gfn);
>> +        *gfn = INVALID_GFN;
>> +    }
>> +
>>       return mfn;
>>   }
>


[-- Attachment #2: 0001-dbg_rw_guest_mem-need-to-call-put_gfn-in-error-path.patch --]
[-- Type: text/x-patch, Size: 3069 bytes --]

>From 1c8ebc3cbc4f415c5942b32736ecb58ef9c2cc14 Mon Sep 17 00:00:00 2001
From: Andrew Cooper <andrew.cooper3@citrix.com>
Date: Fri, 3 Jan 2014 16:12:20 -0500
Subject: [BUGFIX][PATCH v3] dbg_rw_guest_mem: need to call put_gfn in error
 path.

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

Changes v2 to v3:
  Jan Beulich: Fix flow change (added an else to DBGP2).
  Ian Campbell: Fix author  

 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


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

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

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

[Don Slutz]

On 01/08/14 05:40, Ian Campbell wrote:
> On Wed, 2014-01-08 at 00:55 +0000, Andrew Cooper wrote:
>
>>> 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)
> If you are the author then the first line of the mail should also be
>
> From: Andrew Cooper <andre...@citrix.com>
>
> Don, if you git commit --author='....' (or --amend to fixup an existing
> commit) then git send-email will do the right thing.

I have taken my best shot at doing the right thing here in different email.

>> Ian (with RM hat on):
>>    This is a hypervisor reference counting error on a toolstack hypercall
>> path.  Irrespective of any of the other patches in this series, I think
>> this should be included ASAP (although probably subject to review from a
>> third person), which will fix the hypervisor crashes from gdbsx usage.
> I've already given this stuff a release Ack, subject to Muckesh
> approving it and someone saying for sure that this patch can only affect
> debuggers.

For what it is worth, I will say that this patch can only affect debuggers.

     -Don Slutz


> Ian.
>
>

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Don Slutz]

On 01/08/14 05:38, Ian Campbell wrote:
> On Tue, 2014-01-07 at 19:25 -0500, Don Slutz wrote:
>> If dbg_debug is non-zero, output debug.
>>
>> Include put_gfn debug logging.
>>
>> Here is a smaple output at dbg_debug == 2:
>>
>> (XEN) [2014-01-07 03:20:09] gmem:addr:8f56 buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
>> (XEN) [2014-01-07 03:20:09] vaddr:8f56 domid:1
>> (XEN) [2014-01-07 03:20:09] X: vaddr:8f56 domid:1 mfn:64331a
>> (XEN) [2014-01-07 03:20:09] R: addr:8f56 pagecnt=1 domid:1 gfn:8
>> (XEN) [2014-01-07 03:20:09] gmem:exit:len:$0
>> (XEN) [2014-01-07 03:20:09] gmem:addr:8f57 buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
>> (XEN) [2014-01-07 03:20:09] vaddr:8f57 domid:1
>> (XEN) [2014-01-07 03:20:09] X: vaddr:8f57 domid:1 mfn:64331a
>> (XEN) [2014-01-07 03:20:09] R: addr:8f57 pagecnt=1 domid:1 gfn:8
>> (XEN) [2014-01-07 03:20:09] gmem:exit:len:$0
>> (XEN) [2014-01-07 03:20:09] gmem:addr:6ae9168b buf:00000000006e2020 len:$2 domid:1 toaddr:0 dp:ffff83083e5fe000
>> (XEN) [2014-01-07 03:20:09] vaddr:6ae9168b domid:1
>> (XEN) [2014-01-07 03:20:09] X: vaddr:6ae9168b domid:1 mfn:ffffffffffffffff
>> (XEN) [2014-01-07 03:20:09] R: domid:1 gfn:6ae91
>> (XEN) [2014-01-07 03:20:09] gmem:exit:len:$2
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   xen/arch/x86/debug.c | 54 +++++++++++++++++++++++++---------------------------
>>   1 file changed, 26 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
>> index ba6a64d..777e5ba 100644
>> --- a/xen/arch/x86/debug.c
>> +++ b/xen/arch/x86/debug.c
>> @@ -30,16 +30,9 @@
>>    * gdbsx, etc..
>>    */
>>   
>> -#ifdef XEN_KDB_CONFIG
>> -#include "../kdb/include/kdbdefs.h"
>> -#include "../kdb/include/kdbproto.h"
>> -#define DBGP(...) {(kdbdbg) ? kdbp(__VA_ARGS__):0;}
>> -#define DBGP1(...) {(kdbdbg>1) ? kdbp(__VA_ARGS__):0;}
>> -#define DBGP2(...) {(kdbdbg>2) ? kdbp(__VA_ARGS__):0;}
>> -#else
>> -#define DBGP1(...) ((void)0)
>> -#define DBGP2(...) ((void)0)
>> -#endif
>> +static volatile int dbg_debug;
> Using volatile is almost always wrong. Why do you think it is needed
> here?

This was from Mukesh Rathor:

http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html

I saw no reason to make it volatile, but maybe "kdb" needs this? Happy to change any way you want.

> If anything this variable is exactly the opposite, i..e __read_mostly or
> even const (given that I can't see anything which writes it I suppose
> this is a compile time setting?)

That has been how I have been testing it so far (changing the source to set values).  Mukesh claims to be able to change it at will.  Not sure how const may effect this.

       -Don Slutz

> Ian.
>

Re: [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error.

[Don Slutz]

On 01/08/14 05:35, Ian Campbell wrote:
> On Tue, 2014-01-07 at 19:25 -0500, Don Slutz wrote:
>> 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>
>> ---
>>   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);
> Is it worth printing the expect number (i.e. the input) of bytes? Is
> that buflen here?

Not in my testing.  If I turn on debug output (-d), I get:


...
process_remote_request:E:m8957006a,1 curvcpu:0
xg_read_mem:E:gva:8957006a tobuf:1c81020 len:1
ERROR:xg_read_mem:ERROR: failed to read bytes. errno:14 rc:-1
xg_read_mem:X:remain:0 buf8:0x24
process_m_request:Failed read mem. addr:0x8957006a len:1 remn:1 errno:14
process_remote_request:X:E01 curvcpu:0
...


Which outputs the length (1) a few times already.   The expected number here is tobuf_len.


>> +        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);
> Same here.

Since this error message is output without other context, it might help.   Most of the time, the user knows the write size requested. Since this code has an ACK from Mukesh, I lean to not making a change, but if you want it, I will.

    -Don Slutz

>> +        return buflen;
>> +    }
>>       return iop->remain;
>>   }
>>   
>

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

[Don Slutz]

On 01/08/14 03:28, Jan Beulich wrote:
>>>> On 08.01.14 at 01:25, Don Slutz <dslutz@verizon.com> wrote:
>> 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
> Which clearly shows that the series is badly ordered: You shouldn't
> expect committers to know (or even have to guess) that applying
> later patches without earlier ones is okay. I.e. if you think that
> leaving out part of the series for 4.4 is fine, you should place the
> required ones first, the optional ones second, and the 4.5 ones
> last. Or, if the patches are in fact independent, another option
> would be to not send the patches as a series in the first place.

If this was not real close to the release date, I would not have added this information.  Clearly the way I wrote it is not the way it should be expressed.   Thanks for you help, I will try to do that ordering in the future.

    -Don Slutz


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

Re: [PATCH v2 1/5] Add Emacs local variables to source files.

[Ian Campbell]

On Wed, 2014-01-08 at 09:51 +0000, Ian Campbell wrote:
> On Wed, 2014-01-08 at 01:27 +0000, Andrew Cooper wrote:
> > 
> > As a fellow emacsian,
> 
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Thanks.
> 
> > As for 4.4-ness:  If the rest of the series is deemed ok for a
> > release-ack, then this should go in for completeness.  If part of the
> > series is decided to be deferred, then this warrants deferring as well.
> 
> I don't think there is any reason to hold off on a change like this
> irrespective of what happens to the rest of the series.

I've just applied it.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Ian Campbell]

On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > Using volatile is almost always wrong. Why do you think it is needed
> > here?
> 
> This was from Mukesh Rathor:
> 
> http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> 
> I saw no reason to make it volatile, but maybe "kdb" needs this? Happy
> to change any way you want.

I'm not the maintainer but if I were I'd say drop the volatile and maybe
mark it __read_mostly and perhaps __used too (since it's static it might
otherwise get eliminated).

> > If anything this variable is exactly the opposite, i..e __read_mostly or
> > even const (given that I can't see anything which writes it I suppose
> > this is a compile time setting?)
> 
> That has been how I have been testing it so far (changing the source
> to set values).  Mukesh claims to be able to change it at will.  Not
> sure how const may effect this.

I presume this is using kdb and marking it const would probably cause
the dead code to be eliminated so it is worth not doing that for his
sake.

Ian.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Tim Deegan]

At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > Using volatile is almost always wrong. Why do you think it is needed
> > > here?
> > 
> > This was from Mukesh Rathor:
> > 
> > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > 
> > I saw no reason to make it volatile, but maybe "kdb" needs this? Happy
> > to change any way you want.
> 
> I'm not the maintainer but if I were I'd say drop the volatile and maybe
> mark it __read_mostly and perhaps __used too (since it's static it might
> otherwise get eliminated).
> 
> > > If anything this variable is exactly the opposite, i..e __read_mostly or
> > > even const (given that I can't see anything which writes it I suppose
> > > this is a compile time setting?)
> > 
> > That has been how I have been testing it so far (changing the source
> > to set values).  Mukesh claims to be able to change it at will.  Not
> > sure how const may effect this.

If the idea is to use kdb itself to frob the value, then it does need
something to stop the compiler caching it.  This might even be one of
the few cases where 'volatile' actually DTRT; it would still be more
in keeping with Xen style to use an explicit read op (like
atomic_read()) where the value is consumed.

Tim.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Ian Campbell]

On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
> At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> > On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > > Using volatile is almost always wrong. Why do you think it is needed
> > > > here?
> > > 
> > > This was from Mukesh Rathor:
> > > 
> > > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > > 
> > > I saw no reason to make it volatile, but maybe "kdb" needs this? Happy
> > > to change any way you want.
> > 
> > I'm not the maintainer but if I were I'd say drop the volatile and maybe
> > mark it __read_mostly and perhaps __used too (since it's static it might
> > otherwise get eliminated).
> > 
> > > > If anything this variable is exactly the opposite, i..e __read_mostly or
> > > > even const (given that I can't see anything which writes it I suppose
> > > > this is a compile time setting?)
> > > 
> > > That has been how I have been testing it so far (changing the source
> > > to set values).  Mukesh claims to be able to change it at will.  Not
> > > sure how const may effect this.
> 
> If the idea is to use kdb itself to frob the value, then it does need
> something to stop the compiler caching it.  This might even be one of
> the few cases where 'volatile' actually DTRT; it would still be more
> in keeping with Xen style to use an explicit read op (like
> atomic_read()) where the value is consumed.

Is there any need to be asynchronously frobbing this value in the middle
of a function within this file and expecting it to be reliable? I'd have
thought that changing the value and having it take affect on the next
debug event/hypercall/whatever would be what was wanted.

Ian.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Tim Deegan]

At 17:44 +0000 on 08 Jan (1389199462), Ian Campbell wrote:
> On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
> > At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> > > On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > > > Using volatile is almost always wrong. Why do you think it is needed
> > > > > here?
> > > > 
> > > > This was from Mukesh Rathor:
> > > > 
> > > > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > > > 
> > > > I saw no reason to make it volatile, but maybe "kdb" needs this? Happy
> > > > to change any way you want.
> > > 
> > > I'm not the maintainer but if I were I'd say drop the volatile and maybe
> > > mark it __read_mostly and perhaps __used too (since it's static it might
> > > otherwise get eliminated).
> > > 
> > > > > If anything this variable is exactly the opposite, i..e __read_mostly or
> > > > > even const (given that I can't see anything which writes it I suppose
> > > > > this is a compile time setting?)
> > > > 
> > > > That has been how I have been testing it so far (changing the source
> > > > to set values).  Mukesh claims to be able to change it at will.  Not
> > > > sure how const may effect this.
> > 
> > If the idea is to use kdb itself to frob the value, then it does need
> > something to stop the compiler caching it.  This might even be one of
> > the few cases where 'volatile' actually DTRT; it would still be more
> > in keeping with Xen style to use an explicit read op (like
> > atomic_read()) where the value is consumed.
> 
> Is there any need to be asynchronously frobbing this value in the middle
> of a function within this file and expecting it to be reliable? I'd have
> thought that changing the value and having it take affect on the next
> debug event/hypercall/whatever would be what was wanted.

The variable is static and there's nothing in the file that updates
it, so the compiler might drop it entirely.  Maybe __used__ would be
good enough to stop the compiler dropping all reads, but I'm not sure.

Tim.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Mukesh Rathor]

On Wed, 8 Jan 2014 17:44:22 +0000
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
> > At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> > > On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > > > Using volatile is almost always wrong. Why do you think it is
> > > > > needed here?
> > > > 
> > > > This was from Mukesh Rathor:
> > > > 
> > > > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > > > 
> > > > I saw no reason to make it volatile, but maybe "kdb" needs
> > > > this? Happy to change any way you want.
> > > 
> > > I'm not the maintainer but if I were I'd say drop the volatile
> > > and maybe mark it __read_mostly and perhaps __used too (since
> > > it's static it might otherwise get eliminated).
> > > 
> > > > > If anything this variable is exactly the opposite, i..e
> > > > > __read_mostly or even const (given that I can't see anything
> > > > > which writes it I suppose this is a compile time setting?)
> > > > 
> > > > That has been how I have been testing it so far (changing the
> > > > source to set values).  Mukesh claims to be able to change it
> > > > at will.  Not sure how const may effect this.
> > 
> > If the idea is to use kdb itself to frob the value, then it does
> > need something to stop the compiler caching it.  This might even be
> > one of the few cases where 'volatile' actually DTRT; it would still
> > be more in keeping with Xen style to use an explicit read op (like
> > atomic_read()) where the value is consumed.
> 
> Is there any need to be asynchronously frobbing this value in the
> middle of a function within this file and expecting it to be

Yes. I can stop the machine via kdb or other debugger, change the value
during debug, and upon resuming it will start printing stuff. Often
this is needed when going thru several iterations of call before problem
is seen. Making it volatile makes sure the compiler loads it every
instance of its use. This is not in main path, only debugger path, so
the overhead should not be an issue.

thanks,
mukesh

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Ian Campbell]

On Wed, 2014-01-08 at 19:10 +0100, Tim Deegan wrote:
> At 17:44 +0000 on 08 Jan (1389199462), Ian Campbell wrote:
> > On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
> > > At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> > > > On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > > > > Using volatile is almost always wrong. Why do you think it is needed
> > > > > > here?
> > > > > 
> > > > > This was from Mukesh Rathor:
> > > > > 
> > > > > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > > > > 
> > > > > I saw no reason to make it volatile, but maybe "kdb" needs this? Happy
> > > > > to change any way you want.
> > > > 
> > > > I'm not the maintainer but if I were I'd say drop the volatile and maybe
> > > > mark it __read_mostly and perhaps __used too (since it's static it might
> > > > otherwise get eliminated).
> > > > 
> > > > > > If anything this variable is exactly the opposite, i..e __read_mostly or
> > > > > > even const (given that I can't see anything which writes it I suppose
> > > > > > this is a compile time setting?)
> > > > > 
> > > > > That has been how I have been testing it so far (changing the source
> > > > > to set values).  Mukesh claims to be able to change it at will.  Not
> > > > > sure how const may effect this.
> > > 
> > > If the idea is to use kdb itself to frob the value, then it does need
> > > something to stop the compiler caching it.  This might even be one of
> > > the few cases where 'volatile' actually DTRT; it would still be more
> > > in keeping with Xen style to use an explicit read op (like
> > > atomic_read()) where the value is consumed.
> > 
> > Is there any need to be asynchronously frobbing this value in the middle
> > of a function within this file and expecting it to be reliable? I'd have
> > thought that changing the value and having it take affect on the next
> > debug event/hypercall/whatever would be what was wanted.
> 
> The variable is static and there's nothing in the file that updates
> it, so the compiler might drop it entirely.  Maybe __used__ would be
> good enough to stop the compiler dropping all reads, but I'm not sure.

Isn't that exactly what __used (or __used__) is for?

Ian.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Ian Campbell]

On Wed, 2014-01-08 at 16:38 -0800, Mukesh Rathor wrote:
> On Wed, 8 Jan 2014 17:44:22 +0000
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
> > > At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> > > > On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > > > > Using volatile is almost always wrong. Why do you think it is
> > > > > > needed here?
> > > > > 
> > > > > This was from Mukesh Rathor:
> > > > > 
> > > > > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > > > > 
> > > > > I saw no reason to make it volatile, but maybe "kdb" needs
> > > > > this? Happy to change any way you want.
> > > > 
> > > > I'm not the maintainer but if I were I'd say drop the volatile
> > > > and maybe mark it __read_mostly and perhaps __used too (since
> > > > it's static it might otherwise get eliminated).
> > > > 
> > > > > > If anything this variable is exactly the opposite, i..e
> > > > > > __read_mostly or even const (given that I can't see anything
> > > > > > which writes it I suppose this is a compile time setting?)
> > > > > 
> > > > > That has been how I have been testing it so far (changing the
> > > > > source to set values).  Mukesh claims to be able to change it
> > > > > at will.  Not sure how const may effect this.
> > > 
> > > If the idea is to use kdb itself to frob the value, then it does
> > > need something to stop the compiler caching it.  This might even be
> > > one of the few cases where 'volatile' actually DTRT; it would still
> > > be more in keeping with Xen style to use an explicit read op (like
> > > atomic_read()) where the value is consumed.
> > 
> > Is there any need to be asynchronously frobbing this value in the
> > middle of a function within this file and expecting it to be
> 
> Yes. I can stop the machine via kdb or other debugger, change the value
> during debug, and upon resuming it will start printing stuff. Often
> this is needed when going thru several iterations of call before problem
> is seen. Making it volatile makes sure the compiler loads it every
> instance of its use. This is not in main path, only debugger path, so
> the overhead should not be an issue.

So you want to be able to toggle the value in between two immediately
adjacent debug print calls? While debugging the debugging infrastructure
itself? (using itself?).

I'm surprised that even works, but if you say so then OK.

Ian.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Tim Deegan]

At 08:41 +0000 on 09 Jan (1389253311), Ian Campbell wrote:
> On Wed, 2014-01-08 at 19:10 +0100, Tim Deegan wrote:
> > At 17:44 +0000 on 08 Jan (1389199462), Ian Campbell wrote:
> > > On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
> > > > At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> > > > > On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > > > > > Using volatile is almost always wrong. Why do you think it is needed
> > > > > > > here?
> > > > > > 
> > > > > > This was from Mukesh Rathor:
> > > > > > 
> > > > > > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > > > > > 
> > > > > > I saw no reason to make it volatile, but maybe "kdb" needs this? Happy
> > > > > > to change any way you want.
> > > > > 
> > > > > I'm not the maintainer but if I were I'd say drop the volatile and maybe
> > > > > mark it __read_mostly and perhaps __used too (since it's static it might
> > > > > otherwise get eliminated).
> > > > > 
> > > > > > > If anything this variable is exactly the opposite, i..e __read_mostly or
> > > > > > > even const (given that I can't see anything which writes it I suppose
> > > > > > > this is a compile time setting?)
> > > > > > 
> > > > > > That has been how I have been testing it so far (changing the source
> > > > > > to set values).  Mukesh claims to be able to change it at will.  Not
> > > > > > sure how const may effect this.
> > > > 
> > > > If the idea is to use kdb itself to frob the value, then it does need
> > > > something to stop the compiler caching it.  This might even be one of
> > > > the few cases where 'volatile' actually DTRT; it would still be more
> > > > in keeping with Xen style to use an explicit read op (like
> > > > atomic_read()) where the value is consumed.
> > > 
> > > Is there any need to be asynchronously frobbing this value in the middle
> > > of a function within this file and expecting it to be reliable? I'd have
> > > thought that changing the value and having it take affect on the next
> > > debug event/hypercall/whatever would be what was wanted.
> > 
> > The variable is static and there's nothing in the file that updates
> > it, so the compiler might drop it entirely.  Maybe __used__ would be
> > good enough to stop the compiler dropping all reads, but I'm not sure.
> 
> Isn't that exactly what __used (or __used__) is for?

The docs say that "the variable must be emitted even if it appears
that the variable is not referenced" but what that means for
individual accesses I don't know.  So, probably?  I would be inclined
to distruct the compiler here and just wrap the accesses in a
read_atomic.

Tim.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Don Slutz]

[-- Attachment #1: Type: text/plain, Size: 2539 bytes --]

On 01/09/14 04:59, Ian Campbell wrote:
> On Wed, 2014-01-08 at 16:38 -0800, Mukesh Rathor wrote:
>> On Wed, 8 Jan 2014 17:44:22 +0000
>> Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>
>>> On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
>>>> At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
>>>>> On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
>>>>>>> Using volatile is almost always wrong. Why do you think it is
>>>>>>> needed here?
>>>>>> This was from Mukesh Rathor:
>>>>>>
>>>>>> http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
>>>>>>
>>>>>> I saw no reason to make it volatile, but maybe "kdb" needs
>>>>>> this? Happy to change any way you want.
>>>>> I'm not the maintainer but if I were I'd say drop the volatile
>>>>> and maybe mark it __read_mostly and perhaps __used too (since
>>>>> it's static it might otherwise get eliminated).
>>>>>
>>>>>>> If anything this variable is exactly the opposite, i..e
>>>>>>> __read_mostly or even const (given that I can't see anything
>>>>>>> which writes it I suppose this is a compile time setting?)
>>>>>> That has been how I have been testing it so far (changing the
>>>>>> source to set values).  Mukesh claims to be able to change it
>>>>>> at will.  Not sure how const may effect this.
>>>> If the idea is to use kdb itself to frob the value, then it does
>>>> need something to stop the compiler caching it.  This might even be
>>>> one of the few cases where 'volatile' actually DTRT; it would still
>>>> be more in keeping with Xen style to use an explicit read op (like
>>>> atomic_read()) where the value is consumed.
>>> Is there any need to be asynchronously frobbing this value in the
>>> middle of a function within this file and expecting it to be
>> Yes. I can stop the machine via kdb or other debugger, change the value
>> during debug, and upon resuming it will start printing stuff. Often
>> this is needed when going thru several iterations of call before problem
>> is seen. Making it volatile makes sure the compiler loads it every
>> instance of its use. This is not in main path, only debugger path, so
>> the overhead should not be an issue.
> So you want to be able to toggle the value in between two immediately
> adjacent debug print calls? While debugging the debugging infrastructure
> itself? (using itself?).
>
> I'm surprised that even works, but if you say so then OK.
>
> Ian.
>

Based on Mukesh's statement, attached is the rebased version of this patch (labeled v3).  I included Mukesh's ack.

     -Don Slutz


[-- Attachment #2: 0003-dbg_rw_guest_mem-Conditionally-enable-debug-log-outp.patch --]
[-- Type: text/x-patch, Size: 7406 bytes --]

>From aac1a83a34e5a9d07975015e925563d399c5cd13 Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Sat, 4 Jan 2014 11:24:36 -0500
Subject: [BUGFIX][PATCH v3 3/5] dbg_rw_guest_mem: Conditionally enable debug
 log output

If dbg_debug is non-zero, output debug.

Include put_gfn debug logging.

Here is a smaple output at dbg_debug == 2:

(XEN) [2014-01-07 03:20:09] gmem:addr:8f56 buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
(XEN) [2014-01-07 03:20:09] vaddr:8f56 domid:1
(XEN) [2014-01-07 03:20:09] X: vaddr:8f56 domid:1 mfn:64331a
(XEN) [2014-01-07 03:20:09] R: addr:8f56 pagecnt=1 domid:1 gfn:8
(XEN) [2014-01-07 03:20:09] gmem:exit:len:$0
(XEN) [2014-01-07 03:20:09] gmem:addr:8f57 buf:00000000006e2020 len:$1 domid:1 toaddr:0 dp:ffff83083e5fe000
(XEN) [2014-01-07 03:20:09] vaddr:8f57 domid:1
(XEN) [2014-01-07 03:20:09] X: vaddr:8f57 domid:1 mfn:64331a
(XEN) [2014-01-07 03:20:09] R: addr:8f57 pagecnt=1 domid:1 gfn:8
(XEN) [2014-01-07 03:20:09] gmem:exit:len:$0
(XEN) [2014-01-07 03:20:09] gmem:addr:6ae9168b buf:00000000006e2020 len:$2 domid:1 toaddr:0 dp:ffff83083e5fe000
(XEN) [2014-01-07 03:20:09] vaddr:6ae9168b domid:1
(XEN) [2014-01-07 03:20:09] X: vaddr:6ae9168b domid:1 mfn:ffffffffffffffff
(XEN) [2014-01-07 03:20:09] R: domid:1 gfn:6ae91
(XEN) [2014-01-07 03:20:09] gmem:exit:len:$2

Signed-off-by: Don Slutz <dslutz@verizon.com>
Acked-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/debug.c | 54 +++++++++++++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 435bd40..d28fb70 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -30,16 +30,9 @@
  * gdbsx, etc..
  */
 
-#ifdef XEN_KDB_CONFIG
-#include "../kdb/include/kdbdefs.h"
-#include "../kdb/include/kdbproto.h"
-#define DBGP(...) {(kdbdbg) ? kdbp(__VA_ARGS__):0;}
-#define DBGP1(...) {(kdbdbg>1) ? kdbp(__VA_ARGS__):0;}
-#define DBGP2(...) {(kdbdbg>2) ? kdbp(__VA_ARGS__):0;}
-#else
-#define DBGP1(...) ((void)0)
-#define DBGP2(...) ((void)0)
-#endif
+static volatile int dbg_debug;
+#define DBGP(...) {(dbg_debug) ? printk(__VA_ARGS__) : 0;}
+#define DBGP1(...) {(dbg_debug > 1) ? printk(__VA_ARGS__) : 0;}
 
 /* Returns: mfn for the given (hvm guest) vaddr */
 static unsigned long 
@@ -50,27 +43,28 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
     uint32_t pfec = PFEC_page_present;
     p2m_type_t gfntype;
 
-    DBGP2("vaddr:%lx domid:%d\n", vaddr, dp->domain_id);
+    DBGP1("vaddr:%lx domid:%d\n", vaddr, dp->domain_id);
 
     *gfn = paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec);
     if ( *gfn == INVALID_GFN )
     {
-        DBGP2("kdb:bad gfn from gva_to_gfn\n");
+        DBGP1("kdb:bad gfn from gva_to_gfn\n");
         return INVALID_MFN;
     }
 
     mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); 
     if ( p2m_is_readonly(gfntype) && toaddr )
     {
-        DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
+        DBGP1("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
         mfn = INVALID_MFN;
     }
     else
-        DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
+        DBGP1("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
 
     if ( mfn == INVALID_MFN )
     {
         put_gfn(dp, *gfn);
+        DBGP1("R: domid:%d gfn:%lx\n", dp->domain_id, *gfn);
         *gfn = INVALID_GFN;
     }
 
@@ -100,7 +94,7 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val)
     unsigned long cr3 = (pgd3val ? pgd3val : dp->vcpu[0]->arch.cr3);
     unsigned long mfn = cr3 >> PAGE_SHIFT;
 
-    DBGP2("vaddr:%lx domid:%d cr3:%lx pgd3:%lx\n", vaddr, dp->domain_id, 
+    DBGP1("vaddr:%lx domid:%d cr3:%lx pgd3:%lx\n", vaddr, dp->domain_id,
           cr3, pgd3val);
 
     if ( pgd3val == 0 )
@@ -109,11 +103,11 @@ 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);
+        DBGP1("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);
+            DBGP("l4 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
             return INVALID_MFN;
         }
 
@@ -121,12 +115,12 @@ 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);
+        DBGP1("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) )
         {
-            DBGP1("l3 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
+            DBGP("l3 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
             return INVALID_MFN;
         }
     }
@@ -135,20 +129,20 @@ 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);
+    DBGP1("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) )
     {
-        DBGP1("l2 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
+        DBGP("l2 PAGE not present. vaddr:%lx cr3:%lx\n", vaddr, cr3);
         return INVALID_MFN;
     }
     l1t = map_domain_page(mfn);
     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);
+    DBGP1("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;
 }
@@ -186,7 +180,11 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len, struct domain *dp,
 
         unmap_domain_page(va);
         if ( gfn != INVALID_GFN )
+        {
             put_gfn(dp, gfn);
+            DBGP1("R: addr:%lx pagecnt=%ld domid:%d gfn:%lx\n",
+                  addr, pagecnt, dp->domain_id, gfn);
+        }
 
         addr += pagecnt;
         buf += pagecnt;
@@ -210,7 +208,7 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
     struct domain *dp = get_domain_by_id(domid);
     int hyp = (domid == DOMID_IDLE);
 
-    DBGP2("gmem:addr:%lx buf:%p len:$%d domid:%x toaddr:%x dp:%p\n", 
+    DBGP1("gmem:addr:%lx buf:%p len:$%d domid:%x toaddr:%x dp:%p\n",
           addr, buf, len, domid, toaddr, dp);
     if ( hyp )
     {
@@ -226,7 +224,7 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
         put_domain(dp);
     }
 
-    DBGP2("gmem:exit:len:$%d\n", len);
+    DBGP1("gmem:exit:len:$%d\n", len);
     return len;
 }
 
-- 
1.8.4


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

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

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Jan Beulich]

>>> On 09.01.14 at 17:08, Don Slutz <dslutz@verizon.com> wrote:
> Based on Mukesh's statement, attached is the rebased version of this patch 
> (labeled v3).  I included Mukesh's ack.

Unless this is meant just for reviewing purposes (albeit even then
it's likely problematic), could you please get used to sending
patch revisions with mail subjects (i.e. not retaining the prior
version indicator), so there is a reasonable chance to reconstruct
things by searching just the titles in a mail archive. (It's still fine -
at least as far as I'm concerned - to reply to an earlier version,
thus tying things into a single thread on the archive.)

Jan

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Don Slutz]

On 01/09/14 11:30, Jan Beulich wrote:
>>>> On 09.01.14 at 17:08, Don Slutz <dslutz@verizon.com> wrote:
>> Based on Mukesh's statement, attached is the rebased version of this patch
>> (labeled v3).  I included Mukesh's ack.
> Unless this is meant just for reviewing purposes (albeit even then
> it's likely problematic), could you please get used to sending
> patch revisions with mail subjects (i.e. not retaining the prior
> version indicator), so there is a reasonable chance to reconstruct
> things by searching just the titles in a mail archive. (It's still fine -
> at least as far as I'm concerned - to reply to an earlier version,
> thus tying things into a single thread on the archive.)
>
> Jan
>

I will try to.  I had not noticed this in the past.

    -Don Slutz

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Mukesh Rathor]

On Thu, 9 Jan 2014 09:59:08 +0000
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Wed, 2014-01-08 at 16:38 -0800, Mukesh Rathor wrote:
> > On Wed, 8 Jan 2014 17:44:22 +0000
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > > On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
> > > > At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> > > > > On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > > > > > Using volatile is almost always wrong. Why do you think
> > > > > > > it is needed here?
> > > > > > 
> > > > > > This was from Mukesh Rathor:
> > > > > > 
> > > > > > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > > > > > 
> > > > > > I saw no reason to make it volatile, but maybe "kdb" needs
> > > > > > this? Happy to change any way you want.
> > > > > 
> > > > > I'm not the maintainer but if I were I'd say drop the volatile
> > > > > and maybe mark it __read_mostly and perhaps __used too (since
> > > > > it's static it might otherwise get eliminated).
> > > > > 
> > > > > > > If anything this variable is exactly the opposite, i..e
> > > > > > > __read_mostly or even const (given that I can't see
> > > > > > > anything which writes it I suppose this is a compile time
> > > > > > > setting?)
> > > > > > 
> > > > > > That has been how I have been testing it so far (changing
> > > > > > the source to set values).  Mukesh claims to be able to
> > > > > > change it at will.  Not sure how const may effect this.
> > > > 
> > > > If the idea is to use kdb itself to frob the value, then it does
> > > > need something to stop the compiler caching it.  This might
> > > > even be one of the few cases where 'volatile' actually DTRT; it
> > > > would still be more in keeping with Xen style to use an
> > > > explicit read op (like atomic_read()) where the value is
> > > > consumed.
> > > 
> > > Is there any need to be asynchronously frobbing this value in the
> > > middle of a function within this file and expecting it to be
> > 
> > Yes. I can stop the machine via kdb or other debugger, change the
> > value during debug, and upon resuming it will start printing stuff.
> > Often this is needed when going thru several iterations of call
> > before problem is seen. Making it volatile makes sure the compiler
> > loads it every instance of its use. This is not in main path, only
> > debugger path, so the overhead should not be an issue.
> 
> So you want to be able to toggle the value in between two immediately
> adjacent debug print calls? While debugging the debugging
> infrastructure itself? (using itself?).

I often debug gdbsx via kdb, and one could use JTAG also to debug kdb,
or some other debugger. Debugging a debugger is hard. Since, the variable
is accessed via outside means that the compiler is not aware of, making
it volatile makes the most sense to me.

thanks,
Mukesh

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Ian Campbell]

On Thu, 2014-01-09 at 12:56 -0500, Don Slutz wrote:
> On 01/09/14 11:30, Jan Beulich wrote:
> >>>> On 09.01.14 at 17:08, Don Slutz <dslutz@verizon.com> wrote:
> >> Based on Mukesh's statement, attached is the rebased version of this patch
> >> (labeled v3).  I included Mukesh's ack.
> > Unless this is meant just for reviewing purposes (albeit even then
> > it's likely problematic), could you please get used to sending
> > patch revisions with mail subjects (i.e. not retaining the prior
> > version indicator), so there is a reasonable chance to reconstruct
> > things by searching just the titles in a mail archive. (It's still fine -
> > at least as far as I'm concerned - to reply to an earlier version,
> > thus tying things into a single thread on the archive.)
> >
> > Jan
> >
> 
> I will try to.  I had not noticed this in the past.

Thanks, as Jan says it is very confusing.

If there are tools things outstanding in this series which should be for
4.4 then I don't know what is where or what has been acked.

Please can resend whatever you think is outstanding for 4.4 as a fresh
thread with a suitable vN larger than any of the ones mentioned in any
of the replies here and with the acks collected.

Ian.

Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Don Slutz]

On 01/10/14 12:13, Ian Campbell wrote:
> On Thu, 2014-01-09 at 12:56 -0500, Don Slutz wrote:
>> On 01/09/14 11:30, Jan Beulich wrote:
>>>>>> On 09.01.14 at 17:08, Don Slutz <dslutz@verizon.com> wrote:
>>>> Based on Mukesh's statement, attached is the rebased version of this patch
>>>> (labeled v3).  I included Mukesh's ack.
>>> Unless this is meant just for reviewing purposes (albeit even then
>>> it's likely problematic), could you please get used to sending
>>> patch revisions with mail subjects (i.e. not retaining the prior
>>> version indicator), so there is a reasonable chance to reconstruct
>>> things by searching just the titles in a mail archive. (It's still fine -
>>> at least as far as I'm concerned - to reply to an earlier version,
>>> thus tying things into a single thread on the archive.)
>>>
>>> Jan
>>>
>> I will try to.  I had not noticed this in the past.
> Thanks, as Jan says it is very confusing.
>
> If there are tools things outstanding in this series which should be for
> 4.4 then I don't know what is where or what has been acked.
>
> Please can resend whatever you think is outstanding for 4.4 as a fresh
> thread with a suitable vN larger than any of the ones mentioned in any
> of the replies here and with the acks collected.
>
> Ian.
>
Will do.  I expect ~1 hour to rebase, build and quick test.

    -Don

Re: [PATCH v3 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Don Slutz]

On 01/10/14 16:15, Don Slutz wrote:
> On 01/10/14 12:13, Ian Campbell wrote:
>> On Thu, 2014-01-09 at 12:56 -0500, Don Slutz wrote:
>>> On 01/09/14 11:30, Jan Beulich wrote:
>>>>>>> On 09.01.14 at 17:08, Don Slutz <dslutz@verizon.com> wrote:
>>>>> Based on Mukesh's statement, attached is the rebased version of 
>>>>> this patch
>>>>> (labeled v3).  I included Mukesh's ack.
>>>> Unless this is meant just for reviewing purposes (albeit even then
>>>> it's likely problematic), could you please get used to sending
>>>> patch revisions with mail subjects (i.e. not retaining the prior
>>>> version indicator), so there is a reasonable chance to reconstruct
>>>> things by searching just the titles in a mail archive. (It's still 
>>>> fine -
>>>> at least as far as I'm concerned - to reply to an earlier version,
>>>> thus tying things into a single thread on the archive.)
>>>>
>>>> Jan
>>>>
>>> I will try to.  I had not noticed this in the past.
>> Thanks, as Jan says it is very confusing.
>>
>> If there are tools things outstanding in this series which should be for
>> 4.4 then I don't know what is where or what has been acked.
>>
>> Please can resend whatever you think is outstanding for 4.4 as a fresh
>> thread with a suitable vN larger than any of the ones mentioned in any
>> of the replies here and with the acks collected.
>>
>> Ian.
>>
> Will do.  I expect ~1 hour to rebase, build and quick test.
>
>    -Don


I have send out v4 of the rest. Adjusted this thread to v3.  I did not 
include this one in the 4.4 because:

1) Is is debug logging.

2) not 100% sure on volatile, __read_mostly,  __used , __used__, and 
maybe drop static.

3) Most developers cannot change dbg_debug value without a re-compile. 
And then volatile is not needed for them.

So I think it is fine to delay until 4.5 is open for this.

     -Don Slutz