Re: [Crash-utility] [PATCH 0/7] Upgrade the remote access to a xen domain.

----- Original Message ----- > From: Don Slutz <dslutz(a)verizon.com&gt; > > Currently crash 4.0 and later using the code: > > http://lists.xen.org/archives/html/xen-devel/2013-11/msg02569.html > > There is some very limited documention on crashes remote ptotocol in: > > http://lists.xen.org/archives/html/xen-devel/2013-11/msg02352.html > > and some fixes in the attachment > 0001-xen-crashd-Connect-crash-with-domain.patch > from the 1st link above. > > How ever there are issues with the current code: > > 1) The remote protocol has a minor issue (which I have not been able to > happen) based on the fact TCP/IP is stream based protocol. This means > a RECV or a SEND may not do the fully requested size of data. In fact > the current code assumes that the amount of data that a SEND is called > with will all be read with a single RECV. > > 2) The most common mismatch between crash and older kernels is > phys_base. In the remote case, see if the remote server supports > vitrual memory access, and if so, see if phys_base can be retreived. > > 3) crash assumes that the remote system is active and can not return > currect IP and SP to do a better back trace. > > 4) enable a non-active mode of remote access. > > This patch set attempts to fix these: > > The fix for #1 I have called NIL mode (patch 1). > The fix for #2 uses get_remote_phys_base (patch 3). > The fix for #3 uses get_remote_regs (patch 5). > The fix for #4 uses special file /dev/xenmem (patch 7). > It also REMOTE_NIL() to indicate "remote paused system". > > Don Slutz (7): > Add NIL mode to remote. > remote_proc_version: NULL terminate passed buffer on error. > Add get_remote_phys_base. > Add remote_vtop. > bt: get remote live registers if possible. > Add get_remote_cr3 > Add support for non-live remote. > > defs.h | 8 +- > kernel.c | 25 +++- > memory.c | 25 +++- > remote.c | 462 > +++++++++++++++++++++++++++++++++++++++++++++++++++++---------- > x86_64.c | 24 ++++ > 5 files changed, 465 insertions(+), 79 deletions(-) > > -- > 1.8.4 Hi Don, A few comments... I don't really understand what entity the code is communicating with, so for the most part, I'm primarily interested in making sure that your patches in no way can affect the normal usage of the crash utility. That being said, is the term "NIL mode" some kind of Xen-ism?

It wasn't until I saw the display_sys_stats() function that I even understood that REMOTE_NIL mean "(remote paused system)". Anyway, if you made it something like "REMOTE_PAUSED" or something like that, the code might make some sense to the untrained eye.

And it's not entirely clear to me what REMOTE_ACTIVE() means now? It used to imply communicating with a remote crash daeamon that accessed the live system that the daemon was running on. Has that meaning been changed to mean that crash is communicating with a daemon that's accessing a "live" Xen guest kernel?

I'm confused about these two changes, where you re-patch your own patch: - else + else if (REMOTE_ACTIVE() && ((bt->task == tt->this_task) || is_task_active(bt->task))) { + if (get_remote_regs(bt, &eip, &esp)) + machdep->get_stack_frame(bt, &eip, &esp); + } else followed by: - else if (REMOTE_ACTIVE() && ((bt->task == tt->this_task) || is_task_active(bt->task))) { + else if (REMOTE_NIL() && ((bt->task == tt->this_task) || is_task_active(bt->task))) { if (get_remote_regs(bt, &eip, &esp)) machdep->get_stack_frame(bt, &eip, &esp); } else

If I'm not mistaken, it would seem to be impossible for "(bt->task == tt->this_task)" to ever be true in the REMOTE_NIL() case? As I recall tt->this_task used to be the PID of the remote daemon itself, which would be accessing the memory of its own kernel. So how could the pid of the remote daemon ever be the target of a back trace of a paused guest? And so I don't understand why you first made an if clause for REMOTE_ACTIVE() to begin with -- whatever that means now -- and then changed it to REMOTE_NIL()?

In any case, since the ultimate change is only interested in REMOTE_NIL(), can you make it all less of an eyesore by encapulating its if case something like: else if (SADUMP_DUMPFILE()) get_sadump_regs(bt, &eip, &esp); + else if (REMOTE_NIL()) { + if (!((bt->task == tt->this_task) || is_task_active(bt->task)) || + !get_remote_regs(bt, &eip, &esp)) + machdep->get_stack_frame(bt, &eip, &esp); } else machdep->get_stack_frame(bt, &eip, &esp); Note above, for consistency's sake with the rest of the crash utility, can you make get_remote_regs() return non-zero when it successfully gets the registers, and zero if it fails?

And lastly, can you fix these? $ make warn ... remote.c: In function ‘validate_phys_base’: remote.c:2302:25: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘physaddr_t’ [-Wformat] remote.c:2302:25: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘physaddr_t’ [-Wformat] remote.c:2302:25: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 5 has type ‘physaddr_t’ [-Wformat] remote.c:2302:25: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 6 has type ‘physaddr_t’ [-Wformat] remote.c: In function ‘remote_vtop’: remote.c:2361:2: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘physaddr_t’ [-Wformat] remote.c:2367:4: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘physaddr_t’ [-Wformat] remote.c:2352:18: warning: variable ‘p3’ set but not used [-Wunused-but-set-variable] remote.c:2352:13: warning: variable ‘p2’ set but not used [-Wunused-but-set-variable] remote.c:2352:8: warning: variable ‘p1’ set but not used [-Wunused-but-set-variable] remote.c: In function ‘get_remote_regs’: remote.c:2392:33: warning: unused variable ‘p6’ [-Wunused-variable] remote.c:2392:28: warning: variable ‘p5’ set but not used [-Wunused-but-set-variable] remote.c:2392:18: warning: variable ‘p3’ set but not used [-Wunused-but-set-variable] remote.c:2392:8: warning: variable ‘p1’ set but not used [-Wunused-but-set-variable] remote.c: In function ‘get_remote_cr3’: remote.c:2449:13: warning: variable ‘p2’ set but not used [-Wunused-but-set-variable] remote.c:2449:8: warning: variable ‘p1’ set but not used [-Wunused-but-set-variable] ...

And I'm not interested in appending these to the 4 files that you changed: +/* + * Specify Emacs local variables so the formating + * of the code stays the same. + * + * Local variables: + * mode: C + * c-set-style: "BSD" + * c-basic-offset: 8 + * indent-tabs-mode: t + * End: + */ Thanks, Dave