[PATCH v2 00/12] xenctx: Many changes.

[PATCH v2 00/12] xenctx: Many changes.

[Don Slutz]

From: Don Slutz <dslutz@verizon.com>

Change from v1 to v2:
  title was: xenctx: Add an option to output more registers.
  Processed review comments.
  Jan Beulich:
    Split 1 change into 12.
    Switch to enum for is_kernel_text(), renamed to is_kernel_addr().
    Renamed vars like memAddr to mem_addr.
  Ian Campbell:
    More on is_kernel_text().

Don Slutz (12):
  xenctx: Clean up stack trace when hypercall_page not in symbol table
  xenctx: Add -2 (--two-pages) option to switch stack size to 8KiB
  xenctx: Output ascii version of stack also.
  xenctx: Add stack address to dump, switch to 16 bytes per line.
  xenctx: Change print_symbol to do the space before.
  xenctx: More info on failed to map page.
  xenctx: Add stack addr to call trace.
  xenctx: Add -d <daddr> option to dump memory at daddr as a stack.
  xenctx: Add -m <maddr> option to dump memory at maddr.
  xenctx: change is_kernel_text() into is_kernel_addr().
  xenctx: Dump registers via hvm info if available.
  xenctx: Add optional fCPU.

 tools/xentrace/xenctx.c |  628 +++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 549 insertions(+), 79 deletions(-)

[PATCH v2 01/12] xenctx: Clean up stack trace when hypercall_page not in symbol table

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

Before:

Call Trace:
  [<ffffffff8006b2b0>] default_idle+0x29  <--
  [<ffffffff80048d19>] cpu_idle+0x95
  [<ffffffff803e7801>] start_kernel+0x220
  [<0000000000000000>] startup_64+0x80000000
  [<ffffffff803e722f>] x86_64_start_kernel+0x22f
  [<0000000000000000>] startup_64+0x80000000
  [<0000000000000000>] startup_64+0x80000000
  [<0000000000000000>] startup_64+0x80000000
  [<0000000000000000>] startup_64+0x80000000

After:

Call Trace:
  [<ffffffff8006b2b0>] default_idle+0x29  <--
  [<ffffffff80048d19>] cpu_idle+0x95
  [<ffffffff803e7801>] start_kernel+0x220
  [<ffffffff803e722f>] x86_64_start_kernel+0x22f

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 060e480..10292fa 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -83,8 +83,9 @@ static int is_kernel_text(guest_word_t addr)
     if (addr >= kernel_stext &&
         addr <= kernel_etext)
         return 1;
-    if (addr >= kernel_hypercallpage &&
-        addr <= kernel_hypercallpage + 4096)
+    if (kernel_hypercallpage &&
+        (addr >= kernel_hypercallpage &&
+         addr <= kernel_hypercallpage + 4096))
         return 1;
     if (addr >= kernel_sinittext &&
         addr <= kernel_einittext)
-- 
1.7.1

Re: [PATCH v2 01/12] xenctx: Clean up stack trace when hypercall_page not in symbol table

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> From: Don Slutz <Don@CloudSwitch.com>
> 
> Before:
> 
> Call Trace:
>   [<ffffffff8006b2b0>] default_idle+0x29  <--
>   [<ffffffff80048d19>] cpu_idle+0x95
>   [<ffffffff803e7801>] start_kernel+0x220
>   [<0000000000000000>] startup_64+0x80000000
>   [<ffffffff803e722f>] x86_64_start_kernel+0x22f
>   [<0000000000000000>] startup_64+0x80000000
>   [<0000000000000000>] startup_64+0x80000000
>   [<0000000000000000>] startup_64+0x80000000
>   [<0000000000000000>] startup_64+0x80000000
> 
> After:
> 
> Call Trace:
>   [<ffffffff8006b2b0>] default_idle+0x29  <--
>   [<ffffffff80048d19>] cpu_idle+0x95
>   [<ffffffff803e7801>] start_kernel+0x220
>   [<ffffffff803e722f>] x86_64_start_kernel+0x22f
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

And a note on patch submission: You send _to_ the list, and _cc_
maintainers and other relevant people (without stretching the
meaning of "relevant" too much).

Jan

> ---
>  tools/xentrace/xenctx.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index 060e480..10292fa 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -83,8 +83,9 @@ static int is_kernel_text(guest_word_t addr)
>      if (addr >= kernel_stext &&
>          addr <= kernel_etext)
>          return 1;
> -    if (addr >= kernel_hypercallpage &&
> -        addr <= kernel_hypercallpage + 4096)
> +    if (kernel_hypercallpage &&
> +        (addr >= kernel_hypercallpage &&
> +         addr <= kernel_hypercallpage + 4096))
>          return 1;
>      if (addr >= kernel_sinittext &&
>          addr <= kernel_einittext)
> -- 
> 1.7.1

Re: [PATCH v2 01/12] xenctx: Clean up stack trace when hypercall_page not in symbol table

[Ian Campbell]

On Thu, 2013-11-07 at 08:07 +0000, Jan Beulich wrote:
> And a note on patch submission: You send _to_ the list, and _cc_
> maintainers and other relevant people (without stretching the
> meaning of "relevant" too much).

Do we need such a rule? Why is it useful?

I personally don't mind which field causes the mail to arrive in my
INBOX. I suppose you filter "To: xen-devel; Cc: Jan" differently to "To:
Jan; Cc: xen-devel"?

http://wiki.xenproject.org/wiki/Submitting_Xen_Patches#Cc_the_maintainer_of_the_code_you_are_modifying is confusingly worded and somewhat contradictory, in that it initially requests to send to the maintainer and then later to cc them. As I say, I don't think it really matters which but if you care then it would be useful to clarify the wording there.

Ian.

Re: [PATCH v2 01/12] xenctx: Clean up stack trace when hypercall_page not in symbol table

[Jan Beulich]

>>> On 07.11.13 at 09:22, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2013-11-07 at 08:07 +0000, Jan Beulich wrote:
>> And a note on patch submission: You send _to_ the list, and _cc_
>> maintainers and other relevant people (without stretching the
>> meaning of "relevant" too much).
> 
> Do we need such a rule? Why is it useful?
> 
> I personally don't mind which field causes the mail to arrive in my
> INBOX. I suppose you filter "To: xen-devel; Cc: Jan" differently to "To:
> Jan; Cc: xen-devel"?

Exactly. Mails Cc-ed to me get treated almost equally to all other
xen-devel traffic, whereas mails directed at me mean to me that
a response or other kind of action is expected. Basically the
usual (common sense?) email rules...

So maybe I was too strict about the maintainer part - I don't mind
being sent mails to me that fall under the areas I'm maintainer of,
since there I'm obviously requested to take some sort of action.
But in the case here I certainly feel it was wrong to send the
whole series _to_ everyone.

> http://wiki.xenproject.org/wiki/Submitting_Xen_Patches#Cc_the_maintainer_of_ 
> the_code_you_are_modifying is confusingly worded and somewhat contradictory, 
> in that it initially requests to send to the maintainer and then later to cc 
> them. As I say, I don't think it really matters which but if you care then it 
> would be useful to clarify the wording there.

Yes, I think we should.

Jan

Re: [PATCH v2 01/12] xenctx: Clean up stack trace when hypercall_page not in symbol table

[Ian Campbell]

On Thu, 2013-11-07 at 08:47 +0000, Jan Beulich wrote:
> >>> On 07.11.13 at 09:22, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Thu, 2013-11-07 at 08:07 +0000, Jan Beulich wrote:
> >> And a note on patch submission: You send _to_ the list, and _cc_
> >> maintainers and other relevant people (without stretching the
> >> meaning of "relevant" too much).
> > 
> > Do we need such a rule? Why is it useful?
> > 
> > I personally don't mind which field causes the mail to arrive in my
> > INBOX. I suppose you filter "To: xen-devel; Cc: Jan" differently to "To:
> > Jan; Cc: xen-devel"?
> 
> Exactly. Mails Cc-ed to me get treated almost equally to all other
> xen-devel traffic, whereas mails directed at me mean to me that
> a response or other kind of action is expected. Basically the
> usual (common sense?) email rules...
> 
> So maybe I was too strict about the maintainer part - I don't mind
> being sent mails to me that fall under the areas I'm maintainer of,
> since there I'm obviously requested to take some sort of action.
> But in the case here I certainly feel it was wrong to send the
> whole series _to_ everyone.

Ah, yes that makes sense, I hadn't noticed that bit.

> > http://wiki.xenproject.org/wiki/Submitting_Xen_Patches#Cc_the_maintainer_of_ 
> > the_code_you_are_modifying is confusingly worded and somewhat contradictory, 
> > in that it initially requests to send to the maintainer and then later to cc 
> > them. As I say, I don't think it really matters which but if you care then it 
> > would be useful to clarify the wording there.
> 
> Yes, I think we should.

Re: [PATCH v2 01/12] xenctx: Clean up stack trace when hypercall_page not in symbol table

[Don Slutz]

[-- Attachment #1.1: Type: text/plain, Size: 3268 bytes --]

On 11/07/13 07:36, Ian Campbell wrote:
> On Thu, 2013-11-07 at 08:47 +0000, Jan Beulich wrote:
>>>>> On 07.11.13 at 09:22, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Thu, 2013-11-07 at 08:07 +0000, Jan Beulich wrote:
>>>> And a note on patch submission: You send _to_ the list, and _cc_
>>>> maintainers and other relevant people (without stretching the
>>>> meaning of "relevant" too much).
>>> Do we need such a rule? Why is it useful?
>>>
>>> I personally don't mind which field causes the mail to arrive in my
>>> INBOX. I suppose you filter "To: xen-devel; Cc: Jan" differently to "To:
>>> Jan; Cc: xen-devel"?
>> Exactly. Mails Cc-ed to me get treated almost equally to all other
>> xen-devel traffic, whereas mails directed at me mean to me that
>> a response or other kind of action is expected. Basically the
>> usual (common sense?) email rules...
>>
>> So maybe I was too strict about the maintainer part - I don't mind
>> being sent mails to me that fall under the areas I'm maintainer of,
>> since there I'm obviously requested to take some sort of action.
>> But in the case here I certainly feel it was wrong to send the
>> whole series _to_ everyone.
> Ah, yes that makes sense, I hadn't noticed that bit.
Well, I was confused.  get_maintainer.pl reports that

George Dunlap <george.dunlap@eu.citrix.com> (supporter:XENTRACE)
Ian Jackson <ian.jackson@eu.citrix.com> (supporter:TOOLSTACK)
Stefano Stabellini <stefano.stabellini@eu.citrix.com> (supporter:TOOLSTACK)
Ian Campbell <ian.campbell@citrix.com> (supporter:TOOLSTACK)

 From MAINTAINERS:

         M: Mail patches to: FullName <address@domain>

and I followed that.  It was months ago that I read over the wiki page.

Jan came from:

    Subject: 	Re: [Xen-devel] [PATCH] xenctx: Add an option to output
    more registers.
    Date: 	Fri, 18 Oct 2013 09:00:05 +0100
    From: 	Jan Beulich <JBeulich@suse.com>
    To: 	Don Slutz <dslutz@verizon.com>
    CC: 	Ian Campbell <ian.campbell@citrix.com>, Don Slutz
    <Don@CloudSwitch.com>, George Dunlap <george.dunlap@eu.citrix.com>,
    Ian Jackson <ian.jackson@eu.citrix.com>, Stefano Stabellini
    <stefano.stabellini@eu.citrix.com>, xen-devel
    <xen-devel@lists.xenproject.org>



    >>> On 17.10.13 at 20:41, Don Slutz <dslutz@verizon.com> wrote:
    > From: Don Slutz <Don@CloudSwitch.com>
    >
    > Also fixup handling of symbol files, and output of 2 page stacks, and output
    > of non stack memory.

    Considering the size of the change this should be broken up, and
    the individual changes need to be described better in the commit
    message.
    [...]

and my understanding was that re-worked patches need to have 
--to=<responder> added.  One on the things that is not clear is do I 
change from --cc= to --to=after a response?

>>> http://wiki.xenproject.org/wiki/Submitting_Xen_Patches#Cc_the_maintainer_of_
>>> the_code_you_are_modifying is confusingly worded and somewhat contradictory,
>>> in that it initially requests to send to the maintainer and then later to cc
>>> them. As I say, I don't think it really matters which but if you care then it
>>> would be useful to clarify the wording there.
>> Yes, I think we should.
I would agree.  I would also change the wording in MAINTAINERS.

    -Don Slutz

[-- Attachment #1.2: Type: text/html, Size: 6733 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 01/12] xenctx: Clean up stack trace when hypercall_page not in symbol table

[Ian Campbell]

On Wed, 2013-11-06 at 15:08 -0500, Don Slutz wrote:
> From: Don Slutz <Don@CloudSwitch.com>
> 
> Before:
> 
> Call Trace:
>   [<ffffffff8006b2b0>] default_idle+0x29  <--
>   [<ffffffff80048d19>] cpu_idle+0x95
>   [<ffffffff803e7801>] start_kernel+0x220
>   [<0000000000000000>] startup_64+0x80000000
>   [<ffffffff803e722f>] x86_64_start_kernel+0x22f
>   [<0000000000000000>] startup_64+0x80000000
>   [<0000000000000000>] startup_64+0x80000000
>   [<0000000000000000>] startup_64+0x80000000
>   [<0000000000000000>] startup_64+0x80000000
> 
> After:
> 
> Call Trace:
>   [<ffffffff8006b2b0>] default_idle+0x29  <--
>   [<ffffffff80048d19>] cpu_idle+0x95
>   [<ffffffff803e7801>] start_kernel+0x220
>   [<ffffffff803e722f>] x86_64_start_kernel+0x22f
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 02/12] xenctx: Add -2 (--two-pages) option to switch stack size to 8KiB

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 10292fa..06a8850 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -35,6 +35,7 @@ static struct xenctx {
     int frame_ptrs;
     int stack_trace;
     int disp_all;
+    int two_pages;
     int all_vcpus;
     int self_paused;
     xc_dominfo_t dominfo;
@@ -650,9 +651,11 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
 
     stack_limit = ((stack_pointer(ctx) + XC_PAGE_SIZE)
                    & ~((guest_word_t) XC_PAGE_SIZE - 1));
+    if (xenctx.two_pages)
+        stack_limit += XC_PAGE_SIZE;
     printf("\n");
     printf("Stack:\n");
-    for (i=1; i<5 && stack < stack_limit; i++) {
+    for (i=1; i<10 && stack < stack_limit; i++) {
         while(stack < stack_limit && stack < stack_pointer(ctx) + i*32) {
             p = map_page(ctx, vcpu, stack);
             if (!p)
@@ -821,6 +824,7 @@ static void usage(void)
     printf("  -k, --kernel-start\n");
     printf("                    set user/kernel split. (default 0xc0000000)\n");
     printf("  -a --all          display more registers\n");
+    printf("  -2, --two-pages   assume the kernel was compiled with 8KiB stacks.\n");
     printf("  -C --all-vcpus    print info for all vcpus\n");
 }
 
@@ -828,12 +832,13 @@ int main(int argc, char **argv)
 {
     int ch;
     int ret;
-    static const char *sopts = "fs:hak:SC";
+    static const char *sopts = "fs:hak:SC2";
     static const struct option lopts[] = {
         {"stack-trace", 0, NULL, 'S'},
         {"symbol-table", 1, NULL, 's'},
         {"frame-pointers", 0, NULL, 'f'},
         {"kernel-start", 1, NULL, 'k'},
+        {"two-pages", 0, NULL, '2'},
         {"all", 0, NULL, 'a'},
         {"all-vcpus", 0, NULL, 'C'},
         {"help", 0, NULL, 'h'},
@@ -857,6 +862,9 @@ int main(int argc, char **argv)
         case 'a':
             xenctx.disp_all = 1;
             break;
+        case '2':
+            xenctx.two_pages = 1;
+            break;
         case 'C':
             xenctx.all_vcpus = 1;
             break;
-- 
1.7.1

Re: [PATCH v2 02/12] xenctx: Add -2 (--two-pages) option to switch stack size to 8KiB

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> @@ -650,9 +651,11 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>  
>      stack_limit = ((stack_pointer(ctx) + XC_PAGE_SIZE)
>                     & ~((guest_word_t) XC_PAGE_SIZE - 1));
> +    if (xenctx.two_pages)
> +        stack_limit += XC_PAGE_SIZE;
>      printf("\n");
>      printf("Stack:\n");
> -    for (i=1; i<5 && stack < stack_limit; i++) {
> +    for (i=1; i<10 && stack < stack_limit; i++) {

Touching a malformed line like this makes it almost mandatory imo
to fix the coding style (missing blanks around = and <).

Apart from that, the change isn't really related to the subject of
the patch afaict, and as you're making thing more flexible anyway,
it would seem reasonable to also have a command line option to
control this limit.

Jan

Re: [PATCH v2 02/12] xenctx: Add -2 (--two-pages) option to switch stack size to 8KiB

[Ian Campbell]

On Thu, 2013-11-07 at 08:04 +0000, Jan Beulich wrote:
> >>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> > @@ -650,9 +651,11 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
> >  
> >      stack_limit = ((stack_pointer(ctx) + XC_PAGE_SIZE)
> >                     & ~((guest_word_t) XC_PAGE_SIZE - 1));
> > +    if (xenctx.two_pages)
> > +        stack_limit += XC_PAGE_SIZE;
> >      printf("\n");
> >      printf("Stack:\n");
> > -    for (i=1; i<5 && stack < stack_limit; i++) {
> > +    for (i=1; i<10 && stack < stack_limit; i++) {
> 
> Touching a malformed line like this makes it almost mandatory imo
> to fix the coding style (missing blanks around = and <).
> 
> Apart from that, the change isn't really related to the subject of
> the patch afaict, and as you're making thing more flexible anyway,
> it would seem reasonable to also have a command line option to
> control this limit.

Or derive it from stack_limit and/or the number of pages?

What about kernels which use 4 pages for their stack ;-)

Ian.

Re: [PATCH v2 02/12] xenctx: Add -2 (--two-pages) option to switch stack size to 8KiB

[Don Slutz]

On 11/07/13 07:40, Ian Campbell wrote:
> On Thu, 2013-11-07 at 08:04 +0000, Jan Beulich wrote:
>>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>>> @@ -650,9 +651,11 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>>>   
>>>       stack_limit = ((stack_pointer(ctx) + XC_PAGE_SIZE)
>>>                      & ~((guest_word_t) XC_PAGE_SIZE - 1));
>>> +    if (xenctx.two_pages)
>>> +        stack_limit += XC_PAGE_SIZE;
>>>       printf("\n");
>>>       printf("Stack:\n");
>>> -    for (i=1; i<5 && stack < stack_limit; i++) {
>>> +    for (i=1; i<10 && stack < stack_limit; i++) {
>> Touching a malformed line like this makes it almost mandatory imo
>> to fix the coding style (missing blanks around = and <).
Will do.
>>
>> Apart from that, the change isn't really related to the subject of
>> the patch afaict, and as you're making thing more flexible anyway,
>> it would seem reasonable to also have a command line option to
>> control this limit.
Part 1 is.  Part 2 ("i<5" to "i<10") is not, it is part of patch #3 (and 
should have been there) to keep the amount output the same.  A new 
option does make sense; will add it.
> Or derive it from stack_limit and/or the number of pages?
Not sure this is better then a new option. stack_limit currently reduces 
the amount of output.
> What about kernels which use 4 pages for their stack ;-)
Patch #8 would allow you to do this, 2 pages at a time. (or  could also 
add -4...)
    -Don Slutz

> Ian.
>

[PATCH v2 03/12] xenctx: Output ascii version of stack also.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 06a8850..dabce16 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -640,6 +640,8 @@ static int print_code(vcpu_guest_context_any_t *ctx, int vcpu)
     return 0;
 }
 
+#define BYTES_PER_LINE 32
+
 static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
 {
     guest_word_t stack = stack_pointer(ctx);
@@ -647,6 +649,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
     guest_word_t frame;
     guest_word_t word;
     guest_word_t *p;
+    guest_word_t ascii[BYTES_PER_LINE/4];
+    unsigned char *bytep;
     int i;
 
     stack_limit = ((stack_pointer(ctx) + XC_PAGE_SIZE)
@@ -656,15 +660,36 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
     printf("\n");
     printf("Stack:\n");
     for (i=1; i<10 && stack < stack_limit; i++) {
-        while(stack < stack_limit && stack < stack_pointer(ctx) + i*32) {
+        int j = 0;
+        int k;
+
+        while(stack < stack_limit && stack < stack_pointer(ctx) + i*BYTES_PER_LINE) {
             p = map_page(ctx, vcpu, stack);
             if (!p)
                 return -1;
             word = read_stack_word(p, width);
+            ascii[j++] = word;
             printf(" ");
             print_stack_word(word, width);
             stack += width;
         }
+        printf("  ");
+        for (k = j; k < BYTES_PER_LINE/width; k++)
+            printf("%s ", width == 8
+                   ? "                "
+                   : "        ");
+        for (k = 0; k < j; k++) {
+            int l;
+
+            bytep = (unsigned char*)&ascii[k];
+            for (l = 0; l < width; l++) {
+                if ((*bytep < 127) && (*bytep >= 32))
+                    printf("%c", *bytep);
+                else
+                    printf(".");
+                bytep++;
+            }
+        }
         printf("\n");
     }
     printf("\n");
-- 
1.7.1

Re: [PATCH v2 03/12] xenctx: Output ascii version of stack also.

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:

Why? I personally don't see a need - there's rarely much useful
ASCII data on the stack. So afaic: If at all, then only via command
line option defaulting to off.

Jan

> From: Don Slutz <Don@CloudSwitch.com>
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> ---
>  tools/xentrace/xenctx.c |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index 06a8850..dabce16 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -640,6 +640,8 @@ static int print_code(vcpu_guest_context_any_t *ctx, int 
> vcpu)
>      return 0;
>  }
>  
> +#define BYTES_PER_LINE 32
> +
>  static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>  {
>      guest_word_t stack = stack_pointer(ctx);
> @@ -647,6 +649,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int 
> vcpu, int width)
>      guest_word_t frame;
>      guest_word_t word;
>      guest_word_t *p;
> +    guest_word_t ascii[BYTES_PER_LINE/4];
> +    unsigned char *bytep;
>      int i;
>  
>      stack_limit = ((stack_pointer(ctx) + XC_PAGE_SIZE)
> @@ -656,15 +660,36 @@ static int print_stack(vcpu_guest_context_any_t *ctx, 
> int vcpu, int width)
>      printf("\n");
>      printf("Stack:\n");
>      for (i=1; i<10 && stack < stack_limit; i++) {
> -        while(stack < stack_limit && stack < stack_pointer(ctx) + i*32) {
> +        int j = 0;
> +        int k;
> +
> +        while(stack < stack_limit && stack < stack_pointer(ctx) + 
> i*BYTES_PER_LINE) {
>              p = map_page(ctx, vcpu, stack);
>              if (!p)
>                  return -1;
>              word = read_stack_word(p, width);
> +            ascii[j++] = word;
>              printf(" ");
>              print_stack_word(word, width);
>              stack += width;
>          }
> +        printf("  ");
> +        for (k = j; k < BYTES_PER_LINE/width; k++)
> +            printf("%s ", width == 8
> +                   ? "                "
> +                   : "        ");
> +        for (k = 0; k < j; k++) {
> +            int l;
> +
> +            bytep = (unsigned char*)&ascii[k];
> +            for (l = 0; l < width; l++) {
> +                if ((*bytep < 127) && (*bytep >= 32))
> +                    printf("%c", *bytep);
> +                else
> +                    printf(".");
> +                bytep++;
> +            }
> +        }
>          printf("\n");
>      }
>      printf("\n");
> -- 
> 1.7.1

Re: [PATCH v2 03/12] xenctx: Output ascii version of stack also.

[Ian Campbell]

On Thu, 2013-11-07 at 08:09 +0000, Jan Beulich wrote:
> >>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> 
> Why? I personally don't see a need - there's rarely much useful
> ASCII data on the stack.

This is true IME

> So afaic: If at all, then only via command
> line option defaulting to off.

Well, assuming it doesn't make the output >80 columns (which I can't
tell from here) it seems harmless enough?

>        ");
> > +        for (k = 0; k < j; k++) {
> > +            int l;
> > +
> > +            bytep = (unsigned char*)&ascii[k];
> > +            for (l = 0; l < width; l++) {
> > +                if ((*bytep < 127) && (*bytep >= 32))

is there not a ctype.h function which deals with this? isprint seems
like a good candidate...

> > +                    printf("%c", *bytep);
> > +                else
> > +                    printf(".");
> > +                bytep++;
> > +            }
> > +        }
> >          printf("\n");
> >      }
> >      printf("\n");
> > -- 
> > 1.7.1
> 
> 
> 

Re: [PATCH v2 03/12] xenctx: Output ascii version of stack also.

[Jan Beulich]

>>> On 07.11.13 at 13:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2013-11-07 at 08:09 +0000, Jan Beulich wrote:
>> >>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>> 
>> Why? I personally don't see a need - there's rarely much useful
>> ASCII data on the stack.
> 
> This is true IME
> 
>> So afaic: If at all, then only via command
>> line option defaulting to off.
> 
> Well, assuming it doesn't make the output >80 columns (which I can't
> tell from here) it seems harmless enough?

If you end up looking at the output in a wide enough window, it'll
make you scroll more.

Jan

Re: [PATCH v2 03/12] xenctx: Output ascii version of stack also.

[Jan Beulich]

>>> On 07.11.13 at 14:17, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 07.11.13 at 13:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Thu, 2013-11-07 at 08:09 +0000, Jan Beulich wrote:
>>> >>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>>> 
>>> Why? I personally don't see a need - there's rarely much useful
>>> ASCII data on the stack.
>> 
>> This is true IME
>> 
>>> So afaic: If at all, then only via command
>>> line option defaulting to off.
>> 
>> Well, assuming it doesn't make the output >80 columns (which I can't
>> tell from here) it seems harmless enough?
> 
> If you end up looking at the output in a wide enough window, it'll
> make you scroll more.

Oops, I was mentally in patch 04 here. So yes, it may be harmless,
but clutters the data that you need to look at, and I personally
prefer to look at _just_ the data that's interesting, without having
to (mentally or physically) discard other pieces.

Jan

Re: [PATCH v2 03/12] xenctx: Output ascii version of stack also.

[Don Slutz]

On 11/07/13 08:21, Jan Beulich wrote:
>>>> On 07.11.13 at 14:17, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>> On 07.11.13 at 13:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Thu, 2013-11-07 at 08:09 +0000, Jan Beulich wrote:
>>>>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>>>> Why? I personally don't see a need - there's rarely much useful
>>>> ASCII data on the stack.
>>> This is true IME
>>>
>>>> So afaic: If at all, then only via command
>>>> line option defaulting to off.
>>> Well, assuming it doesn't make the output >80 columns (which I can't
>>> tell from here) it seems harmless enough?
>> If you end up looking at the output in a wide enough window, it'll
>> make you scroll more.
> Oops, I was mentally in patch 04 here. So yes, it may be harmless,
> but clutters the data that you need to look at, and I personally
> prefer to look at _just_ the data that's interesting, without having
> to (mentally or physically) discard other pieces.
>
> Jan
>
I am happy to make it a command line option.
    -Don Slutz

[PATCH v2 04/12] xenctx: Add stack address to dump, switch to 16 bytes per line.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index dabce16..a20281e 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -640,7 +640,7 @@ static int print_code(vcpu_guest_context_any_t *ctx, int vcpu)
     return 0;
 }
 
-#define BYTES_PER_LINE 32
+#define BYTES_PER_LINE 16
 
 static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
 {
@@ -663,6 +663,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
         int j = 0;
         int k;
 
+        print_stack_word(stack, width);
+        printf(":");
         while(stack < stack_limit && stack < stack_pointer(ctx) + i*BYTES_PER_LINE) {
             p = map_page(ctx, vcpu, stack);
             if (!p)
-- 
1.7.1

Re: [PATCH v2 04/12] xenctx: Add stack address to dump, switch to 16 bytes per line.

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:

Again - why? This makes it just two entries per line for a 64-bit
guest. Pretty little I would say.

Jan

> From: Don Slutz <Don@CloudSwitch.com>
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> ---
>  tools/xentrace/xenctx.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index dabce16..a20281e 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -640,7 +640,7 @@ static int print_code(vcpu_guest_context_any_t *ctx, int 
> vcpu)
>      return 0;
>  }
>  
> -#define BYTES_PER_LINE 32
> +#define BYTES_PER_LINE 16
>  
>  static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>  {
> @@ -663,6 +663,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int 
> vcpu, int width)
>          int j = 0;
>          int k;
>  
> +        print_stack_word(stack, width);
> +        printf(":");
>          while(stack < stack_limit && stack < stack_pointer(ctx) + 
> i*BYTES_PER_LINE) {
>              p = map_page(ctx, vcpu, stack);
>              if (!p)
> -- 
> 1.7.1

Re: [PATCH v2 04/12] xenctx: Add stack address to dump, switch to 16 bytes per line.

[Ian Campbell]

On Thu, 2013-11-07 at 08:12 +0000, Jan Beulich wrote:
> >>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> 
> Again - why?

Having the stack address there can make it easier to locate a word
referenced by a register for example, without error prone counting. In
general many of the changelogs in this series could do with a second
sentence in them after the title giving some rationale so we don't have
to guess.

>  This makes it just two entries per line for a 64-bit
> guest. Pretty little I would say.

Agreed.

I expect that along with the previous patch lines would now be over 80
columns for sure. If that's the case then either the ascii or the
address (or both) needs to be optional IMHO.

> 
> Jan
> 
> > From: Don Slutz <Don@CloudSwitch.com>
> > 
> > Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> > ---
> >  tools/xentrace/xenctx.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> > index dabce16..a20281e 100644
> > --- a/tools/xentrace/xenctx.c
> > +++ b/tools/xentrace/xenctx.c
> > @@ -640,7 +640,7 @@ static int print_code(vcpu_guest_context_any_t *ctx, int 
> > vcpu)
> >      return 0;
> >  }
> >  
> > -#define BYTES_PER_LINE 32
> > +#define BYTES_PER_LINE 16
> >  
> >  static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
> >  {
> > @@ -663,6 +663,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int 
> > vcpu, int width)
> >          int j = 0;
> >          int k;
> >  
> > +        print_stack_word(stack, width);
> > +        printf(":");
> >          while(stack < stack_limit && stack < stack_pointer(ctx) + 
> > i*BYTES_PER_LINE) {
> >              p = map_page(ctx, vcpu, stack);
> >              if (!p)
> > -- 
> > 1.7.1
> 
> 
> 

Re: [PATCH v2 04/12] xenctx: Add stack address to dump, switch to 16 bytes per line.

[Don Slutz]

On 11/07/13 07:46, Ian Campbell wrote:
> On Thu, 2013-11-07 at 08:12 +0000, Jan Beulich wrote:
>>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>> Again - why?
> Having the stack address there can make it easier to locate a word
> referenced by a register for example, without error prone counting. In
> general many of the changelogs in this series could do with a second
> sentence in them after the title giving some rationale so we don't have
> to guess.
>
>>   This makes it just two entries per line for a 64-bit
>> guest. Pretty little I would say.
> Agreed.
>
> I expect that along with the previous patch lines would now be over 80
> columns for sure. If that's the case then either the ascii or the
> address (or both) needs to be optional IMHO.
>
>> Jan
>>
I am happy to make it a command line option.
    -Don Slutz

[PATCH v2 05/12] xenctx: Change print_symbol to do the space before.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

This stops the output of an extra space at the end of the line.

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index a20281e..5e2354c 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -158,9 +158,9 @@ static void print_symbol(guest_word_t addr)
         return;
 
     if (addr==s->address)
-        printf("%s ", s->name);
+        printf(" %s", s->name);
     else
-        printf("%s+%#x ", s->name, (unsigned int)(addr - s->address));
+        printf(" %s+%#x", s->name, (unsigned int)(addr - s->address));
 }
 
 static void read_symbol_table(const char *symtab)
@@ -287,7 +287,7 @@ static void print_ctx_32(vcpu_guest_context_x86_32_t *ctx)
 {
     struct cpu_user_regs_x86_32 *regs = &ctx->user_regs;
 
-    printf("cs:eip: %04x:%08x ", regs->cs, regs->eip);
+    printf("cs:eip: %04x:%08x", regs->cs, regs->eip);
     print_symbol(regs->eip);
     print_flags(regs->eflags);
     printf("ss:esp: %04x:%08x\n", regs->ss, regs->esp);
@@ -316,7 +316,7 @@ static void print_ctx_32on64(vcpu_guest_context_x86_64_t *ctx)
 {
     struct cpu_user_regs_x86_64 *regs = &ctx->user_regs;
 
-    printf("cs:eip: %04x:%08x ", regs->cs, (uint32_t)regs->eip);
+    printf("cs:eip: %04x:%08x", regs->cs, (uint32_t)regs->eip);
     print_symbol((uint32_t)regs->eip);
     print_flags((uint32_t)regs->eflags);
     printf("ss:esp: %04x:%08x\n", regs->ss, (uint32_t)regs->esp);
@@ -345,7 +345,7 @@ static void print_ctx_64(vcpu_guest_context_x86_64_t *ctx)
 {
     struct cpu_user_regs_x86_64 *regs = &ctx->user_regs;
 
-    printf("rip: %016"PRIx64" ", regs->rip);
+    printf("rip: %016"PRIx64, regs->rip);
     print_symbol(regs->rip);
     print_flags(regs->rflags);
     printf("rsp: %016"PRIx64"\n", regs->rsp);
@@ -443,7 +443,7 @@ static void print_ctx_32(vcpu_guest_context_t *ctx)
 {
     vcpu_guest_core_regs_t *regs = &ctx->user_regs;
 
-    printf("PC:       %08"PRIx32" ", regs->pc32);
+    printf("PC:       %08"PRIx32, regs->pc32);
     print_symbol(regs->pc32);
     printf("\n");
     printf("CPSR:     %08"PRIx32"\n", regs->cpsr);
@@ -495,7 +495,7 @@ static void print_ctx_64(vcpu_guest_context_t *ctx)
 {
     vcpu_guest_core_regs_t *regs = &ctx->user_regs;
 
-    printf("PC:       %016"PRIx64" ", regs->pc64);
+    printf("PC:       %016"PRIx64, regs->pc64);
     print_symbol(regs->pc64);
     printf("\n");
 
@@ -702,7 +702,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
         printf("Call Trace:\n");
     printf("%c [<", xenctx.stack_trace ? '*' : ' ');
     print_stack_word(instr_pointer(ctx), width);
-    printf(">] ");
+    printf(">]");
 
     print_symbol(instr_pointer(ctx));
     printf(" <--\n");
@@ -742,7 +742,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
                 word = read_stack_word(p, width);
                 printf("%c [<", xenctx.stack_trace ? '|' : ' ');
                 print_stack_word(word, width);
-                printf(">] ");
+                printf(">]");
                 print_symbol(word);
                 printf("\n");
                 stack += width;
@@ -758,7 +758,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
             if (is_kernel_text(word)) {
                 printf("  [<");
                 print_stack_word(word, width);
-                printf(">] ");
+                printf(">]");
                 print_symbol(word);
                 printf("\n");
             } else if (xenctx.stack_trace) {
-- 
1.7.1

Re: [PATCH v2 05/12] xenctx: Change print_symbol to do the space before.

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> From: Don Slutz <Don@CloudSwitch.com>
> 
> This stops the output of an extra space at the end of the line.
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> ---
>  tools/xentrace/xenctx.c |   20 ++++++++++----------
>  1 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index a20281e..5e2354c 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -158,9 +158,9 @@ static void print_symbol(guest_word_t addr)
>          return;
>  
>      if (addr==s->address)
> -        printf("%s ", s->name);
> +        printf(" %s", s->name);
>      else
> -        printf("%s+%#x ", s->name, (unsigned int)(addr - s->address));
> +        printf(" %s+%#x", s->name, (unsigned int)(addr - s->address));
>  }
>  
>  static void read_symbol_table(const char *symtab)
> @@ -287,7 +287,7 @@ static void print_ctx_32(vcpu_guest_context_x86_32_t 
> *ctx)
>  {
>      struct cpu_user_regs_x86_32 *regs = &ctx->user_regs;
>  
> -    printf("cs:eip: %04x:%08x ", regs->cs, regs->eip);
> +    printf("cs:eip: %04x:%08x", regs->cs, regs->eip);
>      print_symbol(regs->eip);
>      print_flags(regs->eflags);
>      printf("ss:esp: %04x:%08x\n", regs->ss, regs->esp);
> @@ -316,7 +316,7 @@ static void print_ctx_32on64(vcpu_guest_context_x86_64_t 
> *ctx)
>  {
>      struct cpu_user_regs_x86_64 *regs = &ctx->user_regs;
>  
> -    printf("cs:eip: %04x:%08x ", regs->cs, (uint32_t)regs->eip);
> +    printf("cs:eip: %04x:%08x", regs->cs, (uint32_t)regs->eip);
>      print_symbol((uint32_t)regs->eip);
>      print_flags((uint32_t)regs->eflags);
>      printf("ss:esp: %04x:%08x\n", regs->ss, (uint32_t)regs->esp);
> @@ -345,7 +345,7 @@ static void print_ctx_64(vcpu_guest_context_x86_64_t 
> *ctx)
>  {
>      struct cpu_user_regs_x86_64 *regs = &ctx->user_regs;
>  
> -    printf("rip: %016"PRIx64" ", regs->rip);
> +    printf("rip: %016"PRIx64, regs->rip);
>      print_symbol(regs->rip);
>      print_flags(regs->rflags);
>      printf("rsp: %016"PRIx64"\n", regs->rsp);
> @@ -443,7 +443,7 @@ static void print_ctx_32(vcpu_guest_context_t *ctx)
>  {
>      vcpu_guest_core_regs_t *regs = &ctx->user_regs;
>  
> -    printf("PC:       %08"PRIx32" ", regs->pc32);
> +    printf("PC:       %08"PRIx32, regs->pc32);
>      print_symbol(regs->pc32);
>      printf("\n");
>      printf("CPSR:     %08"PRIx32"\n", regs->cpsr);
> @@ -495,7 +495,7 @@ static void print_ctx_64(vcpu_guest_context_t *ctx)
>  {
>      vcpu_guest_core_regs_t *regs = &ctx->user_regs;
>  
> -    printf("PC:       %016"PRIx64" ", regs->pc64);
> +    printf("PC:       %016"PRIx64, regs->pc64);
>      print_symbol(regs->pc64);
>      printf("\n");
>  
> @@ -702,7 +702,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int 
> vcpu, int width)
>          printf("Call Trace:\n");
>      printf("%c [<", xenctx.stack_trace ? '*' : ' ');
>      print_stack_word(instr_pointer(ctx), width);
> -    printf(">] ");
> +    printf(">]");
>  
>      print_symbol(instr_pointer(ctx));
>      printf(" <--\n");
> @@ -742,7 +742,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int 
> vcpu, int width)
>                  word = read_stack_word(p, width);
>                  printf("%c [<", xenctx.stack_trace ? '|' : ' ');
>                  print_stack_word(word, width);
> -                printf(">] ");
> +                printf(">]");
>                  print_symbol(word);
>                  printf("\n");
>                  stack += width;
> @@ -758,7 +758,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int 
> vcpu, int width)
>              if (is_kernel_text(word)) {
>                  printf("  [<");
>                  print_stack_word(word, width);
> -                printf(">] ");
> +                printf(">]");
>                  print_symbol(word);
>                  printf("\n");
>              } else if (xenctx.stack_trace) {
> -- 
> 1.7.1

Re: [PATCH v2 05/12] xenctx: Change print_symbol to do the space before.

[Ian Campbell]

On Wed, 2013-11-06 at 15:08 -0500, Don Slutz wrote:
> From: Don Slutz <Don@CloudSwitch.com>
> 
> This stops the output of an extra space at the end of the line.
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 06/12] xenctx: More info on failed to map page.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

Also output an extra new line since we may be in the middle of output.

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 5e2354c..dcf431c 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -593,7 +593,7 @@ static void *map_page(vcpu_guest_context_any_t *ctx, int vcpu, guest_word_t virt
     mapped = xc_map_foreign_range(xenctx.xc_handle, xenctx.domid, XC_PAGE_SIZE, PROT_READ, mfn);
 
     if (mapped == NULL) {
-        fprintf(stderr, "failed to map page.\n");
+        fprintf(stderr, "\nfailed to map page for "FMT_32B_WORD".\n", virt);
         return NULL;
     }
 
-- 
1.7.1

Re: [PATCH v2 06/12] xenctx: More info on failed to map page.

[Ian Campbell]

On Wed, 2013-11-06 at 15:08 -0500, Don Slutz wrote:
> From: Don Slutz <Don@CloudSwitch.com>
> 
> Also output an extra new line since we may be in the middle of output.
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 07/12] xenctx: Add stack addr to call trace.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index dcf431c..4dc6574 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -700,6 +700,9 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
         printf("Stack Trace:\n");
     else
         printf("Call Trace:\n");
+    printf("%s ", width == 8
+           ? "                "
+           : "        ");
     printf("%c [<", xenctx.stack_trace ? '*' : ' ');
     print_stack_word(instr_pointer(ctx), width);
     printf(">]");
@@ -715,9 +718,10 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
                     p = map_page(ctx, vcpu, stack);
                     if (!p)
                         return -1;
-                    printf("|   ");
+                    print_stack_word(stack, width);
+                    printf(": |   ");
                     print_stack_word(read_stack_word(p, width), width);
-                    printf("   \n");
+                    printf("\n");
                     stack += width;
                 }
             } else {
@@ -729,7 +733,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
                 return -1;
             frame = read_stack_word(p, width);
             if (xenctx.stack_trace) {
-                printf("|-- ");
+                print_stack_word(stack, width);
+                printf(": |-- ");
                 print_stack_word(read_stack_word(p, width), width);
                 printf("\n");
             }
@@ -740,7 +745,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
                 if (!p)
                     return -1;
                 word = read_stack_word(p, width);
-                printf("%c [<", xenctx.stack_trace ? '|' : ' ');
+                print_stack_word(stack, width);
+                printf(": %c [<", xenctx.stack_trace ? '|' : ' ');
                 print_stack_word(word, width);
                 printf(">]");
                 print_symbol(word);
@@ -756,13 +762,15 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
                 return -1;
             word = read_stack_word(p, width);
             if (is_kernel_text(word)) {
-                printf("  [<");
+                print_stack_word(stack, width);
+                printf(":  [<");
                 print_stack_word(word, width);
                 printf(">]");
                 print_symbol(word);
                 printf("\n");
             } else if (xenctx.stack_trace) {
-                printf("    ");
+                print_stack_word(stack, width);
+                printf(":    ");
                 print_stack_word(word, width);
                 printf("\n");
             }
-- 
1.7.1

Re: [PATCH v2 07/12] xenctx: Add stack addr to call trace.

[Ian Campbell]

On Wed, 2013-11-06 at 15:08 -0500, Don Slutz wrote:
> From: Don Slutz <Don@CloudSwitch.com>

Can you give an example of the output please.

What is "stack address" is it the base of the function's stack frame
perhaps? Or maybe the top? Or maybe the framepointer?

> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> ---
>  tools/xentrace/xenctx.c |   20 ++++++++++++++------
>  1 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index dcf431c..4dc6574 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -700,6 +700,9 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>          printf("Stack Trace:\n");
>      else
>          printf("Call Trace:\n");
> +    printf("%s ", width == 8
> +           ? "                "
> +           : "        ");

I think this can be done as ("%*s", width*2, "") or something roughly
like that.

>      printf("%c [<", xenctx.stack_trace ? '*' : ' ');
>      print_stack_word(instr_pointer(ctx), width);
>      printf(">]");
> @@ -715,9 +718,10 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>                      p = map_page(ctx, vcpu, stack);
>                      if (!p)
>                          return -1;
> -                    printf("|   ");
> +                    print_stack_word(stack, width);
> +                    printf(": |   ");
>                      print_stack_word(read_stack_word(p, width), width);
> -                    printf("   \n");
> +                    printf("\n");
>                      stack += width;
>                  }
>              } else {
> @@ -729,7 +733,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>                  return -1;
>              frame = read_stack_word(p, width);
>              if (xenctx.stack_trace) {
> -                printf("|-- ");
> +                print_stack_word(stack, width);
> +                printf(": |-- ");
>                  print_stack_word(read_stack_word(p, width), width);
>                  printf("\n");
>              }
> @@ -740,7 +745,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>                  if (!p)
>                      return -1;
>                  word = read_stack_word(p, width);
> -                printf("%c [<", xenctx.stack_trace ? '|' : ' ');
> +                print_stack_word(stack, width);
> +                printf(": %c [<", xenctx.stack_trace ? '|' : ' ');
>                  print_stack_word(word, width);
>                  printf(">]");
>                  print_symbol(word);
> @@ -756,13 +762,15 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>                  return -1;
>              word = read_stack_word(p, width);
>              if (is_kernel_text(word)) {
> -                printf("  [<");
> +                print_stack_word(stack, width);
> +                printf(":  [<");
>                  print_stack_word(word, width);
>                  printf(">]");
>                  print_symbol(word);
>                  printf("\n");
>              } else if (xenctx.stack_trace) {
> -                printf("    ");
> +                print_stack_word(stack, width);
> +                printf(":    ");
>                  print_stack_word(word, width);
>                  printf("\n");
>              }

Re: [PATCH v2 07/12] xenctx: Add stack addr to call trace.

[Don Slutz]

[-- Attachment #1.1: Type: text/plain, Size: 5656 bytes --]

On 11/07/13 07:50, Ian Campbell wrote:
> On Wed, 2013-11-06 at 15:08 -0500, Don Slutz wrote:
>> From: Don Slutz <Don@CloudSwitch.com>
> Can you give an example of the output please.
Here it is (With all patches active):

    dcs-xen-54:~/xen>sudo
    /home/don/xen/dist/install/usr/lib/xen/bin/xenctx -s
    /boot/System.map-2.6.18-128.el5 2
    rip: ffffffff8006b2b0 default_idle+0x29
    flags: 00000246 i z p
    rsp: ffffffff803ddf90
    rax: 0000000000000000   rcx: 0000000000000000   rdx: 0000000000000000
    rbx: ffffffff8006b287   rsi: 0000000000000001   rdi: ffffffff802f0658
    rbp: 0000000000086800    r8: ffffffff803dc000    r9: 000000000000003f
    r10: ffff81017d2437c0   r11: 0000000000000282   r12: 0000000000000000
    r13: 0000000000000000   r14: 0000000000000000   r15: 0000000000000000
       cs: 0010 @ 0000000000000000
                /ffffffff(a9b)
       ss: 0018 @ 0000000000000000
                /ffffffff(c93)
       ds: 0018 @ 0000000000000000
                /ffffffff(c93)
       es: 0018 @ 0000000000000000
                /ffffffff(c93)
       fs: 0000 @ 0000000000000000
                /ffffffff(c00)
       gs: 0000 @ ffffffff803ac000\0000000000000000 boot_cpu_pda\
                /ffffffff(c00)
    Code (instr addr ffffffff8006b2b0)
    65 48 8b 04 25 10 00 00 00 8b 80 38 e0 ff ff a8 08 75 04 fb f4 <eb>
    01 fb 65 48 8b 04 25 10 00 00


    Stack:
    ffffffff803ddf90: ffffffff80048d19 0000000000200800 .......... .....
    ffffffff803ddfa0: ffffffff803e7801 0000000000086800 .x>......h......
    ffffffff803ddfb0: 0000000000000000 ffffffff80430720 ........ .C.....
    ffffffff803ddfc0: ffffffff803e722f 80008e000010019c /r>.............
    ffffffff803ddfd0: 00000000ffffffff 0000000000000000 ................
    ffffffff803ddfe0: 0000000000000000 0000000000200000 .......... .....
    ffffffff803ddff0: 0000000000000000 0000000000000000 ................

    Call Trace:
                        [<ffffffff8006b2b0>] default_idle+0x29 <--
    ffffffff803ddf90:  [<ffffffff80048d19>] cpu_idle+0x95
    ffffffff803ddfa0:  [<ffffffff803e7801>] start_kernel+0x220
    ffffffff803ddfc0:  [<ffffffff803e722f>] x86_64_start_kernel+0x22f

>
> What is "stack address" is it the base of the function's stack frame
> perhaps? Or maybe the top? Or maybe the framepointer?
It is the address of the stack "word".
>> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
>> ---
>>   tools/xentrace/xenctx.c |   20 ++++++++++++++------
>>   1 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
>> index dcf431c..4dc6574 100644
>> --- a/tools/xentrace/xenctx.c
>> +++ b/tools/xentrace/xenctx.c
>> @@ -700,6 +700,9 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>>           printf("Stack Trace:\n");
>>       else
>>           printf("Call Trace:\n");
>> +    printf("%s ", width == 8
>> +           ? "                "
>> +           : "        ");
> I think this can be done as ("%*s", width*2, "") or something roughly
> like that.
Will look into it.
    -Don Slutz
>>       printf("%c [<", xenctx.stack_trace ? '*' : ' ');
>>       print_stack_word(instr_pointer(ctx), width);
>>       printf(">]");
>> @@ -715,9 +718,10 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>>                       p = map_page(ctx, vcpu, stack);
>>                       if (!p)
>>                           return -1;
>> -                    printf("|   ");
>> +                    print_stack_word(stack, width);
>> +                    printf(": |   ");
>>                       print_stack_word(read_stack_word(p, width), width);
>> -                    printf("   \n");
>> +                    printf("\n");
>>                       stack += width;
>>                   }
>>               } else {
>> @@ -729,7 +733,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>>                   return -1;
>>               frame = read_stack_word(p, width);
>>               if (xenctx.stack_trace) {
>> -                printf("|-- ");
>> +                print_stack_word(stack, width);
>> +                printf(": |-- ");
>>                   print_stack_word(read_stack_word(p, width), width);
>>                   printf("\n");
>>               }
>> @@ -740,7 +745,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>>                   if (!p)
>>                       return -1;
>>                   word = read_stack_word(p, width);
>> -                printf("%c [<", xenctx.stack_trace ? '|' : ' ');
>> +                print_stack_word(stack, width);
>> +                printf(": %c [<", xenctx.stack_trace ? '|' : ' ');
>>                   print_stack_word(word, width);
>>                   printf(">]");
>>                   print_symbol(word);
>> @@ -756,13 +762,15 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>>                   return -1;
>>               word = read_stack_word(p, width);
>>               if (is_kernel_text(word)) {
>> -                printf("  [<");
>> +                print_stack_word(stack, width);
>> +                printf(":  [<");
>>                   print_stack_word(word, width);
>>                   printf(">]");
>>                   print_symbol(word);
>>                   printf("\n");
>>               } else if (xenctx.stack_trace) {
>> -                printf("    ");
>> +                print_stack_word(stack, width);
>> +                printf(":    ");
>>                   print_stack_word(word, width);
>>                   printf("\n");
>>               }
>


[-- Attachment #1.2: Type: text/html, Size: 7988 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 07/12] xenctx: Add stack addr to call trace.

[Ian Campbell]

On Thu, 2013-11-07 at 09:34 -0500, Don Slutz wrote:
> On 11/07/13 07:50, Ian Campbell wrote:
> 
> > On Wed, 2013-11-06 at 15:08 -0500, Don Slutz wrote:
> > > From: Don Slutz <Don@CloudSwitch.com>
> > Can you give an example of the output please.
> Here it is (With all patches active):
>           cs: 0010 @ 0000000000000000
>                    /ffffffff(a9b)

Is the ugly wrapping real or email whitespace damage?

>           ss: 0018 @ 0000000000000000
>                    /ffffffff(c93)
>           ds: 0018 @ 0000000000000000
>                    /ffffffff(c93)
>           es: 0018 @ 0000000000000000
>                    /ffffffff(c93)
>           fs: 0000 @ 0000000000000000
>                    /ffffffff(c00)
>           gs: 0000 @ ffffffff803ac000\0000000000000000 boot_cpu_pda\
>                    /ffffffff(c00)
[...]
> 
> > What is "stack address" is it the base of the function's stack frame
> > perhaps? Or maybe the top? Or maybe the framepointer?

> It is the address of the stack "word".

I'm afraid I am non the wiser.

Ian.

Re: [PATCH v2 07/12] xenctx: Add stack addr to call trace.

[Don Slutz]

On 11/07/13 09:44, Ian Campbell wrote:
> On Thu, 2013-11-07 at 09:34 -0500, Don Slutz wrote:
>> On 11/07/13 07:50, Ian Campbell wrote:
>>
>>> On Wed, 2013-11-06 at 15:08 -0500, Don Slutz wrote:
>>>> From: Don Slutz <Don@CloudSwitch.com>
>>> Can you give an example of the output please.
>> Here it is (With all patches active):
>>            cs: 0010 @ 0000000000000000
>>                     /ffffffff(a9b)
> Is the ugly wrapping real or email whitespace damage?
Real. The way it currently looks is clearer with a symbol.  Doing both 
the address and symbol and other data is not simple. I was trying to 
keep things aligned which is very hard with variable length output. 
xen-hvmctx's output:

              cs 0x00000010 (0x0000000000000000 + 0xffffffff / 0x00a9b)

Using + to indicate a limit is not so clear either.



Things like idt are simpler.
      idt: ffffffff8042c000/fff idt_table


>>            ss: 0018 @ 0000000000000000
>>                     /ffffffff(c93)
>>            ds: 0018 @ 0000000000000000
>>                     /ffffffff(c93)
>>            es: 0018 @ 0000000000000000
>>                     /ffffffff(c93)
>>            fs: 0000 @ 0000000000000000
>>                     /ffffffff(c00)
>>            gs: 0000 @ ffffffff803ac000\0000000000000000 boot_cpu_pda\
>>                     /ffffffff(c00)
Should this be split into "gs" and "shadow gs"?
> [...]
>>> What is "stack address" is it the base of the function's stack frame
>>> perhaps? Or maybe the top? Or maybe the framepointer?
>> It is the address of the stack "word".
> I'm afraid I am non the wiser.
>
> Ian.
Does just:

   ffffffff803ddf90: ffffffff80048d19 0000000000200800 .......... .....
vs
   ffffffff803ddf90:  [<ffffffff80048d19>] cpu_idle+0x95

help?  The start of the line is the same.
     -Don

Re: [PATCH v2 07/12] xenctx: Add stack addr to call trace.

[Ian Campbell]

On Thu, 2013-11-07 at 10:06 -0500, Don Slutz wrote:

> >>> What is "stack address" is it the base of the function's stack frame
> >>> perhaps? Or maybe the top? Or maybe the framepointer?
> >> It is the address of the stack "word".
> > I'm afraid I am non the wiser.
> >
> > Ian.
> Does just:
> 
>    ffffffff803ddf90: ffffffff80048d19 0000000000200800 .......... .....
> vs
>    ffffffff803ddf90:  [<ffffffff80048d19>] cpu_idle+0x95
> 
> help?  The start of the line is the same.

So it is the address of the word on the stack which contains the return
address for the function call?

Ian.

Re: [PATCH v2 07/12] xenctx: Add stack addr to call trace.

[Don Slutz]

On 11/07/13 11:03, Ian Campbell wrote:
> On Thu, 2013-11-07 at 10:06 -0500, Don Slutz wrote:
>
>>>>> What is "stack address" is it the base of the function's stack frame
>>>>> perhaps? Or maybe the top? Or maybe the framepointer?
>>>> It is the address of the stack "word".
>>> I'm afraid I am non the wiser.
>>>
>>> Ian.
>> Does just:
>>
>>     ffffffff803ddf90: ffffffff80048d19 0000000000200800 .......... .....
>> vs
>>     ffffffff803ddf90:  [<ffffffff80048d19>] cpu_idle+0x95
>>
>> help?  The start of the line is the same.
> So it is the address of the word on the stack which contains the return
> address for the function call?
Yes.
     -Don Slutz
> Ian.
>

Re: [PATCH v2 07/12] xenctx: Add stack addr to call trace.

[Ian Campbell]

On Thu, 2013-11-07 at 13:13 -0500, Don Slutz wrote:
> On 11/07/13 11:03, Ian Campbell wrote:
> > On Thu, 2013-11-07 at 10:06 -0500, Don Slutz wrote:
> >
> >>>>> What is "stack address" is it the base of the function's stack frame
> >>>>> perhaps? Or maybe the top? Or maybe the framepointer?
> >>>> It is the address of the stack "word".
> >>> I'm afraid I am non the wiser.
> >>>
> >>> Ian.
> >> Does just:
> >>
> >>     ffffffff803ddf90: ffffffff80048d19 0000000000200800 .......... .....
> >> vs
> >>     ffffffff803ddf90:  [<ffffffff80048d19>] cpu_idle+0x95
> >>
> >> help?  The start of the line is the same.
> > So it is the address of the word on the stack which contains the return
> > address for the function call?
> Yes.

Is that useful? The outermost word of the stack frame might arguably be
useful, and I'm not even sure of that, but this one I'm even less
convinced by (the two may correspond on some arches, but not all).

Ian.

[PATCH v2 08/12] xenctx: Add -d <daddr> option to dump memory at daddr as a stack.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |   85 ++++++++++++++++++++++++++++-------------------
 1 files changed, 51 insertions(+), 34 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 4dc6574..233f537 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -642,18 +642,21 @@ static int print_code(vcpu_guest_context_any_t *ctx, int vcpu)
 
 #define BYTES_PER_LINE 16
 
-static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
+static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest_word_t stk_addr)
 {
-    guest_word_t stack = stack_pointer(ctx);
+    guest_word_t stk_addr_start = stack_pointer(ctx);
+    guest_word_t stack;
     guest_word_t stack_limit;
     guest_word_t frame;
     guest_word_t word;
-    guest_word_t *p;
     guest_word_t ascii[BYTES_PER_LINE/4];
     unsigned char *bytep;
     int i;
 
-    stack_limit = ((stack_pointer(ctx) + XC_PAGE_SIZE)
+    if (stk_addr && !xenctx.frame_ptrs)
+        stk_addr_start = stk_addr;
+    stack = stk_addr_start;
+    stack_limit = ((stk_addr_start + XC_PAGE_SIZE)
                    & ~((guest_word_t) XC_PAGE_SIZE - 1));
     if (xenctx.two_pages)
         stack_limit += XC_PAGE_SIZE;
@@ -665,8 +668,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
 
         print_stack_word(stack, width);
         printf(":");
-        while(stack < stack_limit && stack < stack_pointer(ctx) + i*BYTES_PER_LINE) {
-            p = map_page(ctx, vcpu, stack);
+        while(stack < stack_limit && stack < stk_addr_start + i*BYTES_PER_LINE) {
+            void *p = map_page(ctx, vcpu, stack);
             if (!p)
                 return -1;
             word = read_stack_word(p, width);
@@ -700,16 +703,19 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
         printf("Stack Trace:\n");
     else
         printf("Call Trace:\n");
-    printf("%s ", width == 8
-           ? "                "
-           : "        ");
-    printf("%c [<", xenctx.stack_trace ? '*' : ' ');
-    print_stack_word(instr_pointer(ctx), width);
-    printf(">]");
-
-    print_symbol(instr_pointer(ctx));
-    printf(" <--\n");
+    if (!stk_addr || xenctx.frame_ptrs) {
+        printf("%s ", width == 8
+               ? "                "
+               : "        ");
+        printf("%c [<", xenctx.stack_trace ? '*' : ' ');
+        print_stack_word(instr_pointer(ctx), width);
+        printf(">]");
+        print_symbol(instr_pointer(ctx));
+        printf(" <--\n");
+    }
     if (xenctx.frame_ptrs) {
+        void *p;
+
         stack = stack_pointer(ctx);
         frame = frame_pointer(ctx);
         while(frame && stack < stack_limit) {
@@ -755,9 +761,9 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
             }
         }
     } else {
-        stack = stack_pointer(ctx);
+        stack = stk_addr_start;
         while(stack < stack_limit) {
-            p = map_page(ctx, vcpu, stack);
+            void *p = map_page(ctx, vcpu, stack);
             if (!p)
                 return -1;
             word = read_stack_word(p, width);
@@ -781,7 +787,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
 }
 #endif
 
-static void dump_ctx(int vcpu)
+static void dump_ctx(int vcpu, guest_word_t stk_addr)
 {
     vcpu_guest_context_any_t ctx;
 
@@ -820,17 +826,21 @@ static void dump_ctx(int vcpu)
     }
 #endif
 
-    print_ctx(&ctx);
+    if (!stk_addr) {
+        print_ctx(&ctx);
+    }
 #ifndef NO_TRANSLATION
-    if (print_code(&ctx, vcpu))
-        return;
-    if (is_kernel_text(instr_pointer(&ctx)))
-        if (print_stack(&ctx, vcpu, guest_word_size))
-            return;
+    if (!stk_addr) {
+        print_code(&ctx, vcpu);
+        if (is_kernel_text(instr_pointer(&ctx)))
+            print_stack(&ctx, vcpu, guest_word_size, stk_addr);
+    } else {
+        print_stack(&ctx, vcpu, guest_word_size, stk_addr);
+    }
 #endif
 }
 
-static void dump_all_vcpus(void)
+static void dump_all_vcpus(guest_word_t stk_addr)
 {
     xc_vcpuinfo_t vinfo;
     int vcpu;
@@ -839,7 +849,7 @@ static void dump_all_vcpus(void)
         if ( xc_vcpu_getinfo(xenctx.xc_handle, xenctx.domid, vcpu, &vinfo) )
             continue;
         if ( vinfo.online )
-            dump_ctx(vcpu);
+            dump_ctx(vcpu, stk_addr);
     }
 }
 
@@ -855,31 +865,35 @@ static void usage(void)
     printf("                    frame pointers.\n");
     printf("  -s SYMTAB, --symbol-table=SYMTAB\n");
     printf("                    read symbol table from SYMTAB.\n");
-    printf("  -S --stack-trace  print a complete stack trace.\n");
-    printf("  -k, --kernel-start\n");
-    printf("                    set user/kernel split. (default 0xc0000000)\n");
-    printf("  -a --all          display more registers\n");
+    printf("  -S, --stack-trace print a complete stack trace.\n");
+    printf("  -k kaddr, --kernel-start=kaddr\n");
+    printf("                    set user/kernel split at kaddr. (default 0xc0000000)\n");
+    printf("  -d daddr, --dump-as-stack=daddr\n");
+    printf("                    dump memory as a stack at daddr.\n");
+    printf("  -a, --all         display more registers\n");
     printf("  -2, --two-pages   assume the kernel was compiled with 8KiB stacks.\n");
-    printf("  -C --all-vcpus    print info for all vcpus\n");
+    printf("  -C, --all-vcpus   print info for all vcpus\n");
 }
 
 int main(int argc, char **argv)
 {
     int ch;
     int ret;
-    static const char *sopts = "fs:hak:SC2";
+    static const char *sopts = "fs:hak:SCd:2";
     static const struct option lopts[] = {
         {"stack-trace", 0, NULL, 'S'},
         {"symbol-table", 1, NULL, 's'},
         {"frame-pointers", 0, NULL, 'f'},
         {"kernel-start", 1, NULL, 'k'},
         {"two-pages", 0, NULL, '2'},
+        {"dump-as-stack", 1, NULL, 'd'},
         {"all", 0, NULL, 'a'},
         {"all-vcpus", 0, NULL, 'C'},
         {"help", 0, NULL, 'h'},
         {0, 0, 0, 0}
     };
     const char *symbol_table = NULL;
+    guest_word_t stk_addr = 0;
 
     int vcpu = 0;
 
@@ -906,6 +920,9 @@ int main(int argc, char **argv)
         case 'k':
             kernel_start = strtoull(optarg, NULL, 0);
             break;
+        case 'd':
+            stk_addr = strtoull(optarg, NULL, 0);
+            break;
         case 'h':
             usage();
             exit(-1);
@@ -956,9 +973,9 @@ int main(int argc, char **argv)
     }
 
     if (xenctx.all_vcpus)
-        dump_all_vcpus();
+        dump_all_vcpus(stk_addr);
     else
-        dump_ctx(vcpu);
+        dump_ctx(vcpu, stk_addr);
 
     if (xenctx.self_paused) {
         ret = xc_domain_unpause(xenctx.xc_handle, xenctx.domid);
-- 
1.7.1

Re: [PATCH v2 08/12] xenctx: Add -d <daddr> option to dump memory at daddr as a stack.

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> @@ -956,9 +973,9 @@ int main(int argc, char **argv)
>      }
>  
>      if (xenctx.all_vcpus)
> -        dump_all_vcpus();
> +        dump_all_vcpus(stk_addr);
>      else
> -        dump_ctx(vcpu);
> +        dump_ctx(vcpu, stk_addr);

For one, it hardly makes much sense to dump all vCPU-s with a
single stack address. And assuming a stack can't sit at address
zero is wrong too - you shouldn't make any assumptions in
particular for HVM guests.

So what you intend here should lead to just the requested
"stack" to be printed, without touching other functionality (i.e.
likely you'll want the "if()" above preceded by another one, and
itself converted to an "else if()").

Jan

Re: [PATCH v2 08/12] xenctx: Add -d <daddr> option to dump memory at daddr as a stack.

[Don Slutz]

On 11/07/13 03:22, Jan Beulich wrote:
>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>> @@ -956,9 +973,9 @@ int main(int argc, char **argv)
>>       }
>>   
>>       if (xenctx.all_vcpus)
>> -        dump_all_vcpus();
>> +        dump_all_vcpus(stk_addr);
>>       else
>> -        dump_ctx(vcpu);
>> +        dump_ctx(vcpu, stk_addr);
> For one, it hardly makes much sense to dump all vCPU-s with a
> single stack address. And assuming a stack can't sit at address
> zero is wrong too - you shouldn't make any assumptions in
> particular for HVM guests.
>
> So what you intend here should lead to just the requested
> "stack" to be printed, without touching other functionality (i.e.
> likely you'll want the "if()" above preceded by another one, and
> itself converted to an "else if()").
>
> Jan
>
It is clear that handling "-d" out side of dump_ctx() make sense. Not so sure on the simple else.  I think that -C should still dump all cpus even if -d is provided.  I guess I could complain if both -d and -C are specified, but I like to just do the requested actions.
    -Don Slutz

[PATCH v2 09/12] xenctx: Add -m <maddr> option to dump memory at maddr.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |  126 ++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 109 insertions(+), 17 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 233f537..d6f8482 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -609,6 +609,33 @@ static guest_word_t read_stack_word(guest_word_t *src, int width)
     return word;
 }
 
+static guest_word_t read_mem_word(vcpu_guest_context_any_t *ctx, int vcpu, guest_word_t virt, int width)
+{
+    if ((virt & 7) == 0) {
+        guest_word_t *p = map_page(ctx, vcpu, virt);
+
+        if (p)
+            return read_stack_word(p, width);
+        else
+            return -1;
+    } else {
+        guest_word_t word = -1;
+        char *src, *dst;
+        int i;
+
+        dst = (char*)&word;
+        for(i = 0; i < width; i++) {
+            src = map_page(ctx, vcpu, virt + i);
+            if (src)
+                *dst++ = *src;
+            else
+                return word;
+        }
+        return word;
+    }
+}
+
+
 static void print_stack_word(guest_word_t word, int width)
 {
     if (width == 4)
@@ -617,6 +644,58 @@ static void print_stack_word(guest_word_t word, int width)
         printf(FMT_64B_WORD, word);
 }
 
+#define BYTES_PER_LINE 16
+
+static void print_mem(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest_word_t mem_addr)
+{
+    guest_word_t instr;
+    guest_word_t instr_start;
+    guest_word_t word;
+    guest_word_t ascii[BYTES_PER_LINE/4];
+    unsigned char *bytep;
+    int i;
+
+    instr_start = mem_addr;
+    instr = mem_addr;
+    printf("Memory (addr %08llx)\n", instr);
+    for (i=1; i<10; i++) {
+        int j = 0;
+        int k;
+
+        print_stack_word(instr, width);
+        printf(":");
+        while(instr < instr_start + i*BYTES_PER_LINE) {
+            void *p = map_page(ctx, vcpu, instr);
+            if (!p)
+                return;
+            word = read_mem_word(ctx, vcpu, instr, width);
+            ascii[j++] = word;
+            printf(" ");
+            print_stack_word(word, width);
+            instr += width;
+        }
+        printf("  ");
+        for (k = j; k < BYTES_PER_LINE/width; k++)
+            printf("%s ", width == 8
+                   ? "                "
+                   : "        ");
+        for (k = 0; k < j; k++) {
+            int l;
+
+            bytep = (unsigned char*)&ascii[k];
+            for (l = 0; l < width; l++) {
+                if ((*bytep < 127) && (*bytep >= 32))
+                    printf("%c", *bytep);
+                else
+                    printf(".");
+                bytep++;
+            }
+        }
+        printf("\n");
+    }
+    printf("\n");
+}
+
 static int print_code(vcpu_guest_context_any_t *ctx, int vcpu)
 {
     guest_word_t instr;
@@ -640,8 +719,6 @@ static int print_code(vcpu_guest_context_any_t *ctx, int vcpu)
     return 0;
 }
 
-#define BYTES_PER_LINE 16
-
 static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest_word_t stk_addr)
 {
     guest_word_t stk_addr_start = stack_pointer(ctx);
@@ -787,7 +864,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
 }
 #endif
 
-static void dump_ctx(int vcpu, guest_word_t stk_addr)
+static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
 {
     vcpu_guest_context_any_t ctx;
 
@@ -826,21 +903,29 @@ static void dump_ctx(int vcpu, guest_word_t stk_addr)
     }
 #endif
 
-    if (!stk_addr) {
-        print_ctx(&ctx);
-    }
+    if (mem_addr) {
+        print_mem(&ctx, vcpu, guest_word_size, mem_addr);
 #ifndef NO_TRANSLATION
-    if (!stk_addr) {
-        print_code(&ctx, vcpu);
-        if (is_kernel_text(instr_pointer(&ctx)))
+        if (stk_addr)
             print_stack(&ctx, vcpu, guest_word_size, stk_addr);
+#endif
     } else {
-        print_stack(&ctx, vcpu, guest_word_size, stk_addr);
-    }
+        if (!stk_addr) {
+            print_ctx(&ctx);
+        }
+#ifndef NO_TRANSLATION
+        if (!stk_addr) {
+            print_code(&ctx, vcpu);
+            if (is_kernel_text(instr_pointer(&ctx)))
+                print_stack(&ctx, vcpu, guest_word_size, stk_addr);
+        } else {
+            print_stack(&ctx, vcpu, guest_word_size, stk_addr);
+        }
 #endif
+    }
 }
 
-static void dump_all_vcpus(guest_word_t stk_addr)
+static void dump_all_vcpus(guest_word_t mem_addr, guest_word_t stk_addr)
 {
     xc_vcpuinfo_t vinfo;
     int vcpu;
@@ -849,7 +934,7 @@ static void dump_all_vcpus(guest_word_t stk_addr)
         if ( xc_vcpu_getinfo(xenctx.xc_handle, xenctx.domid, vcpu, &vinfo) )
             continue;
         if ( vinfo.online )
-            dump_ctx(vcpu, stk_addr);
+            dump_ctx(vcpu, mem_addr, stk_addr);
     }
 }
 
@@ -868,6 +953,8 @@ static void usage(void)
     printf("  -S, --stack-trace print a complete stack trace.\n");
     printf("  -k kaddr, --kernel-start=kaddr\n");
     printf("                    set user/kernel split at kaddr. (default 0xc0000000)\n");
+    printf("  -m maddr, --memory=maddr\n");
+    printf("                    dump memory at maddr.\n");
     printf("  -d daddr, --dump-as-stack=daddr\n");
     printf("                    dump memory as a stack at daddr.\n");
     printf("  -a, --all         display more registers\n");
@@ -879,13 +966,14 @@ int main(int argc, char **argv)
 {
     int ch;
     int ret;
-    static const char *sopts = "fs:hak:SCd:2";
+    static const char *sopts = "fs:hak:SCm:d:2";
     static const struct option lopts[] = {
         {"stack-trace", 0, NULL, 'S'},
         {"symbol-table", 1, NULL, 's'},
         {"frame-pointers", 0, NULL, 'f'},
         {"kernel-start", 1, NULL, 'k'},
         {"two-pages", 0, NULL, '2'},
+        {"memory", 1, NULL, 'm'},
         {"dump-as-stack", 1, NULL, 'd'},
         {"all", 0, NULL, 'a'},
         {"all-vcpus", 0, NULL, 'C'},
@@ -893,6 +981,7 @@ int main(int argc, char **argv)
         {0, 0, 0, 0}
     };
     const char *symbol_table = NULL;
+    guest_word_t mem_addr = 0;
     guest_word_t stk_addr = 0;
 
     int vcpu = 0;
@@ -920,6 +1009,9 @@ int main(int argc, char **argv)
         case 'k':
             kernel_start = strtoull(optarg, NULL, 0);
             break;
+        case 'm':
+            mem_addr = strtoull(optarg, NULL, 0);
+            break;
         case 'd':
             stk_addr = strtoull(optarg, NULL, 0);
             break;
@@ -952,7 +1044,7 @@ int main(int argc, char **argv)
         read_symbol_table(symbol_table);
 
     xenctx.xc_handle = xc_interface_open(0,0,0); /* for accessing control interface */
-    if (xenctx.xc_handle < 0) {
+    if (!xenctx.xc_handle) {
         perror("xc_interface_open");
         exit(-1);
     }
@@ -973,9 +1065,9 @@ int main(int argc, char **argv)
     }
 
     if (xenctx.all_vcpus)
-        dump_all_vcpus(stk_addr);
+        dump_all_vcpus(mem_addr, stk_addr);
     else
-        dump_ctx(vcpu, stk_addr);
+        dump_ctx(vcpu, mem_addr, stk_addr);
 
     if (xenctx.self_paused) {
         ret = xc_domain_unpause(xenctx.xc_handle, xenctx.domid);
-- 
1.7.1

Re: [PATCH v2 09/12] xenctx: Add -m <maddr> option to dump memory at maddr.

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> @@ -973,9 +1065,9 @@ int main(int argc, char **argv)
>      }
>  
>      if (xenctx.all_vcpus)
> -        dump_all_vcpus(stk_addr);
> +        dump_all_vcpus(mem_addr, stk_addr);
>      else
> -        dump_ctx(vcpu, stk_addr);
> +        dump_ctx(vcpu, mem_addr, stk_addr);

Same comment as on the previous patch - just dump the requested
memory here, without modifying other behavior.

Jan

Re: [PATCH v2 09/12] xenctx: Add -m <maddr> option to dump memory at maddr.

[Don Slutz]

On 11/07/13 03:25, Jan Beulich wrote:
>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>> @@ -973,9 +1065,9 @@ int main(int argc, char **argv)
>>       }
>>   
>>       if (xenctx.all_vcpus)
>> -        dump_all_vcpus(stk_addr);
>> +        dump_all_vcpus(mem_addr, stk_addr);
>>       else
>> -        dump_ctx(vcpu, stk_addr);
>> +        dump_ctx(vcpu, mem_addr, stk_addr);
> Same comment as on the previous patch - just dump the requested
> memory here, without modifying other behavior.
>
> Jan
>
Yes, will re-work to do the same as the previous patch's re-work will do.
    -Don

[PATCH v2 10/12] xenctx: change is_kernel_text() into is_kernel_addr().

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

A new enum has been added to allow the caller to determine if this
kernel address is a text on data address.  This is currenlty not
used, but will be in the next patch.

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |   70 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index d6f8482..6ec9c74 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -41,6 +41,15 @@ static struct xenctx {
     xc_dominfo_t dominfo;
 } xenctx;
 
+/* Note: the order of these matter.
+ * NOT_KERNEL_ADDR must be < both KERNEL_DATA_ADDR and KERNEL_TEXT_ADDR.
+ * KERNEL_DATA_ADDR must be < KERNEL_TEXT_ADDR. */
+typedef enum type_of_addr_ {
+    NOT_KERNEL_ADDR,
+    KERNEL_DATA_ADDR,
+    KERNEL_TEXT_ADDR,
+} type_of_addr;
+
 #if defined (__i386__) || defined (__x86_64__)
 typedef unsigned long long guest_word_t;
 #define FMT_32B_WORD "%08llx"
@@ -69,6 +78,7 @@ struct symbol {
 } *symbol_table = NULL;
 
 guest_word_t kernel_stext, kernel_etext, kernel_sinittext, kernel_einittext, kernel_hypercallpage;
+guest_word_t kernel_text, kernel_end;
 
 #if defined (__i386__)
 unsigned long long kernel_start = 0xc0000000;
@@ -76,22 +86,28 @@ unsigned long long kernel_start = 0xc0000000;
 unsigned long long kernel_start = 0xffffffff80000000UL;
 #endif
 
-static int is_kernel_text(guest_word_t addr)
+static type_of_addr is_kernel_addr(guest_word_t addr)
 {
-    if (symbol_table == NULL)
-        return (addr > kernel_start);
+    if (symbol_table == NULL) {
+        if (addr > kernel_start)
+            return KERNEL_TEXT_ADDR;
+        else
+            return NOT_KERNEL_ADDR;
+    }
 
     if (addr >= kernel_stext &&
         addr <= kernel_etext)
-        return 1;
-    if (kernel_hypercallpage &&
-        (addr >= kernel_hypercallpage &&
-         addr <= kernel_hypercallpage + 4096))
-        return 1;
+        return KERNEL_TEXT_ADDR;
+    if (kernel_hypercallpage && (addr >= kernel_hypercallpage &&
+                                 addr <= kernel_hypercallpage + 4096))
+        return KERNEL_TEXT_ADDR;
     if (addr >= kernel_sinittext &&
         addr <= kernel_einittext)
-        return 1;
-    return 0;
+        return KERNEL_TEXT_ADDR;
+    if (addr >= kernel_text &&
+        addr <= kernel_end)
+        return KERNEL_DATA_ADDR;
+    return NOT_KERNEL_ADDR;
 }
 
 #if 0
@@ -145,11 +161,11 @@ static struct symbol *lookup_symbol(guest_word_t address)
     return s->next && s->next->address <= address ? s->next : s;
 }
 
-static void print_symbol(guest_word_t addr)
+static void print_symbol(guest_word_t addr, type_of_addr type)
 {
     struct symbol *s;
 
-    if (!is_kernel_text(addr))
+    if (is_kernel_addr(addr) < type)
         return;
 
     s = lookup_symbol(addr);
@@ -217,6 +233,10 @@ static void read_symbol_table(const char *symtab)
             kernel_stext = symbol->address;
         else if (strcmp(symbol->name, "_etext") == 0)
             kernel_etext = symbol->address;
+        else if (strcmp(symbol->name, "_text") == 0)
+            kernel_text = symbol->address;
+        else if (strcmp(symbol->name, "_end") == 0)
+            kernel_end = symbol->address;
         else if (strcmp(symbol->name, "_sinittext") == 0)
             kernel_sinittext = symbol->address;
         else if (strcmp(symbol->name, "_einittext") == 0)
@@ -288,7 +308,7 @@ static void print_ctx_32(vcpu_guest_context_x86_32_t *ctx)
     struct cpu_user_regs_x86_32 *regs = &ctx->user_regs;
 
     printf("cs:eip: %04x:%08x", regs->cs, regs->eip);
-    print_symbol(regs->eip);
+    print_symbol(regs->eip, KERNEL_TEXT_ADDR);
     print_flags(regs->eflags);
     printf("ss:esp: %04x:%08x\n", regs->ss, regs->esp);
 
@@ -317,7 +337,7 @@ static void print_ctx_32on64(vcpu_guest_context_x86_64_t *ctx)
     struct cpu_user_regs_x86_64 *regs = &ctx->user_regs;
 
     printf("cs:eip: %04x:%08x", regs->cs, (uint32_t)regs->eip);
-    print_symbol((uint32_t)regs->eip);
+    print_symbol((uint32_t)regs->eip, KERNEL_TEXT_ADDR);
     print_flags((uint32_t)regs->eflags);
     printf("ss:esp: %04x:%08x\n", regs->ss, (uint32_t)regs->esp);
 
@@ -346,7 +366,7 @@ static void print_ctx_64(vcpu_guest_context_x86_64_t *ctx)
     struct cpu_user_regs_x86_64 *regs = &ctx->user_regs;
 
     printf("rip: %016"PRIx64, regs->rip);
-    print_symbol(regs->rip);
+    print_symbol(regs->rip, KERNEL_TEXT_ADDR);
     print_flags(regs->rflags);
     printf("rsp: %016"PRIx64"\n", regs->rsp);
 
@@ -444,7 +464,7 @@ static void print_ctx_32(vcpu_guest_context_t *ctx)
     vcpu_guest_core_regs_t *regs = &ctx->user_regs;
 
     printf("PC:       %08"PRIx32, regs->pc32);
-    print_symbol(regs->pc32);
+    print_symbol(regs->pc32, KERNEL_TEXT_ADDR);
     printf("\n");
     printf("CPSR:     %08"PRIx32"\n", regs->cpsr);
     printf("USR:               SP:%08"PRIx32" LR:%08"PRIx32"\n",
@@ -496,7 +516,7 @@ static void print_ctx_64(vcpu_guest_context_t *ctx)
     vcpu_guest_core_regs_t *regs = &ctx->user_regs;
 
     printf("PC:       %016"PRIx64, regs->pc64);
-    print_symbol(regs->pc64);
+    print_symbol(regs->pc64, KERNEL_TEXT_ADDR);
     printf("\n");
 
     printf("LR:       %016"PRIx64"zn", regs->x30);
@@ -749,7 +769,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
             void *p = map_page(ctx, vcpu, stack);
             if (!p)
                 return -1;
-            word = read_stack_word(p, width);
+            word = read_mem_word(ctx, vcpu, stack, width);
             ascii[j++] = word;
             printf(" ");
             print_stack_word(word, width);
@@ -787,7 +807,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
         printf("%c [<", xenctx.stack_trace ? '*' : ' ');
         print_stack_word(instr_pointer(ctx), width);
         printf(">]");
-        print_symbol(instr_pointer(ctx));
+        print_symbol(instr_pointer(ctx), KERNEL_TEXT_ADDR);
         printf(" <--\n");
     }
     if (xenctx.frame_ptrs) {
@@ -818,7 +838,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
             if (xenctx.stack_trace) {
                 print_stack_word(stack, width);
                 printf(": |-- ");
-                print_stack_word(read_stack_word(p, width), width);
+                print_stack_word(frame, width);
                 printf("\n");
             }
             stack += width;
@@ -832,7 +852,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
                 printf(": %c [<", xenctx.stack_trace ? '|' : ' ');
                 print_stack_word(word, width);
                 printf(">]");
-                print_symbol(word);
+                print_symbol(word, KERNEL_TEXT_ADDR);
                 printf("\n");
                 stack += width;
             }
@@ -843,13 +863,13 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
             void *p = map_page(ctx, vcpu, stack);
             if (!p)
                 return -1;
-            word = read_stack_word(p, width);
-            if (is_kernel_text(word)) {
+            word = read_mem_word(ctx, vcpu, stack, width);
+            if (is_kernel_addr(word) >= KERNEL_TEXT_ADDR) {
                 print_stack_word(stack, width);
                 printf(":  [<");
                 print_stack_word(word, width);
                 printf(">]");
-                print_symbol(word);
+                print_symbol(word, KERNEL_TEXT_ADDR);
                 printf("\n");
             } else if (xenctx.stack_trace) {
                 print_stack_word(stack, width);
@@ -916,7 +936,7 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
 #ifndef NO_TRANSLATION
         if (!stk_addr) {
             print_code(&ctx, vcpu);
-            if (is_kernel_text(instr_pointer(&ctx)))
+            if (is_kernel_addr(instr_pointer(&ctx)) >= KERNEL_TEXT_ADDR)
                 print_stack(&ctx, vcpu, guest_word_size, stk_addr);
         } else {
             print_stack(&ctx, vcpu, guest_word_size, stk_addr);
-- 
1.7.1

Re: [PATCH v2 10/12] xenctx: change is_kernel_text() into is_kernel_addr().

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> @@ -76,22 +86,28 @@ unsigned long long kernel_start = 0xc0000000;
>  unsigned long long kernel_start = 0xffffffff80000000UL;
>  #endif
>  
> -static int is_kernel_text(guest_word_t addr)
> +static type_of_addr is_kernel_addr(guest_word_t addr)

The "is_" prefix is now bogus.

>  {
> -    if (symbol_table == NULL)
> -        return (addr > kernel_start);
> +    if (symbol_table == NULL) {
> +        if (addr > kernel_start)
> +            return KERNEL_TEXT_ADDR;
> +        else
> +            return NOT_KERNEL_ADDR;
> +    }
>  
>      if (addr >= kernel_stext &&
>          addr <= kernel_etext)
> -        return 1;
> -    if (kernel_hypercallpage &&
> -        (addr >= kernel_hypercallpage &&
> -         addr <= kernel_hypercallpage + 4096))
> -        return 1;
> +        return KERNEL_TEXT_ADDR;
> +    if (kernel_hypercallpage && (addr >= kernel_hypercallpage &&
> +                                 addr <= kernel_hypercallpage + 4096))

This reformatting is pointlessly bloating the patch - if you want
the line reformatted, you should (a) do this in the patch that
adds the first part of the condition and (b) obey to indentation
rules.

> +        return KERNEL_TEXT_ADDR;
>      if (addr >= kernel_sinittext &&
>          addr <= kernel_einittext)
> -        return 1;
> -    return 0;
> +        return KERNEL_TEXT_ADDR;
> +    if (addr >= kernel_text &&
> +        addr <= kernel_end)
> +        return KERNEL_DATA_ADDR;

As you supposedly filtered out all text ranges before, did you really
mean to compare to kernel_text here (rather than kernel_start)?

> @@ -749,7 +769,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
>              void *p = map_page(ctx, vcpu, stack);
>              if (!p)
>                  return -1;
> -            word = read_stack_word(p, width);
> +            word = read_mem_word(ctx, vcpu, stack, width);

How is this change related to the purpose of the patch?

> @@ -818,7 +838,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
>              if (xenctx.stack_trace) {
>                  print_stack_word(stack, width);
>                  printf(": |-- ");
> -                print_stack_word(read_stack_word(p, width), width);
> +                print_stack_word(frame, width);

And this one?

> @@ -843,13 +863,13 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
>              void *p = map_page(ctx, vcpu, stack);
>              if (!p)
>                  return -1;
> -            word = read_stack_word(p, width);
> -            if (is_kernel_text(word)) {
> +            word = read_mem_word(ctx, vcpu, stack, width);
> +            if (is_kernel_addr(word) >= KERNEL_TEXT_ADDR) {

And one more.

Jan

Re: [PATCH v2 10/12] xenctx: change is_kernel_text() into is_kernel_addr().

[Don Slutz]

[-- Attachment #1.1: Type: text/plain, Size: 4255 bytes --]

On 11/07/13 03:35, Jan Beulich wrote:
>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>> @@ -76,22 +86,28 @@ unsigned long long kernel_start = 0xc0000000;
>>   unsigned long long kernel_start = 0xffffffff80000000UL;
>>   #endif
>>   
>> -static int is_kernel_text(guest_word_t addr)
>> +static type_of_addr is_kernel_addr(guest_word_t addr)
> The "is_" prefix is now bogus.
Ok, will remove.
>
>>   {
>> -    if (symbol_table == NULL)
>> -        return (addr > kernel_start);
>> +    if (symbol_table == NULL) {
>> +        if (addr > kernel_start)
>> +            return KERNEL_TEXT_ADDR;
>> +        else
>> +            return NOT_KERNEL_ADDR;
>> +    }
>>   
>>       if (addr >= kernel_stext &&
>>           addr <= kernel_etext)
>> -        return 1;
>> -    if (kernel_hypercallpage &&
>> -        (addr >= kernel_hypercallpage &&
>> -         addr <= kernel_hypercallpage + 4096))
>> -        return 1;
>> +        return KERNEL_TEXT_ADDR;
>> +    if (kernel_hypercallpage && (addr >= kernel_hypercallpage &&
>> +                                 addr <= kernel_hypercallpage + 4096))
> This reformatting is pointlessly bloating the patch - if you want
> the line reformatted, you should (a) do this in the patch that
> adds the first part of the condition and (b) obey to indentation
> rules.
Opps, I did not what this reformatting, will just drop it.
>> +        return KERNEL_TEXT_ADDR;
>>       if (addr >= kernel_sinittext &&
>>           addr <= kernel_einittext)
>> -        return 1;
>> -    return 0;
>> +        return KERNEL_TEXT_ADDR;
>> +    if (addr >= kernel_text &&
>> +        addr <= kernel_end)
>> +        return KERNEL_DATA_ADDR;
> As you supposedly filtered out all text ranges before, did you really
> mean to compare to kernel_text here (rather than kernel_start)?
Yes, I did.  I think it is better to use the value that is in the system map over the default.  It has changed:

dcs-xen-54:~/xen>grep " _text" /boot/System.map-*
/boot/System.map-2.6.18-128.el5:ffffffff80000000 A _text
/boot/System.map-3.6.11-5.fc17.x86_64:ffffffff81000000 T _text
/boot/System.map-3.8.11-100.fc17.x86_64:ffffffff81000000 T _text

But since it is a command line argument, if specified, should it be used instead?
>> @@ -749,7 +769,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
>>               void *p = map_page(ctx, vcpu, stack);
>>               if (!p)
>>                   return -1;
>> -            word = read_stack_word(p, width);
>> +            word = read_mem_word(ctx, vcpu, stack, width);
> How is this change related to the purpose of the patch?
Opps, wrong split.  This (and the one 2 below changing to read_mem_word) should be in their own patch (or back in patch 8, with patch 9 before 8.  Without this change specifying an unaligned address can read across a page boundary which would not get he correct data (and can fault).
>> @@ -818,7 +838,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
>>               if (xenctx.stack_trace) {
>>                   print_stack_word(stack, width);
>>                   printf(": |-- ");
>> -                print_stack_word(read_stack_word(p, width), width);
>> +                print_stack_word(frame, width);
> And this one?
>
This cleanup mostly likely should be just dropped (or made it's own patch).  The context is just one line short:

                 frame = read_stack_word(p, width);
                 if (xenctx.stack_trace) {
    ...

                 print_stack_word(read_stack_word(p, width), width);

And since read_stack_word(p, width) should always return the same data for the same address; the 2nd call is not needed.


>> @@ -843,13 +863,13 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
>>               void *p = map_page(ctx, vcpu, stack);
>>               if (!p)
>>                   return -1;
>> -            word = read_stack_word(p, width);
>> -            if (is_kernel_text(word)) {
>> +            word = read_mem_word(ctx, vcpu, stack, width);
>> +            if (is_kernel_addr(word) >= KERNEL_TEXT_ADDR) {
> And one more.
The 2nd line is needed is_kernel_text() changed to is_kernel_addr().
    -Don Slutz
> Jan
>


[-- Attachment #1.2: Type: text/html, Size: 6668 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 10/12] xenctx: change is_kernel_text() into is_kernel_addr().

[Jan Beulich]

>>> On 08.11.13 at 01:51, Don Slutz <dslutz@verizon.com> wrote:
> On 11/07/13 03:35, Jan Beulich wrote:
>>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>>> +        return KERNEL_TEXT_ADDR;
>>>       if (addr >= kernel_sinittext &&
>>>           addr <= kernel_einittext)
>>> -        return 1;
>>> -    return 0;
>>> +        return KERNEL_TEXT_ADDR;
>>> +    if (addr >= kernel_text &&
>>> +        addr <= kernel_end)
>>> +        return KERNEL_DATA_ADDR;
>> As you supposedly filtered out all text ranges before, did you really
>> mean to compare to kernel_text here (rather than kernel_start)?
> Yes, I did.  I think it is better to use the value that is in the system map 
> over the default.  It has changed:
> 
> dcs-xen-54:~/xen>grep " _text" /boot/System.map-*
> /boot/System.map-2.6.18-128.el5:ffffffff80000000 A _text
> /boot/System.map-3.6.11-5.fc17.x86_64:ffffffff81000000 T _text
> /boot/System.map-3.8.11-100.fc17.x86_64:ffffffff81000000 T _text
> 
> But since it is a command line argument, if specified, should it be used 
> instead?

I guess so. In any event - after having split out all text pieces
(hopefully), the check here should cover the whole kernel image,
no matter how the ordering between text and data is.

Jan

Re: [PATCH v2 10/12] xenctx: change is_kernel_text() into is_kernel_addr().

[Ian Campbell]

On Fri, 2013-11-08 at 08:40 +0000, Jan Beulich wrote:
> >>> On 08.11.13 at 01:51, Don Slutz <dslutz@verizon.com> wrote:
> > On 11/07/13 03:35, Jan Beulich wrote:
> >>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> >>> +        return KERNEL_TEXT_ADDR;
> >>>       if (addr >= kernel_sinittext &&
> >>>           addr <= kernel_einittext)
> >>> -        return 1;
> >>> -    return 0;
> >>> +        return KERNEL_TEXT_ADDR;
> >>> +    if (addr >= kernel_text &&
> >>> +        addr <= kernel_end)
> >>> +        return KERNEL_DATA_ADDR;
> >> As you supposedly filtered out all text ranges before, did you really
> >> mean to compare to kernel_text here (rather than kernel_start)?
> > Yes, I did.  I think it is better to use the value that is in the system map 
> > over the default.  It has changed:
> > 
> > dcs-xen-54:~/xen>grep " _text" /boot/System.map-*
> > /boot/System.map-2.6.18-128.el5:ffffffff80000000 A _text
> > /boot/System.map-3.6.11-5.fc17.x86_64:ffffffff81000000 T _text
> > /boot/System.map-3.8.11-100.fc17.x86_64:ffffffff81000000 T _text
> > 
> > But since it is a command line argument, if specified, should it be used 
> > instead?
> 
> I guess so.

Yes, since the user may not have a symbol table for the given kernel and
it may not even be Linux. I'm sure a Windows kernel guru debugging a
Windows guest would know the right number for that version of Windows.
(IIRC the option was added by just such a guru...)

>  In any event - after having split out all text pieces
> (hopefully), the check here should cover the whole kernel image,
> no matter how the ordering between text and data is.
> 
> Jan
> 

Re: [PATCH v2 10/12] xenctx: change is_kernel_text() into is_kernel_addr().

[Don Slutz]

On 11/08/13 04:50, Ian Campbell wrote:
> On Fri, 2013-11-08 at 08:40 +0000, Jan Beulich wrote:
>>>>> On 08.11.13 at 01:51, Don Slutz <dslutz@verizon.com> wrote:
>>> On 11/07/13 03:35, Jan Beulich wrote:
>>>>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>>>>> +        return KERNEL_TEXT_ADDR;
>>>>>        if (addr >= kernel_sinittext &&
>>>>>            addr <= kernel_einittext)
>>>>> -        return 1;
>>>>> -    return 0;
>>>>> +        return KERNEL_TEXT_ADDR;
>>>>> +    if (addr >= kernel_text &&
>>>>> +        addr <= kernel_end)
>>>>> +        return KERNEL_DATA_ADDR;
>>>> As you supposedly filtered out all text ranges before, did you really
>>>> mean to compare to kernel_text here (rather than kernel_start)?
>>> Yes, I did.  I think it is better to use the value that is in the system map
>>> over the default.  It has changed:
>>>
>>> dcs-xen-54:~/xen>grep " _text" /boot/System.map-*
>>> /boot/System.map-2.6.18-128.el5:ffffffff80000000 A _text
>>> /boot/System.map-3.6.11-5.fc17.x86_64:ffffffff81000000 T _text
>>> /boot/System.map-3.8.11-100.fc17.x86_64:ffffffff81000000 T _text
>>>
>>> But since it is a command line argument, if specified, should it be used
>>> instead?
>> I guess so.
> Yes, since the user may not have a symbol table for the given kernel and
> it may not even be Linux. I'm sure a Windows kernel guru debugging a
> Windows guest would know the right number for that version of Windows.
> (IIRC the option was added by just such a guru...)
If you do not specify a symbol file, kernel_start will be used. This is 
the case when both are specified.  If I am reading this right that is 2 
votes to use kernel_start if specified otherwise kernel_text.
    -Don Slutz
>>   In any event - after having split out all text pieces
>> (hopefully), the check here should cover the whole kernel image,
>> no matter how the ordering between text and data is.
>>
>> Jan
>>
>

[PATCH v2 11/12] xenctx: Dump registers via hvm info if available.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

This allows the output of registers that are not available in the
not HVM view.

Look up symbols for various registers as kernel data.

Also a minor check that ip, sp, and cr3 are the same.

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |  284 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 282 insertions(+), 2 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 6ec9c74..d3cbb22 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -415,6 +415,247 @@ static void print_ctx(vcpu_guest_context_any_t *ctx)
         print_ctx_64(&ctx->x64);
 }
 
+#if defined(__i386__) || defined(__x86_64__)
+static void print_cpuctx_regs64(struct hvm_hw_cpu *ctx)
+{
+    printf("rip: %016"PRIx64, ctx->rip);
+    print_symbol(ctx->rip, KERNEL_TEXT_ADDR);
+    print_flags(ctx->rflags);
+    printf("rsp: %016"PRIx64"\n", ctx->rsp);
+
+    printf("rax: %016"PRIx64"\t", ctx->rax);
+    printf("rcx: %016"PRIx64"\t", ctx->rcx);
+    printf("rdx: %016"PRIx64"\n", ctx->rdx);
+
+    printf("rbx: %016"PRIx64"\t", ctx->rbx);
+    printf("rsi: %016"PRIx64"\t", ctx->rsi);
+    printf("rdi: %016"PRIx64"\n", ctx->rdi);
+
+    printf("rbp: %016"PRIx64"\t", ctx->rbp);
+    printf(" r8: %016"PRIx64"\t", ctx->r8);
+    printf(" r9: %016"PRIx64"\n", ctx->r9);
+
+    printf("r10: %016"PRIx64"\t", ctx->r10);
+    printf("r11: %016"PRIx64"\t", ctx->r11);
+    printf("r12: %016"PRIx64"\n", ctx->r12);
+
+    printf("r13: %016"PRIx64"\t", ctx->r13);
+    printf("r14: %016"PRIx64"\t", ctx->r14);
+    printf("r15: %016"PRIx64"\n", ctx->r15);
+
+    printf("  cs: %04x @ %016"PRIx64, ctx->cs_sel, ctx->cs_base);
+    if (ctx->cs_base)
+        print_symbol(ctx->cs_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->cs_limit, ctx->cs_arbytes);
+    printf("  ss: %04x @ %016"PRIx64, ctx->ss_sel, ctx->ss_base);
+    if (ctx->ss_base)
+        print_symbol(ctx->ss_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->ss_limit, ctx->ss_arbytes);
+    printf("  ds: %04x @ %016"PRIx64, ctx->ds_sel, ctx->ds_base);
+    if (ctx->ds_base)
+        print_symbol(ctx->ds_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->ds_limit, ctx->ds_arbytes);
+    printf("  es: %04x @ %016"PRIx64, ctx->es_sel, ctx->es_base);
+    if (ctx->es_base)
+        print_symbol(ctx->es_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->es_limit, ctx->es_arbytes);
+    printf("  fs: %04x @ %016"PRIx64, ctx->fs_sel, ctx->fs_base);
+    if (ctx->fs_base)
+        print_symbol(ctx->fs_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->fs_limit, ctx->fs_arbytes);
+    printf("  gs: %04x @ %016"PRIx64"\\%016"PRIx64, ctx->gs_sel,
+           ctx->gs_base, ctx->shadow_gs);
+    if (ctx->gs_base)
+        print_symbol(ctx->gs_base, KERNEL_DATA_ADDR);
+    printf("\\");
+    if (ctx->shadow_gs)
+        print_symbol(ctx->shadow_gs, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->gs_limit, ctx->gs_arbytes);
+
+    if (xenctx.disp_all) {
+        printf("  tr: %04x @ %016"PRIx64, ctx->tr_sel, ctx->tr_base);
+        if (ctx->tr_base)
+            print_symbol(ctx->tr_base, KERNEL_DATA_ADDR);
+        printf("\n           /%08x(%x)\n", ctx->tr_limit, ctx->tr_arbytes);
+        printf(" ldt: %04x @ %016"PRIx64, ctx->ldtr_sel, ctx->ldtr_base);
+        if (ctx->ldtr_base)
+            print_symbol(ctx->ldtr_base, KERNEL_DATA_ADDR);
+        printf("\n           /%08x(%x)\n", ctx->ldtr_limit, ctx->ldtr_arbytes);
+        printf("\n");
+        printf("cr0: %08"PRIx64"\n", ctx->cr0);
+        printf("cr2: %08"PRIx64, ctx->cr2);
+        print_symbol(ctx->cr2, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("cr3: %08"PRIx64"\n", ctx->cr3);
+        printf("cr4: %08"PRIx64"\n", ctx->cr4);
+        printf("\n");
+        printf("dr0: %08"PRIx64, ctx->dr0);
+        print_symbol(ctx->dr0, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("dr1: %08"PRIx64, ctx->dr1);
+        print_symbol(ctx->dr1, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("dr2: %08"PRIx64, ctx->dr2);
+        print_symbol(ctx->dr2, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("dr3: %08"PRIx64, ctx->dr3);
+        print_symbol(ctx->dr3, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("dr6: %08"PRIx64"\n", ctx->dr6);
+        printf("dr7: %08"PRIx64"\n", ctx->dr7);
+        printf("\n");
+        printf("gdt: %016"PRIx64"/%lx", ctx->gdtr_base, (long)(ctx->gdtr_limit));
+        print_symbol(ctx->gdtr_base, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("idt: %016"PRIx64"/%lx", ctx->idtr_base, (long)(ctx->idtr_limit));
+        print_symbol(ctx->idtr_base, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("\n");
+        printf(" EFER: %08"PRIx64" flags: %08"PRIx64"\n",
+               ctx->msr_efer, ctx->msr_flags);
+        printf("SYSENTER cs: %08"PRIx64" esp: %08"PRIx64" eip: %08"PRIx64,
+               ctx->sysenter_cs, ctx->sysenter_esp, ctx->sysenter_eip);
+        print_symbol(ctx->sysenter_eip, KERNEL_TEXT_ADDR);
+        printf("\n");
+        printf(" STAR: %08"PRIx64" SFMASK: %08"PRIx64"\n",
+               ctx->msr_star, ctx->msr_syscall_mask);
+        printf("LSTAR: %08"PRIx64, ctx->msr_lstar);
+        print_symbol(ctx->msr_lstar, KERNEL_TEXT_ADDR);
+        printf("\n");
+        printf("CSTAR: %08"PRIx64, ctx->msr_cstar);
+        print_symbol(ctx->msr_cstar, KERNEL_TEXT_ADDR);
+        printf("\n");
+        printf("\n");
+        printf("\n");
+        printf(" tsc: %016"PRIx64"\n", ctx->tsc);
+        printf("\n");
+        printf("\n");
+        printf("pending_event: %lx  pending_vector: %lx\n",
+               (long)(ctx->pending_event), (long)(ctx->pending_vector));
+        printf("pending error_code: %lx\n\n", (long)(ctx->error_code));
+    }
+}
+
+static void print_cpuctx_regs32(struct hvm_hw_cpu *ctx)
+{
+    printf("cs:eip: %04x:%08x", ctx->cs_sel, (uint32_t)ctx->rip);
+    print_symbol((uint32_t)ctx->rip, KERNEL_TEXT_ADDR);
+    print_flags((uint32_t)ctx->rflags);
+    printf("ss:esp: %04x:%08x\n", ctx->ss_sel, (uint32_t)ctx->rsp);
+    printf("\n");
+
+    printf("eax: %08x\t", (uint32_t)ctx->rax);
+    printf("ebx: %08x\t", (uint32_t)ctx->rbx);
+    printf("ecx: %08x\t", (uint32_t)ctx->rcx);
+    printf("edx: %08x\n", (uint32_t)ctx->rdx);
+
+    printf("esi: %08x\t", (uint32_t)ctx->rsi);
+    printf("edi: %08x\t", (uint32_t)ctx->rdi);
+    printf("ebp: %08x\n", (uint32_t)ctx->rbp);
+    printf("\n");
+
+    printf("  cs: %04x @ %08"PRIx64, ctx->cs_sel, ctx->cs_base);
+    if (ctx->cs_base)
+        print_symbol(ctx->cs_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->cs_limit, ctx->cs_arbytes);
+    printf("  ss: %04x @ %08"PRIx64, ctx->ss_sel, ctx->ss_base);
+    if (ctx->ss_base)
+        print_symbol(ctx->ss_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->ss_limit, ctx->ss_arbytes);
+    printf("  ds: %04x @ %08"PRIx64, ctx->ds_sel, ctx->ds_base);
+    if (ctx->ds_base)
+        print_symbol(ctx->ds_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->ds_limit, ctx->ds_arbytes);
+    printf("  es: %04x @ %08"PRIx64, ctx->es_sel, ctx->es_base);
+    if (ctx->es_base)
+        print_symbol(ctx->es_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->es_limit, ctx->es_arbytes);
+    printf("  fs: %04x @ %08"PRIx64, ctx->fs_sel, ctx->fs_base);
+    if (ctx->fs_base)
+        print_symbol(ctx->fs_base, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->fs_limit, ctx->fs_arbytes);
+    printf("  gs: %04x @ %08"PRIx64"\\%08"PRIx64, ctx->gs_sel,
+           ctx->gs_base, ctx->shadow_gs);
+    if (ctx->gs_base)
+        print_symbol(ctx->gs_base, KERNEL_DATA_ADDR);
+    printf("\\");
+    if (ctx->shadow_gs)
+        print_symbol(ctx->shadow_gs, KERNEL_DATA_ADDR);
+    printf("\n           /%08x(%x)\n", ctx->gs_limit, ctx->gs_arbytes);
+
+    if (xenctx.disp_all) {
+        printf("  tr: %04x @ %08"PRIx64, ctx->tr_sel, ctx->tr_base);
+        if (ctx->tr_base)
+            print_symbol(ctx->tr_base, KERNEL_DATA_ADDR);
+        printf("\n           /%08x(%x)\n", ctx->tr_limit, ctx->tr_arbytes);
+        printf(" ldt: %04x @ %08"PRIx64, ctx->ldtr_sel, ctx->ldtr_base);
+        if (ctx->ldtr_base)
+            print_symbol(ctx->ldtr_base, KERNEL_DATA_ADDR);
+        printf("\n           /%08x(%x)\n", ctx->ldtr_limit, ctx->ldtr_arbytes);
+        printf("\n");
+        printf("cr0: %08"PRIx64"\n", ctx->cr0);
+        printf("cr2: %08"PRIx64, ctx->cr2);
+        print_symbol(ctx->cr2, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("cr3: %08"PRIx64"\n", ctx->cr3);
+        printf("cr4: %08"PRIx64"\n", ctx->cr4);
+        printf("\n");
+        printf("dr0: %08"PRIx64, ctx->dr0);
+        print_symbol(ctx->dr0, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("dr1: %08"PRIx64, ctx->dr1);
+        print_symbol(ctx->dr1, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("dr2: %08"PRIx64, ctx->dr2);
+        print_symbol(ctx->dr2, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("dr3: %08"PRIx64, ctx->dr3);
+        print_symbol(ctx->dr3, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("dr6: %08"PRIx64"\n", ctx->dr6);
+        printf("dr7: %08"PRIx64"\n", ctx->dr7);
+        printf("\n");
+        printf("gdt: %08"PRIx64"/%lx", ctx->gdtr_base, (long)(ctx->gdtr_limit));
+        print_symbol(ctx->gdtr_base, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("idt: %08"PRIx64"/%lx", ctx->idtr_base, (long)(ctx->idtr_limit));
+        print_symbol(ctx->idtr_base, KERNEL_DATA_ADDR);
+        printf("\n");
+        printf("\n");
+        printf(" EFER: %08"PRIx64" flags: %08"PRIx64"\n",
+               ctx->msr_efer, ctx->msr_flags);
+        printf("SYSENTER cs: %08"PRIx64" esp: %08"PRIx64" eip: %08"PRIx64,
+               ctx->sysenter_cs, ctx->sysenter_esp, ctx->sysenter_eip);
+        print_symbol(ctx->sysenter_eip, KERNEL_TEXT_ADDR);
+        printf("\n");
+        printf(" STAR: %08"PRIx64" SFMASK: %08"PRIx64"\n",
+               ctx->msr_star, ctx->msr_syscall_mask);
+        printf("LSTAR: %08"PRIx64, ctx->msr_lstar);
+        print_symbol(ctx->msr_lstar, KERNEL_TEXT_ADDR);
+        printf("\n");
+        printf("CSTAR: %08"PRIx64, ctx->msr_cstar);
+        print_symbol(ctx->msr_cstar, KERNEL_TEXT_ADDR);
+        printf("\n");
+        printf("\n");
+        printf(" tsc: %016"PRIx64"\n", ctx->tsc);
+        printf("\n");
+        printf("\n");
+        printf("pending_event: %lx  pending_vector: %lx\n",
+               (long)(ctx->pending_event), (long)(ctx->pending_vector));
+        printf("pending error_code: %lx\n\n", (long)(ctx->error_code));
+    }
+}
+
+static void print_cpuctx(struct hvm_hw_cpu *ctx)
+{
+    if (guest_word_size == 8)
+        print_cpuctx_regs64(ctx);
+    else
+        print_cpuctx_regs32(ctx);
+
+}
+#endif
+
 #define NONPROT_MODE_SEGMENT_SHIFT 4
 
 static guest_word_t instr_pointer(vcpu_guest_context_any_t *ctx)
@@ -887,6 +1128,9 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
 static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
 {
     vcpu_guest_context_any_t ctx;
+#if defined(__i386__) || defined(__x86_64__)
+    struct hvm_hw_cpu cpuctx;
+#endif
 
     if (xc_vcpu_getcontext(xenctx.xc_handle, xenctx.domid, vcpu, &ctx) < 0) {
         perror("xc_vcpu_getcontext");
@@ -896,7 +1140,6 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
 #if defined(__i386__) || defined(__x86_64__)
     {
         if (xenctx.dominfo.hvm) {
-            struct hvm_hw_cpu cpuctx;
             xen_capabilities_info_t xen_caps = "";
             if (xc_domain_hvm_getcontext_partial(
                     xenctx.xc_handle, xenctx.domid, HVM_SAVE_CODE(CPU),
@@ -931,7 +1174,44 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
 #endif
     } else {
         if (!stk_addr) {
-            print_ctx(&ctx);
+#if defined(__i386__) || defined(__x86_64__)
+            if (xenctx.dominfo.hvm && ctxt_word_size == 8) {
+                if (guest_word_size == 4) {
+                    if ((((uint32_t)ctx.x64.user_regs.eip) != cpuctx.rip) ||
+                        (((uint32_t)ctx.x64.user_regs.esp) != cpuctx.rsp) ||
+                        (((uint32_t)ctx.x64.ctrlreg[3]) != cpuctx.cr3)) {
+                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
+                                (long long)((uint32_t)ctx.x64.user_regs.eip),
+                                (long long)cpuctx.rip,
+                                (long long)((uint32_t)ctx.x64.user_regs.esp),
+                                (long long)cpuctx.rsp,
+                                (long long)((uint32_t)ctx.x64.ctrlreg[3]),
+                                (long long)cpuctx.cr3);
+                        fprintf(stdout, "=============Regs mismatch start=============\n");
+                        print_ctx(&ctx);
+                        fprintf(stdout, "=============Regs mismatch end=============\n");
+                    }
+                } else {
+                    if ((ctx.x64.user_regs.eip != cpuctx.rip) ||
+                        (ctx.x64.user_regs.esp != cpuctx.rsp) ||
+                        (ctx.x64.ctrlreg[3] != cpuctx.cr3)) {
+                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
+                                (long long)ctx.x64.user_regs.eip,
+                                (long long)cpuctx.rip,
+                                (long long)ctx.x64.user_regs.esp,
+                                (long long)cpuctx.rsp,
+                                (long long)ctx.x64.ctrlreg[3],
+                                (long long)cpuctx.cr3);
+                        fprintf(stdout, "=============Regs mismatch start=============\n");
+                        print_ctx(&ctx);
+                        fprintf(stdout, "=============Regs mismatch end=============\n");
+                    }
+                }
+                print_cpuctx(&cpuctx);
+            }
+            else
+#endif
+                print_ctx(&ctx);
         }
 #ifndef NO_TRANSLATION
         if (!stk_addr) {
-- 
1.7.1

Re: [PATCH v2 11/12] xenctx: Dump registers via hvm info if available.

[Andrew Cooper]

On 06/11/13 20:08, Don Slutz wrote:
> From: Don Slutz <Don@CloudSwitch.com>
>
> This allows the output of registers that are not available in the
> not HVM view.
>
> Look up symbols for various registers as kernel data.
>
> Also a minor check that ip, sp, and cr3 are the same.
>
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>

What about tools/misc/xen-hvmctx ?

This is a particularly awkward tool where we have two tools in different
subtrees which do about 80% the same thing.

I would certainly not object if someone were to merge these two tools
into one.  It is particularly awkward as xen-hvmctx ends up being
installed into /usr/sbin/ and is therefore on the $PATH, while xenctx
ends up off the $PATH in /usr/lib/xen/bin/

I am also fairly sure we have other examples of tools like this with
sightly different variants living in different locations.

~Andrew

> ---
>  tools/xentrace/xenctx.c |  284 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 282 insertions(+), 2 deletions(-)
>
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index 6ec9c74..d3cbb22 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -415,6 +415,247 @@ static void print_ctx(vcpu_guest_context_any_t *ctx)
>          print_ctx_64(&ctx->x64);
>  }
>  
> +#if defined(__i386__) || defined(__x86_64__)
> +static void print_cpuctx_regs64(struct hvm_hw_cpu *ctx)
> +{
> +    printf("rip: %016"PRIx64, ctx->rip);
> +    print_symbol(ctx->rip, KERNEL_TEXT_ADDR);
> +    print_flags(ctx->rflags);
> +    printf("rsp: %016"PRIx64"\n", ctx->rsp);
> +
> +    printf("rax: %016"PRIx64"\t", ctx->rax);
> +    printf("rcx: %016"PRIx64"\t", ctx->rcx);
> +    printf("rdx: %016"PRIx64"\n", ctx->rdx);
> +
> +    printf("rbx: %016"PRIx64"\t", ctx->rbx);
> +    printf("rsi: %016"PRIx64"\t", ctx->rsi);
> +    printf("rdi: %016"PRIx64"\n", ctx->rdi);
> +
> +    printf("rbp: %016"PRIx64"\t", ctx->rbp);
> +    printf(" r8: %016"PRIx64"\t", ctx->r8);
> +    printf(" r9: %016"PRIx64"\n", ctx->r9);
> +
> +    printf("r10: %016"PRIx64"\t", ctx->r10);
> +    printf("r11: %016"PRIx64"\t", ctx->r11);
> +    printf("r12: %016"PRIx64"\n", ctx->r12);
> +
> +    printf("r13: %016"PRIx64"\t", ctx->r13);
> +    printf("r14: %016"PRIx64"\t", ctx->r14);
> +    printf("r15: %016"PRIx64"\n", ctx->r15);
> +
> +    printf("  cs: %04x @ %016"PRIx64, ctx->cs_sel, ctx->cs_base);
> +    if (ctx->cs_base)
> +        print_symbol(ctx->cs_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->cs_limit, ctx->cs_arbytes);
> +    printf("  ss: %04x @ %016"PRIx64, ctx->ss_sel, ctx->ss_base);
> +    if (ctx->ss_base)
> +        print_symbol(ctx->ss_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->ss_limit, ctx->ss_arbytes);
> +    printf("  ds: %04x @ %016"PRIx64, ctx->ds_sel, ctx->ds_base);
> +    if (ctx->ds_base)
> +        print_symbol(ctx->ds_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->ds_limit, ctx->ds_arbytes);
> +    printf("  es: %04x @ %016"PRIx64, ctx->es_sel, ctx->es_base);
> +    if (ctx->es_base)
> +        print_symbol(ctx->es_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->es_limit, ctx->es_arbytes);
> +    printf("  fs: %04x @ %016"PRIx64, ctx->fs_sel, ctx->fs_base);
> +    if (ctx->fs_base)
> +        print_symbol(ctx->fs_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->fs_limit, ctx->fs_arbytes);
> +    printf("  gs: %04x @ %016"PRIx64"\\%016"PRIx64, ctx->gs_sel,
> +           ctx->gs_base, ctx->shadow_gs);
> +    if (ctx->gs_base)
> +        print_symbol(ctx->gs_base, KERNEL_DATA_ADDR);
> +    printf("\\");
> +    if (ctx->shadow_gs)
> +        print_symbol(ctx->shadow_gs, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->gs_limit, ctx->gs_arbytes);
> +
> +    if (xenctx.disp_all) {
> +        printf("  tr: %04x @ %016"PRIx64, ctx->tr_sel, ctx->tr_base);
> +        if (ctx->tr_base)
> +            print_symbol(ctx->tr_base, KERNEL_DATA_ADDR);
> +        printf("\n           /%08x(%x)\n", ctx->tr_limit, ctx->tr_arbytes);
> +        printf(" ldt: %04x @ %016"PRIx64, ctx->ldtr_sel, ctx->ldtr_base);
> +        if (ctx->ldtr_base)
> +            print_symbol(ctx->ldtr_base, KERNEL_DATA_ADDR);
> +        printf("\n           /%08x(%x)\n", ctx->ldtr_limit, ctx->ldtr_arbytes);
> +        printf("\n");
> +        printf("cr0: %08"PRIx64"\n", ctx->cr0);
> +        printf("cr2: %08"PRIx64, ctx->cr2);
> +        print_symbol(ctx->cr2, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("cr3: %08"PRIx64"\n", ctx->cr3);
> +        printf("cr4: %08"PRIx64"\n", ctx->cr4);
> +        printf("\n");
> +        printf("dr0: %08"PRIx64, ctx->dr0);
> +        print_symbol(ctx->dr0, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("dr1: %08"PRIx64, ctx->dr1);
> +        print_symbol(ctx->dr1, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("dr2: %08"PRIx64, ctx->dr2);
> +        print_symbol(ctx->dr2, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("dr3: %08"PRIx64, ctx->dr3);
> +        print_symbol(ctx->dr3, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("dr6: %08"PRIx64"\n", ctx->dr6);
> +        printf("dr7: %08"PRIx64"\n", ctx->dr7);
> +        printf("\n");
> +        printf("gdt: %016"PRIx64"/%lx", ctx->gdtr_base, (long)(ctx->gdtr_limit));
> +        print_symbol(ctx->gdtr_base, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("idt: %016"PRIx64"/%lx", ctx->idtr_base, (long)(ctx->idtr_limit));
> +        print_symbol(ctx->idtr_base, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("\n");
> +        printf(" EFER: %08"PRIx64" flags: %08"PRIx64"\n",
> +               ctx->msr_efer, ctx->msr_flags);
> +        printf("SYSENTER cs: %08"PRIx64" esp: %08"PRIx64" eip: %08"PRIx64,
> +               ctx->sysenter_cs, ctx->sysenter_esp, ctx->sysenter_eip);
> +        print_symbol(ctx->sysenter_eip, KERNEL_TEXT_ADDR);
> +        printf("\n");
> +        printf(" STAR: %08"PRIx64" SFMASK: %08"PRIx64"\n",
> +               ctx->msr_star, ctx->msr_syscall_mask);
> +        printf("LSTAR: %08"PRIx64, ctx->msr_lstar);
> +        print_symbol(ctx->msr_lstar, KERNEL_TEXT_ADDR);
> +        printf("\n");
> +        printf("CSTAR: %08"PRIx64, ctx->msr_cstar);
> +        print_symbol(ctx->msr_cstar, KERNEL_TEXT_ADDR);
> +        printf("\n");
> +        printf("\n");
> +        printf("\n");
> +        printf(" tsc: %016"PRIx64"\n", ctx->tsc);
> +        printf("\n");
> +        printf("\n");
> +        printf("pending_event: %lx  pending_vector: %lx\n",
> +               (long)(ctx->pending_event), (long)(ctx->pending_vector));
> +        printf("pending error_code: %lx\n\n", (long)(ctx->error_code));
> +    }
> +}
> +
> +static void print_cpuctx_regs32(struct hvm_hw_cpu *ctx)
> +{
> +    printf("cs:eip: %04x:%08x", ctx->cs_sel, (uint32_t)ctx->rip);
> +    print_symbol((uint32_t)ctx->rip, KERNEL_TEXT_ADDR);
> +    print_flags((uint32_t)ctx->rflags);
> +    printf("ss:esp: %04x:%08x\n", ctx->ss_sel, (uint32_t)ctx->rsp);
> +    printf("\n");
> +
> +    printf("eax: %08x\t", (uint32_t)ctx->rax);
> +    printf("ebx: %08x\t", (uint32_t)ctx->rbx);
> +    printf("ecx: %08x\t", (uint32_t)ctx->rcx);
> +    printf("edx: %08x\n", (uint32_t)ctx->rdx);
> +
> +    printf("esi: %08x\t", (uint32_t)ctx->rsi);
> +    printf("edi: %08x\t", (uint32_t)ctx->rdi);
> +    printf("ebp: %08x\n", (uint32_t)ctx->rbp);
> +    printf("\n");
> +
> +    printf("  cs: %04x @ %08"PRIx64, ctx->cs_sel, ctx->cs_base);
> +    if (ctx->cs_base)
> +        print_symbol(ctx->cs_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->cs_limit, ctx->cs_arbytes);
> +    printf("  ss: %04x @ %08"PRIx64, ctx->ss_sel, ctx->ss_base);
> +    if (ctx->ss_base)
> +        print_symbol(ctx->ss_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->ss_limit, ctx->ss_arbytes);
> +    printf("  ds: %04x @ %08"PRIx64, ctx->ds_sel, ctx->ds_base);
> +    if (ctx->ds_base)
> +        print_symbol(ctx->ds_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->ds_limit, ctx->ds_arbytes);
> +    printf("  es: %04x @ %08"PRIx64, ctx->es_sel, ctx->es_base);
> +    if (ctx->es_base)
> +        print_symbol(ctx->es_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->es_limit, ctx->es_arbytes);
> +    printf("  fs: %04x @ %08"PRIx64, ctx->fs_sel, ctx->fs_base);
> +    if (ctx->fs_base)
> +        print_symbol(ctx->fs_base, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->fs_limit, ctx->fs_arbytes);
> +    printf("  gs: %04x @ %08"PRIx64"\\%08"PRIx64, ctx->gs_sel,
> +           ctx->gs_base, ctx->shadow_gs);
> +    if (ctx->gs_base)
> +        print_symbol(ctx->gs_base, KERNEL_DATA_ADDR);
> +    printf("\\");
> +    if (ctx->shadow_gs)
> +        print_symbol(ctx->shadow_gs, KERNEL_DATA_ADDR);
> +    printf("\n           /%08x(%x)\n", ctx->gs_limit, ctx->gs_arbytes);
> +
> +    if (xenctx.disp_all) {
> +        printf("  tr: %04x @ %08"PRIx64, ctx->tr_sel, ctx->tr_base);
> +        if (ctx->tr_base)
> +            print_symbol(ctx->tr_base, KERNEL_DATA_ADDR);
> +        printf("\n           /%08x(%x)\n", ctx->tr_limit, ctx->tr_arbytes);
> +        printf(" ldt: %04x @ %08"PRIx64, ctx->ldtr_sel, ctx->ldtr_base);
> +        if (ctx->ldtr_base)
> +            print_symbol(ctx->ldtr_base, KERNEL_DATA_ADDR);
> +        printf("\n           /%08x(%x)\n", ctx->ldtr_limit, ctx->ldtr_arbytes);
> +        printf("\n");
> +        printf("cr0: %08"PRIx64"\n", ctx->cr0);
> +        printf("cr2: %08"PRIx64, ctx->cr2);
> +        print_symbol(ctx->cr2, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("cr3: %08"PRIx64"\n", ctx->cr3);
> +        printf("cr4: %08"PRIx64"\n", ctx->cr4);
> +        printf("\n");
> +        printf("dr0: %08"PRIx64, ctx->dr0);
> +        print_symbol(ctx->dr0, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("dr1: %08"PRIx64, ctx->dr1);
> +        print_symbol(ctx->dr1, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("dr2: %08"PRIx64, ctx->dr2);
> +        print_symbol(ctx->dr2, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("dr3: %08"PRIx64, ctx->dr3);
> +        print_symbol(ctx->dr3, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("dr6: %08"PRIx64"\n", ctx->dr6);
> +        printf("dr7: %08"PRIx64"\n", ctx->dr7);
> +        printf("\n");
> +        printf("gdt: %08"PRIx64"/%lx", ctx->gdtr_base, (long)(ctx->gdtr_limit));
> +        print_symbol(ctx->gdtr_base, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("idt: %08"PRIx64"/%lx", ctx->idtr_base, (long)(ctx->idtr_limit));
> +        print_symbol(ctx->idtr_base, KERNEL_DATA_ADDR);
> +        printf("\n");
> +        printf("\n");
> +        printf(" EFER: %08"PRIx64" flags: %08"PRIx64"\n",
> +               ctx->msr_efer, ctx->msr_flags);
> +        printf("SYSENTER cs: %08"PRIx64" esp: %08"PRIx64" eip: %08"PRIx64,
> +               ctx->sysenter_cs, ctx->sysenter_esp, ctx->sysenter_eip);
> +        print_symbol(ctx->sysenter_eip, KERNEL_TEXT_ADDR);
> +        printf("\n");
> +        printf(" STAR: %08"PRIx64" SFMASK: %08"PRIx64"\n",
> +               ctx->msr_star, ctx->msr_syscall_mask);
> +        printf("LSTAR: %08"PRIx64, ctx->msr_lstar);
> +        print_symbol(ctx->msr_lstar, KERNEL_TEXT_ADDR);
> +        printf("\n");
> +        printf("CSTAR: %08"PRIx64, ctx->msr_cstar);
> +        print_symbol(ctx->msr_cstar, KERNEL_TEXT_ADDR);
> +        printf("\n");
> +        printf("\n");
> +        printf(" tsc: %016"PRIx64"\n", ctx->tsc);
> +        printf("\n");
> +        printf("\n");
> +        printf("pending_event: %lx  pending_vector: %lx\n",
> +               (long)(ctx->pending_event), (long)(ctx->pending_vector));
> +        printf("pending error_code: %lx\n\n", (long)(ctx->error_code));
> +    }
> +}
> +
> +static void print_cpuctx(struct hvm_hw_cpu *ctx)
> +{
> +    if (guest_word_size == 8)
> +        print_cpuctx_regs64(ctx);
> +    else
> +        print_cpuctx_regs32(ctx);
> +
> +}
> +#endif
> +
>  #define NONPROT_MODE_SEGMENT_SHIFT 4
>  
>  static guest_word_t instr_pointer(vcpu_guest_context_any_t *ctx)
> @@ -887,6 +1128,9 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
>  static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
>  {
>      vcpu_guest_context_any_t ctx;
> +#if defined(__i386__) || defined(__x86_64__)
> +    struct hvm_hw_cpu cpuctx;
> +#endif
>  
>      if (xc_vcpu_getcontext(xenctx.xc_handle, xenctx.domid, vcpu, &ctx) < 0) {
>          perror("xc_vcpu_getcontext");
> @@ -896,7 +1140,6 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
>  #if defined(__i386__) || defined(__x86_64__)
>      {
>          if (xenctx.dominfo.hvm) {
> -            struct hvm_hw_cpu cpuctx;
>              xen_capabilities_info_t xen_caps = "";
>              if (xc_domain_hvm_getcontext_partial(
>                      xenctx.xc_handle, xenctx.domid, HVM_SAVE_CODE(CPU),
> @@ -931,7 +1174,44 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
>  #endif
>      } else {
>          if (!stk_addr) {
> -            print_ctx(&ctx);
> +#if defined(__i386__) || defined(__x86_64__)
> +            if (xenctx.dominfo.hvm && ctxt_word_size == 8) {
> +                if (guest_word_size == 4) {
> +                    if ((((uint32_t)ctx.x64.user_regs.eip) != cpuctx.rip) ||
> +                        (((uint32_t)ctx.x64.user_regs.esp) != cpuctx.rsp) ||
> +                        (((uint32_t)ctx.x64.ctrlreg[3]) != cpuctx.cr3)) {
> +                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
> +                                (long long)((uint32_t)ctx.x64.user_regs.eip),
> +                                (long long)cpuctx.rip,
> +                                (long long)((uint32_t)ctx.x64.user_regs.esp),
> +                                (long long)cpuctx.rsp,
> +                                (long long)((uint32_t)ctx.x64.ctrlreg[3]),
> +                                (long long)cpuctx.cr3);
> +                        fprintf(stdout, "=============Regs mismatch start=============\n");
> +                        print_ctx(&ctx);
> +                        fprintf(stdout, "=============Regs mismatch end=============\n");
> +                    }
> +                } else {
> +                    if ((ctx.x64.user_regs.eip != cpuctx.rip) ||
> +                        (ctx.x64.user_regs.esp != cpuctx.rsp) ||
> +                        (ctx.x64.ctrlreg[3] != cpuctx.cr3)) {
> +                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
> +                                (long long)ctx.x64.user_regs.eip,
> +                                (long long)cpuctx.rip,
> +                                (long long)ctx.x64.user_regs.esp,
> +                                (long long)cpuctx.rsp,
> +                                (long long)ctx.x64.ctrlreg[3],
> +                                (long long)cpuctx.cr3);
> +                        fprintf(stdout, "=============Regs mismatch start=============\n");
> +                        print_ctx(&ctx);
> +                        fprintf(stdout, "=============Regs mismatch end=============\n");
> +                    }
> +                }
> +                print_cpuctx(&cpuctx);
> +            }
> +            else
> +#endif
> +                print_ctx(&ctx);
>          }
>  #ifndef NO_TRANSLATION
>          if (!stk_addr) {

Re: [PATCH v2 11/12] xenctx: Dump registers via hvm info if available.

[Don Slutz]

On 11/06/13 15:35, Andrew Cooper wrote:
> On 06/11/13 20:08, Don Slutz wrote:
>> From: Don Slutz <Don@CloudSwitch.com>
>>
>> This allows the output of registers that are not available in the
>> not HVM view.
>>
>> Look up symbols for various registers as kernel data.
>>
>> Also a minor check that ip, sp, and cr3 are the same.
>>
>> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> What about tools/misc/xen-hvmctx ?
1st I have heard of it.  Will look into this.
>
> This is a particularly awkward tool where we have two tools in different
> subtrees which do about 80% the same thing.
80% looks high to me (xenctx does  do  stack, code, and symbol table also), but I do agree that having 2 tools to do 1 thing (display cpu registers) is confusing.
> I would certainly not object if someone were to merge these two tools
> into one.  It is particularly awkward as xen-hvmctx ends up being
> installed into /usr/sbin/ and is therefore on the $PATH, while xenctx
> ends up off the $PATH in /usr/lib/xen/bin/
The way I would do this is to make 1 source file, delete the 2nd, and install the same code as both xenctx and xen-hvmctx.  argv[0] can be decoded and the default action adjusted based on the name. In this case there would be command line options to get the current old output for each tool.
      -Don Slutz
> I am also fairly sure we have other examples of tools like this with
> sightly different variants living in different locations.
>
> ~Andrew
>
>> ---
>>   tools/xentrace/xenctx.c |  284 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 282 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
>> index 6ec9c74..d3cbb22 100644
>> --- a/tools/xentrace/xenctx.c
>> +++ b/tools/xentrace/xenctx.c
>> @@ -415,6 +415,247 @@ static void print_ctx(vcpu_guest_context_any_t *ctx)
>>           print_ctx_64(&ctx->x64);
>>   }
>>   
>> +#if defined(__i386__) || defined(__x86_64__)
>> +static void print_cpuctx_regs64(struct hvm_hw_cpu *ctx)
>> +{
>> +    printf("rip: %016"PRIx64, ctx->rip);
>> +    print_symbol(ctx->rip, KERNEL_TEXT_ADDR);
>> +    print_flags(ctx->rflags);
>> +    printf("rsp: %016"PRIx64"\n", ctx->rsp);
>> +
>> +    printf("rax: %016"PRIx64"\t", ctx->rax);
>> +    printf("rcx: %016"PRIx64"\t", ctx->rcx);
>> +    printf("rdx: %016"PRIx64"\n", ctx->rdx);
>> +
>> +    printf("rbx: %016"PRIx64"\t", ctx->rbx);
>> +    printf("rsi: %016"PRIx64"\t", ctx->rsi);
>> +    printf("rdi: %016"PRIx64"\n", ctx->rdi);
>> +
>> +    printf("rbp: %016"PRIx64"\t", ctx->rbp);
>> +    printf(" r8: %016"PRIx64"\t", ctx->r8);
>> +    printf(" r9: %016"PRIx64"\n", ctx->r9);
>> +
>> +    printf("r10: %016"PRIx64"\t", ctx->r10);
>> +    printf("r11: %016"PRIx64"\t", ctx->r11);
>> +    printf("r12: %016"PRIx64"\n", ctx->r12);
>> +
>> +    printf("r13: %016"PRIx64"\t", ctx->r13);
>> +    printf("r14: %016"PRIx64"\t", ctx->r14);
>> +    printf("r15: %016"PRIx64"\n", ctx->r15);
>> +
>> +    printf("  cs: %04x @ %016"PRIx64, ctx->cs_sel, ctx->cs_base);
>> +    if (ctx->cs_base)
>> +        print_symbol(ctx->cs_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->cs_limit, ctx->cs_arbytes);
>> +    printf("  ss: %04x @ %016"PRIx64, ctx->ss_sel, ctx->ss_base);
>> +    if (ctx->ss_base)
>> +        print_symbol(ctx->ss_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->ss_limit, ctx->ss_arbytes);
>> +    printf("  ds: %04x @ %016"PRIx64, ctx->ds_sel, ctx->ds_base);
>> +    if (ctx->ds_base)
>> +        print_symbol(ctx->ds_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->ds_limit, ctx->ds_arbytes);
>> +    printf("  es: %04x @ %016"PRIx64, ctx->es_sel, ctx->es_base);
>> +    if (ctx->es_base)
>> +        print_symbol(ctx->es_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->es_limit, ctx->es_arbytes);
>> +    printf("  fs: %04x @ %016"PRIx64, ctx->fs_sel, ctx->fs_base);
>> +    if (ctx->fs_base)
>> +        print_symbol(ctx->fs_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->fs_limit, ctx->fs_arbytes);
>> +    printf("  gs: %04x @ %016"PRIx64"\\%016"PRIx64, ctx->gs_sel,
>> +           ctx->gs_base, ctx->shadow_gs);
>> +    if (ctx->gs_base)
>> +        print_symbol(ctx->gs_base, KERNEL_DATA_ADDR);
>> +    printf("\\");
>> +    if (ctx->shadow_gs)
>> +        print_symbol(ctx->shadow_gs, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->gs_limit, ctx->gs_arbytes);
>> +
>> +    if (xenctx.disp_all) {
>> +        printf("  tr: %04x @ %016"PRIx64, ctx->tr_sel, ctx->tr_base);
>> +        if (ctx->tr_base)
>> +            print_symbol(ctx->tr_base, KERNEL_DATA_ADDR);
>> +        printf("\n           /%08x(%x)\n", ctx->tr_limit, ctx->tr_arbytes);
>> +        printf(" ldt: %04x @ %016"PRIx64, ctx->ldtr_sel, ctx->ldtr_base);
>> +        if (ctx->ldtr_base)
>> +            print_symbol(ctx->ldtr_base, KERNEL_DATA_ADDR);
>> +        printf("\n           /%08x(%x)\n", ctx->ldtr_limit, ctx->ldtr_arbytes);
>> +        printf("\n");
>> +        printf("cr0: %08"PRIx64"\n", ctx->cr0);
>> +        printf("cr2: %08"PRIx64, ctx->cr2);
>> +        print_symbol(ctx->cr2, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("cr3: %08"PRIx64"\n", ctx->cr3);
>> +        printf("cr4: %08"PRIx64"\n", ctx->cr4);
>> +        printf("\n");
>> +        printf("dr0: %08"PRIx64, ctx->dr0);
>> +        print_symbol(ctx->dr0, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("dr1: %08"PRIx64, ctx->dr1);
>> +        print_symbol(ctx->dr1, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("dr2: %08"PRIx64, ctx->dr2);
>> +        print_symbol(ctx->dr2, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("dr3: %08"PRIx64, ctx->dr3);
>> +        print_symbol(ctx->dr3, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("dr6: %08"PRIx64"\n", ctx->dr6);
>> +        printf("dr7: %08"PRIx64"\n", ctx->dr7);
>> +        printf("\n");
>> +        printf("gdt: %016"PRIx64"/%lx", ctx->gdtr_base, (long)(ctx->gdtr_limit));
>> +        print_symbol(ctx->gdtr_base, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("idt: %016"PRIx64"/%lx", ctx->idtr_base, (long)(ctx->idtr_limit));
>> +        print_symbol(ctx->idtr_base, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("\n");
>> +        printf(" EFER: %08"PRIx64" flags: %08"PRIx64"\n",
>> +               ctx->msr_efer, ctx->msr_flags);
>> +        printf("SYSENTER cs: %08"PRIx64" esp: %08"PRIx64" eip: %08"PRIx64,
>> +               ctx->sysenter_cs, ctx->sysenter_esp, ctx->sysenter_eip);
>> +        print_symbol(ctx->sysenter_eip, KERNEL_TEXT_ADDR);
>> +        printf("\n");
>> +        printf(" STAR: %08"PRIx64" SFMASK: %08"PRIx64"\n",
>> +               ctx->msr_star, ctx->msr_syscall_mask);
>> +        printf("LSTAR: %08"PRIx64, ctx->msr_lstar);
>> +        print_symbol(ctx->msr_lstar, KERNEL_TEXT_ADDR);
>> +        printf("\n");
>> +        printf("CSTAR: %08"PRIx64, ctx->msr_cstar);
>> +        print_symbol(ctx->msr_cstar, KERNEL_TEXT_ADDR);
>> +        printf("\n");
>> +        printf("\n");
>> +        printf("\n");
>> +        printf(" tsc: %016"PRIx64"\n", ctx->tsc);
>> +        printf("\n");
>> +        printf("\n");
>> +        printf("pending_event: %lx  pending_vector: %lx\n",
>> +               (long)(ctx->pending_event), (long)(ctx->pending_vector));
>> +        printf("pending error_code: %lx\n\n", (long)(ctx->error_code));
>> +    }
>> +}
>> +
>> +static void print_cpuctx_regs32(struct hvm_hw_cpu *ctx)
>> +{
>> +    printf("cs:eip: %04x:%08x", ctx->cs_sel, (uint32_t)ctx->rip);
>> +    print_symbol((uint32_t)ctx->rip, KERNEL_TEXT_ADDR);
>> +    print_flags((uint32_t)ctx->rflags);
>> +    printf("ss:esp: %04x:%08x\n", ctx->ss_sel, (uint32_t)ctx->rsp);
>> +    printf("\n");
>> +
>> +    printf("eax: %08x\t", (uint32_t)ctx->rax);
>> +    printf("ebx: %08x\t", (uint32_t)ctx->rbx);
>> +    printf("ecx: %08x\t", (uint32_t)ctx->rcx);
>> +    printf("edx: %08x\n", (uint32_t)ctx->rdx);
>> +
>> +    printf("esi: %08x\t", (uint32_t)ctx->rsi);
>> +    printf("edi: %08x\t", (uint32_t)ctx->rdi);
>> +    printf("ebp: %08x\n", (uint32_t)ctx->rbp);
>> +    printf("\n");
>> +
>> +    printf("  cs: %04x @ %08"PRIx64, ctx->cs_sel, ctx->cs_base);
>> +    if (ctx->cs_base)
>> +        print_symbol(ctx->cs_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->cs_limit, ctx->cs_arbytes);
>> +    printf("  ss: %04x @ %08"PRIx64, ctx->ss_sel, ctx->ss_base);
>> +    if (ctx->ss_base)
>> +        print_symbol(ctx->ss_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->ss_limit, ctx->ss_arbytes);
>> +    printf("  ds: %04x @ %08"PRIx64, ctx->ds_sel, ctx->ds_base);
>> +    if (ctx->ds_base)
>> +        print_symbol(ctx->ds_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->ds_limit, ctx->ds_arbytes);
>> +    printf("  es: %04x @ %08"PRIx64, ctx->es_sel, ctx->es_base);
>> +    if (ctx->es_base)
>> +        print_symbol(ctx->es_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->es_limit, ctx->es_arbytes);
>> +    printf("  fs: %04x @ %08"PRIx64, ctx->fs_sel, ctx->fs_base);
>> +    if (ctx->fs_base)
>> +        print_symbol(ctx->fs_base, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->fs_limit, ctx->fs_arbytes);
>> +    printf("  gs: %04x @ %08"PRIx64"\\%08"PRIx64, ctx->gs_sel,
>> +           ctx->gs_base, ctx->shadow_gs);
>> +    if (ctx->gs_base)
>> +        print_symbol(ctx->gs_base, KERNEL_DATA_ADDR);
>> +    printf("\\");
>> +    if (ctx->shadow_gs)
>> +        print_symbol(ctx->shadow_gs, KERNEL_DATA_ADDR);
>> +    printf("\n           /%08x(%x)\n", ctx->gs_limit, ctx->gs_arbytes);
>> +
>> +    if (xenctx.disp_all) {
>> +        printf("  tr: %04x @ %08"PRIx64, ctx->tr_sel, ctx->tr_base);
>> +        if (ctx->tr_base)
>> +            print_symbol(ctx->tr_base, KERNEL_DATA_ADDR);
>> +        printf("\n           /%08x(%x)\n", ctx->tr_limit, ctx->tr_arbytes);
>> +        printf(" ldt: %04x @ %08"PRIx64, ctx->ldtr_sel, ctx->ldtr_base);
>> +        if (ctx->ldtr_base)
>> +            print_symbol(ctx->ldtr_base, KERNEL_DATA_ADDR);
>> +        printf("\n           /%08x(%x)\n", ctx->ldtr_limit, ctx->ldtr_arbytes);
>> +        printf("\n");
>> +        printf("cr0: %08"PRIx64"\n", ctx->cr0);
>> +        printf("cr2: %08"PRIx64, ctx->cr2);
>> +        print_symbol(ctx->cr2, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("cr3: %08"PRIx64"\n", ctx->cr3);
>> +        printf("cr4: %08"PRIx64"\n", ctx->cr4);
>> +        printf("\n");
>> +        printf("dr0: %08"PRIx64, ctx->dr0);
>> +        print_symbol(ctx->dr0, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("dr1: %08"PRIx64, ctx->dr1);
>> +        print_symbol(ctx->dr1, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("dr2: %08"PRIx64, ctx->dr2);
>> +        print_symbol(ctx->dr2, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("dr3: %08"PRIx64, ctx->dr3);
>> +        print_symbol(ctx->dr3, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("dr6: %08"PRIx64"\n", ctx->dr6);
>> +        printf("dr7: %08"PRIx64"\n", ctx->dr7);
>> +        printf("\n");
>> +        printf("gdt: %08"PRIx64"/%lx", ctx->gdtr_base, (long)(ctx->gdtr_limit));
>> +        print_symbol(ctx->gdtr_base, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("idt: %08"PRIx64"/%lx", ctx->idtr_base, (long)(ctx->idtr_limit));
>> +        print_symbol(ctx->idtr_base, KERNEL_DATA_ADDR);
>> +        printf("\n");
>> +        printf("\n");
>> +        printf(" EFER: %08"PRIx64" flags: %08"PRIx64"\n",
>> +               ctx->msr_efer, ctx->msr_flags);
>> +        printf("SYSENTER cs: %08"PRIx64" esp: %08"PRIx64" eip: %08"PRIx64,
>> +               ctx->sysenter_cs, ctx->sysenter_esp, ctx->sysenter_eip);
>> +        print_symbol(ctx->sysenter_eip, KERNEL_TEXT_ADDR);
>> +        printf("\n");
>> +        printf(" STAR: %08"PRIx64" SFMASK: %08"PRIx64"\n",
>> +               ctx->msr_star, ctx->msr_syscall_mask);
>> +        printf("LSTAR: %08"PRIx64, ctx->msr_lstar);
>> +        print_symbol(ctx->msr_lstar, KERNEL_TEXT_ADDR);
>> +        printf("\n");
>> +        printf("CSTAR: %08"PRIx64, ctx->msr_cstar);
>> +        print_symbol(ctx->msr_cstar, KERNEL_TEXT_ADDR);
>> +        printf("\n");
>> +        printf("\n");
>> +        printf(" tsc: %016"PRIx64"\n", ctx->tsc);
>> +        printf("\n");
>> +        printf("\n");
>> +        printf("pending_event: %lx  pending_vector: %lx\n",
>> +               (long)(ctx->pending_event), (long)(ctx->pending_vector));
>> +        printf("pending error_code: %lx\n\n", (long)(ctx->error_code));
>> +    }
>> +}
>> +
>> +static void print_cpuctx(struct hvm_hw_cpu *ctx)
>> +{
>> +    if (guest_word_size == 8)
>> +        print_cpuctx_regs64(ctx);
>> +    else
>> +        print_cpuctx_regs32(ctx);
>> +
>> +}
>> +#endif
>> +
>>   #define NONPROT_MODE_SEGMENT_SHIFT 4
>>   
>>   static guest_word_t instr_pointer(vcpu_guest_context_any_t *ctx)
>> @@ -887,6 +1128,9 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
>>   static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
>>   {
>>       vcpu_guest_context_any_t ctx;
>> +#if defined(__i386__) || defined(__x86_64__)
>> +    struct hvm_hw_cpu cpuctx;
>> +#endif
>>   
>>       if (xc_vcpu_getcontext(xenctx.xc_handle, xenctx.domid, vcpu, &ctx) < 0) {
>>           perror("xc_vcpu_getcontext");
>> @@ -896,7 +1140,6 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
>>   #if defined(__i386__) || defined(__x86_64__)
>>       {
>>           if (xenctx.dominfo.hvm) {
>> -            struct hvm_hw_cpu cpuctx;
>>               xen_capabilities_info_t xen_caps = "";
>>               if (xc_domain_hvm_getcontext_partial(
>>                       xenctx.xc_handle, xenctx.domid, HVM_SAVE_CODE(CPU),
>> @@ -931,7 +1174,44 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
>>   #endif
>>       } else {
>>           if (!stk_addr) {
>> -            print_ctx(&ctx);
>> +#if defined(__i386__) || defined(__x86_64__)
>> +            if (xenctx.dominfo.hvm && ctxt_word_size == 8) {
>> +                if (guest_word_size == 4) {
>> +                    if ((((uint32_t)ctx.x64.user_regs.eip) != cpuctx.rip) ||
>> +                        (((uint32_t)ctx.x64.user_regs.esp) != cpuctx.rsp) ||
>> +                        (((uint32_t)ctx.x64.ctrlreg[3]) != cpuctx.cr3)) {
>> +                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
>> +                                (long long)((uint32_t)ctx.x64.user_regs.eip),
>> +                                (long long)cpuctx.rip,
>> +                                (long long)((uint32_t)ctx.x64.user_regs.esp),
>> +                                (long long)cpuctx.rsp,
>> +                                (long long)((uint32_t)ctx.x64.ctrlreg[3]),
>> +                                (long long)cpuctx.cr3);
>> +                        fprintf(stdout, "=============Regs mismatch start=============\n");
>> +                        print_ctx(&ctx);
>> +                        fprintf(stdout, "=============Regs mismatch end=============\n");
>> +                    }
>> +                } else {
>> +                    if ((ctx.x64.user_regs.eip != cpuctx.rip) ||
>> +                        (ctx.x64.user_regs.esp != cpuctx.rsp) ||
>> +                        (ctx.x64.ctrlreg[3] != cpuctx.cr3)) {
>> +                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
>> +                                (long long)ctx.x64.user_regs.eip,
>> +                                (long long)cpuctx.rip,
>> +                                (long long)ctx.x64.user_regs.esp,
>> +                                (long long)cpuctx.rsp,
>> +                                (long long)ctx.x64.ctrlreg[3],
>> +                                (long long)cpuctx.cr3);
>> +                        fprintf(stdout, "=============Regs mismatch start=============\n");
>> +                        print_ctx(&ctx);
>> +                        fprintf(stdout, "=============Regs mismatch end=============\n");
>> +                    }
>> +                }
>> +                print_cpuctx(&cpuctx);
>> +            }
>> +            else
>> +#endif
>> +                print_ctx(&ctx);
>>           }
>>   #ifndef NO_TRANSLATION
>>           if (!stk_addr) {

Re: [PATCH v2 11/12] xenctx: Dump registers via hvm info if available.

[Ian Campbell]

On Thu, 2013-11-07 at 13:56 -0500, Don Slutz wrote:
> On 11/06/13 15:35, Andrew Cooper wrote:

> > I would certainly not object if someone were to merge these two tools
> > into one.  It is particularly awkward as xen-hvmctx ends up being
> > installed into /usr/sbin/ and is therefore on the $PATH, while xenctx
> > ends up off the $PATH in /usr/lib/xen/bin/
> The way I would do this is to make 1 source file, delete the 2nd, and
> install the same code as both xenctx and xen-hvmctx.  argv[0] can be
> decoded and the default action adjusted based on the name. In this
> case there would be command line options to get the current old output
> for each tool.

Personally I don't think we need to retain any kind of CLI compatibility
for such developer focused tools. I'd be perfectly happy for the
(sensible) functionality of hvmctx to be merged into xenctx and to then
disappear.

Ian.

Re: [PATCH v2 11/12] xenctx: Dump registers via hvm info if available.

[Andrew Cooper]

On 08/11/13 10:13, Ian Campbell wrote:
> On Thu, 2013-11-07 at 13:56 -0500, Don Slutz wrote:
>> On 11/06/13 15:35, Andrew Cooper wrote:
>>> I would certainly not object if someone were to merge these two tools
>>> into one.  It is particularly awkward as xen-hvmctx ends up being
>>> installed into /usr/sbin/ and is therefore on the $PATH, while xenctx
>>> ends up off the $PATH in /usr/lib/xen/bin/
>> The way I would do this is to make 1 source file, delete the 2nd, and
>> install the same code as both xenctx and xen-hvmctx.  argv[0] can be
>> decoded and the default action adjusted based on the name. In this
>> case there would be command line options to get the current old output
>> for each tool.
> Personally I don't think we need to retain any kind of CLI compatibility
> for such developer focused tools. I'd be perfectly happy for the
> (sensible) functionality of hvmctx to be merged into xenctx and to then
> disappear.
>
> Ian.
>

Agreed - I would much prefer one single tool with a useful set of
options rather than keeping around something which vaguely looks like an
older tool for the sake of it.

This is strictly a dev tool rather than production tool.

~Andrew

Re: [PATCH v2 11/12] xenctx: Dump registers via hvm info if available.

[Ian Campbell]

On Fri, 2013-11-08 at 10:29 +0000, Andrew Cooper wrote:
> On 08/11/13 10:13, Ian Campbell wrote:
> > On Thu, 2013-11-07 at 13:56 -0500, Don Slutz wrote:
> >> On 11/06/13 15:35, Andrew Cooper wrote:
> >>> I would certainly not object if someone were to merge these two tools
> >>> into one.  It is particularly awkward as xen-hvmctx ends up being
> >>> installed into /usr/sbin/ and is therefore on the $PATH, while xenctx
> >>> ends up off the $PATH in /usr/lib/xen/bin/
> >> The way I would do this is to make 1 source file, delete the 2nd, and
> >> install the same code as both xenctx and xen-hvmctx.  argv[0] can be
> >> decoded and the default action adjusted based on the name. In this
> >> case there would be command line options to get the current old output
> >> for each tool.
> > Personally I don't think we need to retain any kind of CLI compatibility
> > for such developer focused tools. I'd be perfectly happy for the
> > (sensible) functionality of hvmctx to be merged into xenctx and to then
> > disappear.
> >
> > Ian.
> >
> 
> Agreed - I would much prefer one single tool with a useful set of
> options rather than keeping around something which vaguely looks like an
> older tool for the sake of it.
> 
> This is strictly a dev tool rather than production tool.

Right. and on that basis In the short term while this is being sorted
out I'd be happy with a patch to move hvmctx into LIBDIR instead of
SBINDIR too, so it is less prominent.

(the question of whether xenctx really belongs in $PATH or not is one we
can consider separately later...)

Ian.

Re: [PATCH v2 11/12] xenctx: Dump registers via hvm info if available.

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> @@ -931,7 +1174,44 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
>  #endif
>      } else {
>          if (!stk_addr) {
> -            print_ctx(&ctx);
> +#if defined(__i386__) || defined(__x86_64__)
> +            if (xenctx.dominfo.hvm && ctxt_word_size == 8) {
> +                if (guest_word_size == 4) {
> +                    if ((((uint32_t)ctx.x64.user_regs.eip) != cpuctx.rip) ||
> +                        (((uint32_t)ctx.x64.user_regs.esp) != cpuctx.rsp) ||
> +                        (((uint32_t)ctx.x64.ctrlreg[3]) != cpuctx.cr3)) {
> +                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
> +                                (long long)((uint32_t)ctx.x64.user_regs.eip),
> +                                (long long)cpuctx.rip,
> +                                (long long)((uint32_t)ctx.x64.user_regs.esp),
> +                                (long long)cpuctx.rsp,
> +                                (long long)((uint32_t)ctx.x64.ctrlreg[3]),
> +                                (long long)cpuctx.cr3);
> +                        fprintf(stdout, "=============Regs mismatch start=============\n");
> +                        print_ctx(&ctx);
> +                        fprintf(stdout, "=============Regs mismatch end=============\n");
> +                    }
> +                } else {
> +                    if ((ctx.x64.user_regs.eip != cpuctx.rip) ||
> +                        (ctx.x64.user_regs.esp != cpuctx.rsp) ||
> +                        (ctx.x64.ctrlreg[3] != cpuctx.cr3)) {
> +                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
> +                                (long long)ctx.x64.user_regs.eip,
> +                                (long long)cpuctx.rip,
> +                                (long long)ctx.x64.user_regs.esp,
> +                                (long long)cpuctx.rsp,
> +                                (long long)ctx.x64.ctrlreg[3],
> +                                (long long)cpuctx.cr3);
> +                        fprintf(stdout, "=============Regs mismatch start=============\n");
> +                        print_ctx(&ctx);
> +                        fprintf(stdout, "=============Regs mismatch end=============\n");
> +                    }
> +                }
> +                print_cpuctx(&cpuctx);
> +            }
> +            else
> +#endif
> +                print_ctx(&ctx);

Apart from Andrew's comments, which I agree with - most of the
additions above clearly don't belong here: This is not a diagnostic
utility.

Jan

Re: [PATCH v2 11/12] xenctx: Dump registers via hvm info if available.

[Don Slutz]

On 11/07/13 03:38, Jan Beulich wrote:
>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>> @@ -931,7 +1174,44 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
>>   #endif
>>       } else {
>>           if (!stk_addr) {
>> -            print_ctx(&ctx);
>> +#if defined(__i386__) || defined(__x86_64__)
>> +            if (xenctx.dominfo.hvm && ctxt_word_size == 8) {
>> +                if (guest_word_size == 4) {
>> +                    if ((((uint32_t)ctx.x64.user_regs.eip) != cpuctx.rip) ||
>> +                        (((uint32_t)ctx.x64.user_regs.esp) != cpuctx.rsp) ||
>> +                        (((uint32_t)ctx.x64.ctrlreg[3]) != cpuctx.cr3)) {
>> +                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
>> +                                (long long)((uint32_t)ctx.x64.user_regs.eip),
>> +                                (long long)cpuctx.rip,
>> +                                (long long)((uint32_t)ctx.x64.user_regs.esp),
>> +                                (long long)cpuctx.rsp,
>> +                                (long long)((uint32_t)ctx.x64.ctrlreg[3]),
>> +                                (long long)cpuctx.cr3);
>> +                        fprintf(stdout, "=============Regs mismatch start=============\n");
>> +                        print_ctx(&ctx);
>> +                        fprintf(stdout, "=============Regs mismatch end=============\n");
>> +                    }
>> +                } else {
>> +                    if ((ctx.x64.user_regs.eip != cpuctx.rip) ||
>> +                        (ctx.x64.user_regs.esp != cpuctx.rsp) ||
>> +                        (ctx.x64.ctrlreg[3] != cpuctx.cr3)) {
>> +                        fprintf(stderr, "Regs mismatch ip=%llx vs %llx sp=%llx vs %llx cr3=%llx vs %llx\n",
>> +                                (long long)ctx.x64.user_regs.eip,
>> +                                (long long)cpuctx.rip,
>> +                                (long long)ctx.x64.user_regs.esp,
>> +                                (long long)cpuctx.rsp,
>> +                                (long long)ctx.x64.ctrlreg[3],
>> +                                (long long)cpuctx.cr3);
>> +                        fprintf(stdout, "=============Regs mismatch start=============\n");
>> +                        print_ctx(&ctx);
>> +                        fprintf(stdout, "=============Regs mismatch end=============\n");
>> +                    }
>> +                }
>> +                print_cpuctx(&cpuctx);
>> +            }
>> +            else
>> +#endif
>> +                print_ctx(&ctx);
> Apart from Andrew's comments, which I agree with - most of the
> additions above clearly don't belong here: This is not a diagnostic
> utility.
Fine with me, I will drop this part. I added during the time (~2010-2011) when I was looking at a DomU that was crashed.  What I remember about this DomU was that vCPU 1 was offline and vCPU 0 was the crash cause and vCPU 2 & 3 where in the code to go offline. The routine xc_domain_hvm_getcontext_partial() was returning vCPU 2's data when asked for vCPU 1 (via instance == 1).  This is the key reason I did the code in patch #12 (basically a way to call xc_domain_hvm_getcontext_partial() and xc_vcpu_getcontext() with different args).  At the time I was not working on full xen code, just changing xenctx the help me out as needed.  Looking at the code now, I do not understand how this could happen.
     -Don Slutz
> Jan
>

[PATCH v2 12/12] xenctx: Add optional fCPU.

[Don Slutz]

From: Don Slutz <Don@CloudSwitch.com>

This can be used to dump the normal ctx for an HVM DomU by passing
-1 to fCPU.

If only some of the vCPUs in a HVM DomU, you need to ask for ask for
the on line CPU number, not the expected vCPU number; fCPU can be
used for this.

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 tools/xentrace/xenctx.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index d3cbb22..a7a791a 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -1125,7 +1125,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
 }
 #endif
 
-static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
+static void dump_ctx(int vcpu, int fcpu, guest_word_t mem_addr, guest_word_t stk_addr)
 {
     vcpu_guest_context_any_t ctx;
 #if defined(__i386__) || defined(__x86_64__)
@@ -1139,11 +1139,11 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
 
 #if defined(__i386__) || defined(__x86_64__)
     {
-        if (xenctx.dominfo.hvm) {
+        if (fcpu >= 0 && xenctx.dominfo.hvm) {
             xen_capabilities_info_t xen_caps = "";
             if (xc_domain_hvm_getcontext_partial(
                     xenctx.xc_handle, xenctx.domid, HVM_SAVE_CODE(CPU),
-                    vcpu, &cpuctx, sizeof cpuctx) != 0) {
+                    fcpu, &cpuctx, sizeof cpuctx) != 0) {
                 perror("xc_domain_hvm_getcontext_partial");
                 return;
             }
@@ -1175,7 +1175,7 @@ static void dump_ctx(int vcpu, guest_word_t mem_addr, guest_word_t stk_addr)
     } else {
         if (!stk_addr) {
 #if defined(__i386__) || defined(__x86_64__)
-            if (xenctx.dominfo.hvm && ctxt_word_size == 8) {
+            if (fcpu >= 0 && xenctx.dominfo.hvm && ctxt_word_size == 8) {
                 if (guest_word_size == 4) {
                     if ((((uint32_t)ctx.x64.user_regs.eip) != cpuctx.rip) ||
                         (((uint32_t)ctx.x64.user_regs.esp) != cpuctx.rsp) ||
@@ -1229,12 +1229,22 @@ static void dump_all_vcpus(guest_word_t mem_addr, guest_word_t stk_addr)
 {
     xc_vcpuinfo_t vinfo;
     int vcpu;
+    int fcpu = 0;
+
     for (vcpu = 0; vcpu <= xenctx.dominfo.max_vcpu_id; vcpu++)
     {
         if ( xc_vcpu_getinfo(xenctx.xc_handle, xenctx.domid, vcpu, &vinfo) )
             continue;
-        if ( vinfo.online )
-            dump_ctx(vcpu, mem_addr, stk_addr);
+        if ( vinfo.online ) {
+            printf("vcpu=%d(%d)\n", vcpu, fcpu);
+            dump_ctx(vcpu, fcpu, mem_addr, stk_addr);
+            fcpu++;
+            printf("\n");
+        } else {
+            printf("vcpu=%d offline\n", vcpu);
+            dump_ctx(vcpu, -1, mem_addr, stk_addr);
+            printf("\n");
+        }
     }
 }
 
@@ -1242,7 +1252,7 @@ static void usage(void)
 {
     printf("usage:\n\n");
 
-    printf("  xenctx [options] <DOMAIN> [VCPU]\n\n");
+    printf("  xenctx [options] <DOMAIN> [VCPU] [fCPU]\n\n");
 
     printf("options:\n");
     printf("  -f, --frame-pointers\n");
@@ -1285,6 +1295,7 @@ int main(int argc, char **argv)
     guest_word_t stk_addr = 0;
 
     int vcpu = 0;
+    int fcpu = 0;
 
     while ((ch = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) {
         switch(ch) {
@@ -1326,8 +1337,8 @@ int main(int argc, char **argv)
 
     argv += optind; argc -= optind;
 
-    if (argc < 1 || argc > 2) {
-        printf("usage: xenctx [options] <domid> <optional vcpu>\n");
+    if (argc < 1 || argc > 3) {
+        printf("usage: xenctx [options] <domid> <optional vcpu> <optional fcpu>\n");
         exit(-1);
     }
 
@@ -1337,8 +1348,14 @@ int main(int argc, char **argv)
             exit(-1);
     }
 
-    if (argc == 2)
+    if (argc == 2) {
         vcpu = atoi(argv[1]);
+        fcpu = vcpu;
+    }
+
+    if (argc == 3) {
+        fcpu = atoi(argv[2]);
+    }
 
     if (symbol_table)
         read_symbol_table(symbol_table);
@@ -1367,7 +1384,7 @@ int main(int argc, char **argv)
     if (xenctx.all_vcpus)
         dump_all_vcpus(mem_addr, stk_addr);
     else
-        dump_ctx(vcpu, mem_addr, stk_addr);
+        dump_ctx(vcpu, fcpu, mem_addr, stk_addr);
 
     if (xenctx.self_paused) {
         ret = xc_domain_unpause(xenctx.xc_handle, xenctx.domid);
-- 
1.7.1

Re: [PATCH v2 12/12] xenctx: Add optional fCPU.

[Jan Beulich]

>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> This can be used to dump the normal ctx for an HVM DomU by passing
> -1 to fCPU.
> 
> If only some of the vCPUs in a HVM DomU, you need to ask for ask for
> the on line CPU number, not the expected vCPU number; fCPU can be
> used for this.

I'm sorry, but I can't make sense of this description, and hence
can't review the patch.

Jan

Re: [PATCH v2 12/12] xenctx: Add optional fCPU.

[Ian Campbell]

On Thu, 2013-11-07 at 08:40 +0000, Jan Beulich wrote:
> >>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
> > This can be used to dump the normal ctx for an HVM DomU by passing
> > -1 to fCPU.
> > 
> > If only some of the vCPUs in a HVM DomU, you need to ask for ask for
> > the on line CPU number, not the expected vCPU number; fCPU can be
> > used for this.
> 
> I'm sorry, but I can't make sense of this description, and hence
> can't review the patch.

Yeah, I'm wondering what the f stands for.

Ian

Re: [PATCH v2 12/12] xenctx: Add optional fCPU.

[Don Slutz]

[-- Attachment #1.1: Type: text/plain, Size: 1306 bytes --]

On 11/07/13 07:53, Ian Campbell wrote:
> On Thu, 2013-11-07 at 08:40 +0000, Jan Beulich wrote:
>>>>> On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> wrote:
>>> This can be used to dump the normal ctx for an HVM DomU by passing
>>> -1 to fCPU.
>>>
>>> If only some of the vCPUs in a HVM DomU, you need to ask for ask for
>>> the on line CPU number, not the expected vCPU number; fCPU can be
>>> used for this.
>> I'm sorry, but I can't make sense of this description, and hence
>> can't review the patch.
> Yeah, I'm wondering what the f stands for.
>
> Ian
>
As I have said in

Subject: 	Re: [PATCH v2 11/12] xenctx: Dump registers via hvm info if available.
Date: 	Thu, 7 Nov 2013 16:19:34 -0500
From: 	Don Slutz <dslutz@terremark.com>
To: 	Jan Beulich <JBeulich@suse.com>, Don Slutz <dslutz@verizon.com>
CC: 	Ian Campbell <ian.campbell@citrix.com>, Don Slutz <Don@CloudSwitch.com>, George Dunlap <george.dunlap@eu.citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, xen-devel <xen-devel@lists.xenproject.org>


This was code to work around a bug.  I do not see how I got the result I did and since that DomU (and old xen version) is long gone, I will attempt to get a DomU in a similar state and check that this is not need.
     -Don Slutz


[-- Attachment #1.2: Type: text/html, Size: 3617 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 00/12] xenctx: Many changes.

[George Dunlap]

On 06/11/13 20:08, Don Slutz wrote:
> From: Don Slutz <dslutz@verizon.com>
>
> Change from v1 to v2:
>    title was: xenctx: Add an option to output more registers.
>    Processed review comments.
>    Jan Beulich:
>      Split 1 change into 12.
>      Switch to enum for is_kernel_text(), renamed to is_kernel_addr().
>      Renamed vars like memAddr to mem_addr.
>    Ian Campbell:
>      More on is_kernel_text().

I don't think this series really needs my Ack -- xenctx is actually in 
the wrong place, AFAICT, as it doesn't have anything really to do with 
xentrace.  Its functionality is more similar to xen-hvmctx, which ATM 
lives in tools/misc.

At some point it might make sense to reorg stuff a bit...

  -George

Re: [PATCH v2 00/12] xenctx: Many changes.

[Ian Campbell]

On Fri, 2013-11-08 at 11:29 +0000, George Dunlap wrote:

> At some point it might make sense to reorg stuff a bit...

Agreed.