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

[Dave Anderson]

----- 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?  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