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

[Don Slutz <dslutz@verizon.com>]

On 03/19/14 13:08, George Dunlap wrote:
> 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.
>

You are right, Will remove.


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

They are not quite the same.  print_stack also tests for stack limit.  
So yes, I will make a common routine that they both use.



>> +    }
>> +}
>> +#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?)
>

Since -C dumps the ctx for each vcpu, outputting the ctx for vcpu 0 
twice (or other specified vcpu) would be strange.  It would be better to 
report an error if both -C and a vcpu was specified.

I can either add the check to this patch, or add another patch.

    -Don Slutz

    -Don Slutz


>  -George