Re: [PATCH v4 10/15] xenctx: Add -M <maddr> option to dump memory at maddr.

[George Dunlap <george.dunlap@eu.citrix.com>]

On 03/18/2014 10:15 PM, Don Slutz wrote:
> Currently not supported on ARM.
>
> New routine read_mem_word() will correctly read a word that crosses
> a page boundary.  It will not fault if the 2nd page can not be
> mapped.
>
> Here is an example:
>
> Memory (address ffffffff803ddf90):
>   ffffffff80048d19 0000000000200800 ffffffff803e7801 0000000000086800
>   0000000000000000 ffffffff80430720 ffffffff803e722f 80008e000010019c
>   00000000ffffffff 0000000000000000 0000000000000000 0000000000200000
>   0000000000000000 0000000000000000 0000000000000000 00cf9b000000ffff
>   00af9b000000ffff 00cf93000000ffff 00cffb000000ffff 00cff3000000ffff
>
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> ---
> v4 Changed from -m to -M
>
>   tools/xentrace/xenctx.c | 168 ++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 150 insertions(+), 18 deletions(-)
>
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index e5c45df..e7d490e 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -29,23 +29,6 @@
>   #include <xen/foreign/x86_64.h>
>   #include <xen/hvm/save.h>
>   
> -static struct xenctx {
> -    xc_interface *xc_handle;
> -    int domid;
> -    int frame_ptrs;
> -    int stack_trace;
> -    int disp_all;
> -    int multiple_pages;
> -    int bytes_per_line;
> -    int lines;
> -    int decode_as_ascii;
> -    int tag_stack_dump;
> -    int tag_call_trace;
> -    int all_vcpus;
> -    int self_paused;
> -    xc_dominfo_t dominfo;
> -} xenctx;
> -
>   #if defined (__i386__) || defined (__x86_64__)
>   typedef unsigned long long guest_word_t;
>   #define FMT_32B_WORD "%08llx"
> @@ -69,6 +52,27 @@ typedef uint64_t guest_word_t;
>   
>   #define MAX_BYTES_PER_LINE 128
>   
> +static struct xenctx {
> +    xc_interface *xc_handle;
> +    int domid;
> +    int frame_ptrs;
> +    int stack_trace;
> +    int disp_all;
> +    int multiple_pages;
> +    int bytes_per_line;
> +    int lines;
> +    int decode_as_ascii;
> +    int tag_stack_dump;
> +    int tag_call_trace;
> +    int all_vcpus;
> +#ifndef NO_TRANSLATION
> +    guest_word_t mem_addr;
> +    int do_memory;
> +#endif
> +    int self_paused;
> +    xc_dominfo_t dominfo;
> +} xenctx;
> +
>   struct symbol {
>       guest_word_t address;
>       char *name;
> @@ -630,6 +634,37 @@ static guest_word_t read_stack_word(guest_word_t *src, int width)
>       return word;
>   }
>   
> +#ifndef NO_TRANSLATION

You seem to be nesting "#ifndef NO_TRANSLATION" here -- there's already 
an #ifndef up above map_page.

> +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;
> +    }
> +}
> +#endif
> +
>   static void print_stack_word(guest_word_t word, int width)
>   {
>       if (width == 4)
> @@ -638,6 +673,67 @@ static void print_stack_word(guest_word_t word, int width)
>           printf(FMT_64B_WORD, word);
>   }
>   
> +#ifndef NO_TRANSLATION
> +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[MAX_BYTES_PER_LINE/4];
> +    int i;
> +
> +    instr_start = mem_addr;
> +    instr = mem_addr;
> +    printf("Memory (address ");
> +    print_stack_word(instr, width);
> +    printf("):\n");
> +    for (i = 1; i < xenctx.lines + 1; i++)
> +    {
> +        int j = 0;
> +        int k;
> +
> +        if ( xenctx.tag_stack_dump )
> +        {
> +            print_stack_word(instr, width);
> +            printf(":");
> +        }
> +        while ( instr < instr_start + i * xenctx.bytes_per_line )
> +        {
> +            void *p = map_page(ctx, vcpu, instr);
> +            if ( !p )
> +                return;
> +            word = read_mem_word(ctx, vcpu, instr, width);
> +            if ( xenctx.decode_as_ascii )
> +                ascii[j++] = word;
> +            printf(" ");
> +            print_stack_word(word, width);
> +            instr += width;
> +        }
> +        printf("  ");
> +        if ( xenctx.decode_as_ascii )
> +        {
> +            for (k = j; k < xenctx.bytes_per_line / width; k++)
> +                printf(" %*s", width*2, "");
> +            for (k = 0; k < j; k++)
> +            {
> +                int l;
> +                unsigned char *bytep = (unsigned char*)&ascii[k];
> +
> +                for (l = 0; l < width; l++)
> +                {
> +                    if ( (*bytep < 127) && (*bytep >= 32) )
> +                        printf("%c", *bytep);
> +                    else
> +                        printf(".");
> +                    bytep++;
> +                }
> +            }
> +        }
> +        printf("\n");

This inner loop seems to be an exact copy of the code in print_stack -- 
wouldn't it make sense to make a generic "dump memory area" function, 
and have both print_mem and print_stack call it?

> +    }
> +}
> +#endif
> +
>   static int print_code(vcpu_guest_context_any_t *ctx, int vcpu)
>   {
>       guest_word_t instr;
> @@ -874,6 +970,13 @@ static void dump_ctx(int vcpu)
>       }
>   #endif
>   
> +#ifndef NO_TRANSLATION
> +    if ( xenctx.do_memory )
> +    {
> +        print_mem(&ctx, vcpu, guest_word_size, xenctx.mem_addr);
> +        return;
> +    }
> +#endif
>       print_ctx(&ctx);
>   #ifndef NO_TRANSLATION
>       if (print_code(&ctx, vcpu))
> @@ -928,13 +1031,21 @@ static void usage(void)
>       printf("                     add address on each line of Stack dump.\n");
>       printf("  -T, --tag-call-trace\n");
>       printf("                     add address on each line of Call trace.\n");
> +#ifndef NO_TRANSLATION
> +    printf("  -M maddr, --memory=maddr\n");
> +    printf("                     dump memory at maddr.\n");
> +#endif
>   }
>   
>   int main(int argc, char **argv)
>   {
>       int ch;
>       int ret;
> +#ifndef NO_TRANSLATION
> +    static const char *sopts = "fs:hak:SCmb:l:DtTM:";
> +#else
>       static const char *sopts = "fs:hak:SCmb:l:DtT";
> +#endif
>       static const struct option lopts[] = {
>           {"stack-trace", 0, NULL, 'S'},
>           {"symbol-table", 1, NULL, 's'},
> @@ -944,6 +1055,9 @@ int main(int argc, char **argv)
>           {"decode-as-ascii", 0, NULL, 'D'},
>           {"tag-stack-dump", 0, NULL, 't'},
>           {"tag-call-trace", 0, NULL, 'T'},
> +#ifndef NO_TRANSLATION
> +        {"memory", 1, NULL, 'M'},
> +#endif
>           {"bytes-per-line", 1, NULL, 'b'},
>           {"lines", 1, NULL, 'l'},
>           {"all", 0, NULL, 'a'},
> @@ -954,6 +1068,7 @@ int main(int argc, char **argv)
>       const char *symbol_table = NULL;
>   
>       int vcpu = 0;
> +    int do_default = 1;
>   
>       xenctx.bytes_per_line = 32;
>       xenctx.lines = 5;
> @@ -1007,10 +1122,18 @@ int main(int argc, char **argv)
>               break;
>           case 'C':
>               xenctx.all_vcpus = 1;
> +            do_default = 0;
>               break;
>           case 'k':
>               kernel_start = strtoull(optarg, NULL, 0);
>               break;
> +#ifndef NO_TRANSLATION
> +        case 'M':
> +            xenctx.mem_addr = strtoull(optarg, NULL, 0);
> +            xenctx.do_memory = 1;
> +            do_default = 0;
> +            break;
> +#endif
>           case 'h':
>               usage();
>               exit(-1);
> @@ -1060,9 +1183,18 @@ int main(int argc, char **argv)
>           xenctx.self_paused = 1;
>       }
>   
> +#ifndef NO_TRANSLATION
> +    if ( xenctx.do_memory )
> +    {
> +        dump_ctx(vcpu);
> +        if (xenctx.all_vcpus)
> +            printf("\n");
> +    }
> +    xenctx.do_memory = 0;
> +#endif
>       if (xenctx.all_vcpus)
>           dump_all_vcpus();
> -    else
> +    if ( do_default )
>           dump_ctx(vcpu);

So after this change, you can do "xenctx -M <addr>" and get memory, 
"xenctx -C" and get vcpu info, or "xenctx -M <addr> -C" and get both; 
but you can't get the ctx with both.  Would it make sense to add an 
option to explicitly request dump_ctx(), so that you could have any 
combination?  (And of course default to dump_ctx() if nothing is specified?)

  -George