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

[Don Slutz]

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 at verizon.com>
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 at verizon.com>
---
  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 at verizon.com>
>>>>
>>>> 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
>>