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

Hi Don, This v2 version, plus the REMOTE_MEMSRC() patch below, is queued for crash-7.0.4. How would you like the changelog message to read? Tentatively I've got this: - Resurrection of the remote analysis capability for use with the "xen-crashd" daemon running on a Xen Dom0 host, which communicates with a paused or shutdown DomU guest kernel. The daemon can be accessed like so: $ crash localhost:5991,/dev/xenmem vmlinux (dslutz(a)verizon.com) Thanks, Dave

----- Original Message ----- > On 11/19/13 14:28, Dave Anderson wrote: >> Hi Don, >> >> I still need some help understanding this "resurrection" of the remote >> access facility, i.e., the new meanings of the REMOTE_xxx flags/macros. >> >> The currently-existing comment for the is_remote_daemon() function >> in remote.c describes the (obsolete) usage of the facility: >> >> /* >> * Parse, verify and establish a connection with the network daemon >> * specified on the crash command line. >> * >> * The format is: >> [remote-hostname]:port[,remote-namelist][,remote-dumpfile] >> * >> * where everything but the port number is optional, and the >> remote-namelist >> * and remote-dumpfile can be reversed. >> * >> * 1. The default remote host is the local host. >> * 2. The default dumpfile is /dev/mem. >> * 3. If no remote-namelist and remote-dumpfile are given, the daemon >> * is queried for a kernel that matches the remote /proc/version. >> * If no local kernel namelist is entered, the remote version will >> * be copied locally when fd_init() is called. >> * 4. If a remote-dumpfile is given with no remote namelist, it is >> presumed >> * that the kernel namelist will be entered locally. >> */ >> >> Back then, the remote "crashd" daemon could be contacted for: >> >> (1) live access to the remote machine's kernel using its /dev/mem, or >> (2) accessing a vmlinux/vmcore pair that existed on the remote machine. >> >> Can you explain this new usage? It sounds like there is a remote daemon >> running on a remote Dom0 kernel, and that your new functionality can only >> be used to: >> >> (1) access a running domU guest hosted by that remote system, or >> (2) access a paused domU guest hosted by that remote system. >> >> And that there is also a device that the the remote daemon uses >> to access the memory of the guest, be it paused or live. >> >> Am I in the ballpark? > Yes. > > The currently proposed code for Dom0, does not support (1) mostly > because it seems much better to me to be able to poke around and look at > a fixed state of the domU. > > There is a (3) in xen: guest has shutdown. A shutdown guest has > subtypes like "requested shutdown", "requested reboot", "detected > crash", etc. Xen will set a guest in shutdown state when a double fault > or triple fault happens. A paused guest is the same as an unpaused > guest in shutdown state. Using a much older Xen, I known that that > version treated a double fault is the same as a reboot request. The bug > I was tracking down turned out to be a extra swapgs which will cause > many linux kernel's to double fault on a page fault. > > I was also looking to be able to use old versions of crash so as to > make it easier to use and/or deploy. Since the 7.0.3 or older version of > crash only support "live access to the remote machine's kernel using its > /dev/mem "; I picked to default to be a paused system. Since crash > thinks the remote system is live, it keeps fetching state which is not > changing. > > The "device that the the remote daemon uses ", is more a defined > ABI which has 2 layers libxl and libxc (what I all xc_* routines). In > this case I am using libxc calls. None of these require the guest to be > paused. > > So it would not be hard to change the Xen code to support new > option (1) if there is need for such a mode. I could see adding some > sort of pause control as an extension to crash. > > I have recently learn of another Xen tool: gdbsx. It is designed > to connect gdb to a running guest and pause and resume the guest on > demand. While I have not done very much with it, it looks to be > powerful. Since it is just gdb, it is missing crash's kernel support. > >> Dave > While working on this email, I found out that it looks better to me to > include REMOTE_PAUSED() in REMOTE_MEMSRC(). I.E. either add the following: > > From b4093734b2db8666f155032ffd9637dcfda0fc0b Mon Sep 17 00:00:00 2001 > From: Don Slutz <dslutz(a)verizon.com&gt; > Date: Wed, 20 Nov 2013 07:17:51 -0500 > Subject: [PATCH] Add REMOTE_PAUSED() to REMOTE_MEMSRC() > > And remove the redunent 'REMOTE_MEMSRC() || REMOTE_PAUSED()'. > > Signed-off-by: Don Slutz <dslutz(a)verizon.com&gt; > --- > defs.h | 2 +- > memory.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/defs.h b/defs.h > index dae9662..2ec5540 100755 > --- a/defs.h > +++ b/defs.h > @@ -254,7 +254,7 @@ struct number_option { > #define REMOTE_ACTIVE() (pc->flags & REM_LIVE_SYSTEM) > #define REMOTE_DUMPFILE() \ > (pc->flags & (REM_NETDUMP|REM_MCLXCD|REM_LKCD|REM_S390D)) > -#define REMOTE_MEMSRC() (REMOTE_ACTIVE() || REMOTE_DUMPFILE()) > +#define REMOTE_MEMSRC() (REMOTE_ACTIVE() || REMOTE_PAUSED() || > REMOTE_DUMPFILE()) > #define LKCD_DUMPFILE() (pc->flags & (LKCD|REM_LKCD)) > #define NETDUMP_DUMPFILE() (pc->flags & (NETDUMP|REM_NETDUMP)) > #define DISKDUMP_DUMPFILE() (pc->flags & DISKDUMP) > diff --git a/memory.c b/memory.c > index cf8da2c..d17ba3c 100755 > --- a/memory.c > +++ b/memory.c > @@ -14903,7 +14903,7 @@ memory_page_size(void) > if (machdep->pagesize) > return machdep->pagesize; > > - if (REMOTE_MEMSRC() || REMOTE_PAUSED()) > + if (REMOTE_MEMSRC()) > return remote_page_size(); > > switch (pc->flags & MEMORY_SOURCES) > -- > 1.7.11.7 > > or adjust patch v2 7/7 to also do this. > > > -Don Slutz >> ----- Original Message ----- >>> On 11/19/13 11:08, Dave Anderson wrote: >>>> ----- 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? >>> Nope, it is term I came up with to describe the protocol change. >>>> 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. >>> Will do. Looking back at it, I ended up overloading the term. :( >>>> 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? >>> The meaning is not as clear. Since the existing versions of crash already >>> have REMOTE_ACTIVE() and I wanted the Xen utility to be usable to them, in >>> both cases the "live" Xen guest kernel is being accessed while it is >>> paused. >>> I added REMOTE_PAUSED() to have a few changes to crash to correctly >>> understand that the guest is paused. As part of this I add the fake file >>> "/dev/xenmem". >>>> 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 >>> Clearly I need to adjust the patch order. >>>> 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()? >>> This came about from some old changes from 2012 to a crash version 4.0 for >>> other work. I had not thought of /dev/xenmem at the time, and just copied >>> the condition from above. I see no reason not to drop "bt->task == >>> tt->this_task". Will check that the right actions are still done. >>>> 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? >>> Sure. >>>> 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] >>>> ... >>> Yes. >>> >>>> 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 >>> Ok, that is 2 votes to drop this, so I will. >>> -Don Slutz >>> >