From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v4 10/15] xenctx: Add -M option to dump memory at maddr. Date: Wed, 19 Mar 2014 21:36:40 -0400 Message-ID: <532A4628.6050405@terremark.com> References: <1395180940-23901-1-git-send-email-dslutz@verizon.com> <1395180940-23901-11-git-send-email-dslutz@verizon.com> <5329CEF1.9010603@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5329CEF1.9010603@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Ian Campbell , Stefano Stabellini , Don Slutz , Ian Jackson , Don Slutz , xen-devel@lists.xen.org, Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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 >> #include >> -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 " and get memory, > "xenctx -C" and get vcpu info, or "xenctx -M -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