* [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs @ 2014-01-08 0:25 Don Slutz 2014-01-08 0:25 ` [PATCH v2 1/5] Add Emacs local variables to source files Don Slutz ` (5 more replies) 0 siblings, 6 replies; 45+ messages in thread From: Don Slutz @ 2014-01-08 0:25 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 1/5] Add Emacs local variables to source files. 2014-01-08 0:25 [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Don Slutz @ 2014-01-08 0:25 ` Don Slutz 2014-01-08 1:16 ` Mukesh Rathor 2014-01-08 0:25 ` [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path Don Slutz ` (4 subsequent siblings) 5 siblings, 1 reply; 45+ messages in thread From: Don Slutz @ 2014-01-08 0:25 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, Jan Beulich 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v2 1/5] Add Emacs local variables to source files. 2014-01-08 0:25 ` [PATCH v2 1/5] Add Emacs local variables to source files Don Slutz @ 2014-01-08 1:16 ` Mukesh Rathor 2014-01-08 1:27 ` Andrew Cooper 0 siblings, 1 reply; 45+ messages in thread From: Mukesh Rathor @ 2014-01-08 1:16 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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: > + */ ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 1/5] Add Emacs local variables to source files. 2014-01-08 1:16 ` Mukesh Rathor @ 2014-01-08 1:27 ` Andrew Cooper 2014-01-08 9:51 ` Ian Campbell 0 siblings, 1 reply; 45+ messages in thread From: Andrew Cooper @ 2014-01-08 1:27 UTC (permalink / raw) To: Mukesh Rathor, Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 1/5] Add Emacs local variables to source files. 2014-01-08 1:27 ` Andrew Cooper @ 2014-01-08 9:51 ` Ian Campbell 2014-01-08 15:58 ` Ian Campbell 0 siblings, 1 reply; 45+ messages in thread From: Ian Campbell @ 2014-01-08 9:51 UTC (permalink / raw) To: Andrew Cooper Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 1/5] Add Emacs local variables to source files. 2014-01-08 9:51 ` Ian Campbell @ 2014-01-08 15:58 ` Ian Campbell 0 siblings, 0 replies; 45+ messages in thread From: Ian Campbell @ 2014-01-08 15:58 UTC (permalink / raw) To: Andrew Cooper Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 0:25 [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Don Slutz 2014-01-08 0:25 ` [PATCH v2 1/5] Add Emacs local variables to source files Don Slutz @ 2014-01-08 0:25 ` Don Slutz 2014-01-08 0:55 ` Andrew Cooper 2014-01-08 8:36 ` Jan Beulich 2014-01-08 0:25 ` [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output Don Slutz ` (3 subsequent siblings) 5 siblings, 2 replies; 45+ messages in thread From: Don Slutz @ 2014-01-08 0:25 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, Jan Beulich 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 0:25 ` [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path Don Slutz @ 2014-01-08 0:55 ` Andrew Cooper 2014-01-08 1:06 ` Don Slutz ` (3 more replies) 2014-01-08 8:36 ` Jan Beulich 1 sibling, 4 replies; 45+ messages in thread From: Andrew Cooper @ 2014-01-08 0:55 UTC (permalink / raw) To: Don Slutz, xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, Jan Beulich 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; > } > ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 0:55 ` Andrew Cooper @ 2014-01-08 1:06 ` Don Slutz 2014-01-08 1:15 ` Andrew Cooper 2014-01-08 1:14 ` Mukesh Rathor ` (2 subsequent siblings) 3 siblings, 1 reply; 45+ messages in thread From: Don Slutz @ 2014-01-08 1:06 UTC (permalink / raw) To: Andrew Cooper, Don Slutz, xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, Jan Beulich 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; >> } >> ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 1:06 ` Don Slutz @ 2014-01-08 1:15 ` Andrew Cooper 0 siblings, 0 replies; 45+ messages in thread From: Andrew Cooper @ 2014-01-08 1:15 UTC (permalink / raw) To: Don Slutz, xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 0:55 ` Andrew Cooper 2014-01-08 1:06 ` Don Slutz @ 2014-01-08 1:14 ` Mukesh Rathor 2014-01-08 1:44 ` Mukesh Rathor 2014-01-08 10:40 ` Ian Campbell 3 siblings, 0 replies; 45+ messages in thread From: Mukesh Rathor @ 2014-01-08 1:14 UTC (permalink / raw) To: Andrew Cooper Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Tim Deegan, Don Slutz, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 0:55 ` Andrew Cooper 2014-01-08 1:06 ` Don Slutz 2014-01-08 1:14 ` Mukesh Rathor @ 2014-01-08 1:44 ` Mukesh Rathor 2014-01-08 2:30 ` Andrew Cooper 2014-01-08 10:40 ` Ian Campbell 3 siblings, 1 reply; 45+ messages in thread From: Mukesh Rathor @ 2014-01-08 1:44 UTC (permalink / raw) To: Andrew Cooper Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Tim Deegan, Don Slutz, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 1:44 ` Mukesh Rathor @ 2014-01-08 2:30 ` Andrew Cooper 2014-01-08 2:44 ` Mukesh Rathor 0 siblings, 1 reply; 45+ messages in thread From: Andrew Cooper @ 2014-01-08 2:30 UTC (permalink / raw) To: Mukesh Rathor Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Tim Deegan, Don Slutz, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 2:30 ` Andrew Cooper @ 2014-01-08 2:44 ` Mukesh Rathor 0 siblings, 0 replies; 45+ messages in thread From: Mukesh Rathor @ 2014-01-08 2:44 UTC (permalink / raw) To: Andrew Cooper Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Tim Deegan, Don Slutz, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 0:55 ` Andrew Cooper ` (2 preceding siblings ...) 2014-01-08 1:44 ` Mukesh Rathor @ 2014-01-08 10:40 ` Ian Campbell 2014-01-08 14:01 ` Don Slutz 3 siblings, 1 reply; 45+ messages in thread From: Ian Campbell @ 2014-01-08 10:40 UTC (permalink / raw) To: Andrew Cooper Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 10:40 ` Ian Campbell @ 2014-01-08 14:01 ` Don Slutz 0 siblings, 0 replies; 45+ messages in thread From: Don Slutz @ 2014-01-08 14:01 UTC (permalink / raw) To: Ian Campbell, Andrew Cooper Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. > > ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 0:25 ` [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path Don Slutz 2014-01-08 0:55 ` Andrew Cooper @ 2014-01-08 8:36 ` Jan Beulich 2014-01-08 13:48 ` Don Slutz 1 sibling, 1 reply; 45+ messages in thread From: Jan Beulich @ 2014-01-08 8:36 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel >>> 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; > } ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path. 2014-01-08 8:36 ` Jan Beulich @ 2014-01-08 13:48 ` Don Slutz 0 siblings, 0 replies; 45+ messages in thread From: Don Slutz @ 2014-01-08 13:48 UTC (permalink / raw) To: Jan Beulich, Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel [-- 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 0:25 [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Don Slutz 2014-01-08 0:25 ` [PATCH v2 1/5] Add Emacs local variables to source files Don Slutz 2014-01-08 0:25 ` [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path Don Slutz @ 2014-01-08 0:25 ` Don Slutz 2014-01-08 1:38 ` Mukesh Rathor 2014-01-08 10:38 ` Ian Campbell 2014-01-08 0:25 ` [BUGFIX][PATCH v2 4/5] xg_read_mem: Report on error Don Slutz ` (2 subsequent siblings) 5 siblings, 2 replies; 45+ messages in thread From: Don Slutz @ 2014-01-08 0:25 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, Jan Beulich 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 0:25 ` [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output Don Slutz @ 2014-01-08 1:38 ` Mukesh Rathor 2014-01-08 10:38 ` Ian Campbell 1 sibling, 0 replies; 45+ messages in thread From: Mukesh Rathor @ 2014-01-08 1:38 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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; > } > ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 0:25 ` [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output Don Slutz 2014-01-08 1:38 ` Mukesh Rathor @ 2014-01-08 10:38 ` Ian Campbell 2014-01-08 14:28 ` Don Slutz 1 sibling, 1 reply; 45+ messages in thread From: Ian Campbell @ 2014-01-08 10:38 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 10:38 ` Ian Campbell @ 2014-01-08 14:28 ` Don Slutz 2014-01-08 16:47 ` Ian Campbell 0 siblings, 1 reply; 45+ messages in thread From: Don Slutz @ 2014-01-08 14:28 UTC (permalink / raw) To: Ian Campbell, Don Slutz Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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. > ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 14:28 ` Don Slutz @ 2014-01-08 16:47 ` Ian Campbell 2014-01-08 17:04 ` Tim Deegan 0 siblings, 1 reply; 45+ messages in thread From: Ian Campbell @ 2014-01-08 16:47 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 16:47 ` Ian Campbell @ 2014-01-08 17:04 ` Tim Deegan 2014-01-08 17:44 ` Ian Campbell 0 siblings, 1 reply; 45+ messages in thread From: Tim Deegan @ 2014-01-08 17:04 UTC (permalink / raw) To: Ian Campbell Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 17:04 ` Tim Deegan @ 2014-01-08 17:44 ` Ian Campbell 2014-01-08 18:10 ` Tim Deegan 2014-01-09 0:38 ` Mukesh Rathor 0 siblings, 2 replies; 45+ messages in thread From: Ian Campbell @ 2014-01-08 17:44 UTC (permalink / raw) To: Tim Deegan Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 17:44 ` Ian Campbell @ 2014-01-08 18:10 ` Tim Deegan 2014-01-09 8:41 ` Ian Campbell 2014-01-09 0:38 ` Mukesh Rathor 1 sibling, 1 reply; 45+ messages in thread From: Tim Deegan @ 2014-01-08 18:10 UTC (permalink / raw) To: Ian Campbell Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 18:10 ` Tim Deegan @ 2014-01-09 8:41 ` Ian Campbell 2014-01-09 10:32 ` Tim Deegan 0 siblings, 1 reply; 45+ messages in thread From: Ian Campbell @ 2014-01-09 8:41 UTC (permalink / raw) To: Tim Deegan Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-09 8:41 ` Ian Campbell @ 2014-01-09 10:32 ` Tim Deegan 0 siblings, 0 replies; 45+ messages in thread From: Tim Deegan @ 2014-01-09 10:32 UTC (permalink / raw) To: Ian Campbell Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-08 17:44 ` Ian Campbell 2014-01-08 18:10 ` Tim Deegan @ 2014-01-09 0:38 ` Mukesh Rathor 2014-01-09 9:59 ` Ian Campbell 1 sibling, 1 reply; 45+ messages in thread From: Mukesh Rathor @ 2014-01-09 0:38 UTC (permalink / raw) To: Ian Campbell Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Tim Deegan, Don Slutz, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-09 0:38 ` Mukesh Rathor @ 2014-01-09 9:59 ` Ian Campbell 2014-01-09 16:08 ` Don Slutz 2014-01-10 1:54 ` [PATCH v2 " Mukesh Rathor 0 siblings, 2 replies; 45+ messages in thread From: Ian Campbell @ 2014-01-09 9:59 UTC (permalink / raw) To: Mukesh Rathor Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Tim Deegan, Don Slutz, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-09 9:59 ` Ian Campbell @ 2014-01-09 16:08 ` Don Slutz 2014-01-09 16:30 ` Jan Beulich 2014-01-10 1:54 ` [PATCH v2 " Mukesh Rathor 1 sibling, 1 reply; 45+ messages in thread From: Don Slutz @ 2014-01-09 16:08 UTC (permalink / raw) To: Ian Campbell, Mukesh Rathor Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Tim Deegan, Don Slutz, xen-devel, Jan Beulich [-- 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-09 16:08 ` Don Slutz @ 2014-01-09 16:30 ` Jan Beulich 2014-01-09 17:56 ` Don Slutz 0 siblings, 1 reply; 45+ messages in thread From: Jan Beulich @ 2014-01-09 16:30 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-09 16:30 ` Jan Beulich @ 2014-01-09 17:56 ` Don Slutz 2014-01-10 17:13 ` Ian Campbell 0 siblings, 1 reply; 45+ messages in thread From: Don Slutz @ 2014-01-09 17:56 UTC (permalink / raw) To: Jan Beulich, Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, xen-devel 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-09 17:56 ` Don Slutz @ 2014-01-10 17:13 ` Ian Campbell 2014-01-10 21:15 ` Don Slutz 0 siblings, 1 reply; 45+ messages in thread From: Ian Campbell @ 2014-01-10 17:13 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, xen-devel, Jan Beulich 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. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-10 17:13 ` Ian Campbell @ 2014-01-10 21:15 ` Don Slutz 2014-01-10 22:08 ` [PATCH v3 " Don Slutz 0 siblings, 1 reply; 45+ messages in thread From: Don Slutz @ 2014-01-10 21:15 UTC (permalink / raw) To: Ian Campbell Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, Don Slutz, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-10 21:15 ` Don Slutz @ 2014-01-10 22:08 ` Don Slutz 0 siblings, 0 replies; 45+ messages in thread From: Don Slutz @ 2014-01-10 22:08 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Tim Deegan, Ian Jackson, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output 2014-01-09 9:59 ` Ian Campbell 2014-01-09 16:08 ` Don Slutz @ 2014-01-10 1:54 ` Mukesh Rathor 1 sibling, 0 replies; 45+ messages in thread From: Mukesh Rathor @ 2014-01-10 1:54 UTC (permalink / raw) To: Ian Campbell Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, Tim Deegan, Don Slutz, xen-devel, Jan Beulich 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* [BUGFIX][PATCH v2 4/5] xg_read_mem: Report on error. 2014-01-08 0:25 [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Don Slutz ` (2 preceding siblings ...) 2014-01-08 0:25 ` [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output Don Slutz @ 2014-01-08 0:25 ` Don Slutz 2014-01-08 1:16 ` Mukesh Rathor 2014-01-08 0:25 ` [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error Don Slutz 2014-01-08 8:28 ` [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Jan Beulich 5 siblings, 1 reply; 45+ messages in thread From: Don Slutz @ 2014-01-08 0:25 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, Jan Beulich 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 4/5] xg_read_mem: Report on error. 2014-01-08 0:25 ` [BUGFIX][PATCH v2 4/5] xg_read_mem: Report on error Don Slutz @ 2014-01-08 1:16 ` Mukesh Rathor 0 siblings, 0 replies; 45+ messages in thread From: Mukesh Rathor @ 2014-01-08 1:16 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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); ^ permalink raw reply [flat|nested] 45+ messages in thread
* [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error. 2014-01-08 0:25 [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Don Slutz ` (3 preceding siblings ...) 2014-01-08 0:25 ` [BUGFIX][PATCH v2 4/5] xg_read_mem: Report on error Don Slutz @ 2014-01-08 0:25 ` Don Slutz 2014-01-08 1:11 ` Mukesh Rathor 2014-01-08 10:35 ` Ian Campbell 2014-01-08 8:28 ` [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Jan Beulich 5 siblings, 2 replies; 45+ messages in thread From: Don Slutz @ 2014-01-08 0:25 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, Don Slutz, Jan Beulich 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error. 2014-01-08 0:25 ` [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error Don Slutz @ 2014-01-08 1:11 ` Mukesh Rathor 2014-01-08 10:35 ` Ian Campbell 1 sibling, 0 replies; 45+ messages in thread From: Mukesh Rathor @ 2014-01-08 1:11 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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; > } > ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error. 2014-01-08 0:25 ` [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error Don Slutz 2014-01-08 1:11 ` Mukesh Rathor @ 2014-01-08 10:35 ` Ian Campbell 2014-01-08 14:39 ` Don Slutz 1 sibling, 1 reply; 45+ messages in thread From: Ian Campbell @ 2014-01-08 10:35 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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; > } > ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error. 2014-01-08 10:35 ` Ian Campbell @ 2014-01-08 14:39 ` Don Slutz 0 siblings, 0 replies; 45+ messages in thread From: Don Slutz @ 2014-01-08 14:39 UTC (permalink / raw) To: Ian Campbell, Don Slutz Cc: Keir Fraser, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel, Jan Beulich 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; >> } >> > ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs 2014-01-08 0:25 [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Don Slutz ` (4 preceding siblings ...) 2014-01-08 0:25 ` [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error Don Slutz @ 2014-01-08 8:28 ` Jan Beulich 2014-01-08 14:43 ` Don Slutz 5 siblings, 1 reply; 45+ messages in thread From: Jan Beulich @ 2014-01-08 8:28 UTC (permalink / raw) To: Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs 2014-01-08 8:28 ` [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Jan Beulich @ 2014-01-08 14:43 ` Don Slutz 0 siblings, 0 replies; 45+ messages in thread From: Don Slutz @ 2014-01-08 14:43 UTC (permalink / raw) To: Jan Beulich, Don Slutz Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Ian Jackson, xen-devel 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 ^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2014-01-10 22:08 UTC | newest] Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-08 0:25 [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Don Slutz 2014-01-08 0:25 ` [PATCH v2 1/5] Add Emacs local variables to source files Don Slutz 2014-01-08 1:16 ` Mukesh Rathor 2014-01-08 1:27 ` Andrew Cooper 2014-01-08 9:51 ` Ian Campbell 2014-01-08 15:58 ` Ian Campbell 2014-01-08 0:25 ` [BUGFIX][PATCH v2 2/5] dbg_rw_guest_mem: need to call put_gfn in error path Don Slutz 2014-01-08 0:55 ` Andrew Cooper 2014-01-08 1:06 ` Don Slutz 2014-01-08 1:15 ` Andrew Cooper 2014-01-08 1:14 ` Mukesh Rathor 2014-01-08 1:44 ` Mukesh Rathor 2014-01-08 2:30 ` Andrew Cooper 2014-01-08 2:44 ` Mukesh Rathor 2014-01-08 10:40 ` Ian Campbell 2014-01-08 14:01 ` Don Slutz 2014-01-08 8:36 ` Jan Beulich 2014-01-08 13:48 ` Don Slutz 2014-01-08 0:25 ` [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output Don Slutz 2014-01-08 1:38 ` Mukesh Rathor 2014-01-08 10:38 ` Ian Campbell 2014-01-08 14:28 ` Don Slutz 2014-01-08 16:47 ` Ian Campbell 2014-01-08 17:04 ` Tim Deegan 2014-01-08 17:44 ` Ian Campbell 2014-01-08 18:10 ` Tim Deegan 2014-01-09 8:41 ` Ian Campbell 2014-01-09 10:32 ` Tim Deegan 2014-01-09 0:38 ` Mukesh Rathor 2014-01-09 9:59 ` Ian Campbell 2014-01-09 16:08 ` Don Slutz 2014-01-09 16:30 ` Jan Beulich 2014-01-09 17:56 ` Don Slutz 2014-01-10 17:13 ` Ian Campbell 2014-01-10 21:15 ` Don Slutz 2014-01-10 22:08 ` [PATCH v3 " Don Slutz 2014-01-10 1:54 ` [PATCH v2 " Mukesh Rathor 2014-01-08 0:25 ` [BUGFIX][PATCH v2 4/5] xg_read_mem: Report on error Don Slutz 2014-01-08 1:16 ` Mukesh Rathor 2014-01-08 0:25 ` [BUGFIX][PATCH v2 5/5] xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error Don Slutz 2014-01-08 1:11 ` Mukesh Rathor 2014-01-08 10:35 ` Ian Campbell 2014-01-08 14:39 ` Don Slutz 2014-01-08 8:28 ` [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs Jan Beulich 2014-01-08 14:43 ` Don Slutz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.