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

[Don Slutz <dslutz@verizon.com>]

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