From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output Date: Thu, 9 Jan 2014 11:08:24 -0500 Message-ID: <52CEC978.7040705@terremark.com> References: <1389140748-26524-1-git-send-email-dslutz@verizon.com> <1389140748-26524-4-git-send-email-dslutz@verizon.com> <1389177510.4883.11.camel@kazak.uk.xensource.com> <52CD60AA.9010607@terremark.com> <1389199626.4883.112.camel@kazak.uk.xensource.com> <20140108170442.GA75747@deinos.phlegethon.org> <1389203062.27473.8.camel@kazak.uk.xensource.com> <20140108163822.30b6f87a@mantra.us.oracle.com> <1389261548.27473.42.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------070809020903090506040507" Return-path: In-Reply-To: <1389261548.27473.42.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Mukesh Rathor Cc: Keir Fraser , Stefano Stabellini , George Dunlap , Ian Jackson , Tim Deegan , Don Slutz , xen-devel@lists.xen.org, Jan Beulich List-Id: xen-devel@lists.xenproject.org --------------070809020903090506040507 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit 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 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 --------------070809020903090506040507 Content-Type: text/x-patch; name="0003-dbg_rw_guest_mem-Conditionally-enable-debug-log-outp.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0003-dbg_rw_guest_mem-Conditionally-enable-debug-log-outp.pa"; filename*1="tch" >>From aac1a83a34e5a9d07975015e925563d399c5cd13 Mon Sep 17 00:00:00 2001 From: Don Slutz 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 Acked-by: Mukesh Rathor --- 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 --------------070809020903090506040507 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------070809020903090506040507--