| Message ID | 4F333C82.4070003@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 2012-02-09 04:24, Wen Congyang wrote: > Crash needs extra memory mapping to determine phys_base. > > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > --- > cpu-all.h | 2 ++ > target-i386/arch-dump.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+), 0 deletions(-) > > diff --git a/cpu-all.h b/cpu-all.h > index efb5ba3..290c43a 100644 > --- a/cpu-all.h > +++ b/cpu-all.h > @@ -530,10 +530,12 @@ int cpu_write_elf64_note(int fd, CPUState *env, int cpuid, > target_phys_addr_t *offset); > int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, > target_phys_addr_t *offset); > +int cpu_add_extra_memory_mapping(MemoryMappingList *list); > #else > #define cpu_get_memory_mapping(list, env) > #define cpu_write_elf64_note(fd, env, cpuid, offset) ({ -1; }) > #define cpu_write_elf32_note(fd, env, cpuid, offset) ({ -1; }) > +#define cpu_add_extra_memory_mapping(list) ({ 0; }) > #endif > > #endif /* CPU_ALL_H */ > diff --git a/target-i386/arch-dump.c b/target-i386/arch-dump.c > index 4c0ff77..d96f6ae 100644 > --- a/target-i386/arch-dump.c > +++ b/target-i386/arch-dump.c > @@ -495,3 +495,46 @@ int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, > { > return x86_write_elf32_note(fd, env, cpuid, offset); > } > + > +/* This function is copied from crash */ And what does it do there and here? I suppose it is Linux-specific - any version? This should be documented and encoded in the function name. > +static target_ulong get_phys_base_addr(CPUState *env, target_ulong *base_vaddr) > +{ > + int i; > + target_ulong kernel_base = -1; > + target_ulong last, mask; > + > + for (i = 30, last = -1; (kernel_base == -1) && (i >= 20); i--) { > + mask = ~((1LL << i) - 1); > + *base_vaddr = env->idt.base & mask; > + if (*base_vaddr == last) { > + continue; > + } > + > + kernel_base = cpu_get_phys_page_debug(env, *base_vaddr); > + last = *base_vaddr; > + } > + > + return kernel_base; > +} > + > +int cpu_add_extra_memory_mapping(MemoryMappingList *list) Again, what does "extra" mean? Probably guest-specific, no? > +{ > +#ifdef TARGET_X86_64 > + target_phys_addr_t kernel_base = -1; > + target_ulong base_vaddr; > + bool lma = !!(first_cpu->hflags & HF_LMA_MASK); > + > + if (!lma) { > + return 0; > + } > + > + kernel_base = get_phys_base_addr(first_cpu, &base_vaddr); > + if (kernel_base == -1) { > + return -1; > + } > + > + create_new_memory_mapping_head(list, kernel_base, base_vaddr, > + TARGET_PAGE_SIZE); > +#endif > + return 0; > +} Jan
At 02/15/2012 01:35 AM, Jan Kiszka Wrote: > On 2012-02-09 04:24, Wen Congyang wrote: >> Crash needs extra memory mapping to determine phys_base. >> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> --- >> cpu-all.h | 2 ++ >> target-i386/arch-dump.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 45 insertions(+), 0 deletions(-) >> >> diff --git a/cpu-all.h b/cpu-all.h >> index efb5ba3..290c43a 100644 >> --- a/cpu-all.h >> +++ b/cpu-all.h >> @@ -530,10 +530,12 @@ int cpu_write_elf64_note(int fd, CPUState *env, int cpuid, >> target_phys_addr_t *offset); >> int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >> target_phys_addr_t *offset); >> +int cpu_add_extra_memory_mapping(MemoryMappingList *list); >> #else >> #define cpu_get_memory_mapping(list, env) >> #define cpu_write_elf64_note(fd, env, cpuid, offset) ({ -1; }) >> #define cpu_write_elf32_note(fd, env, cpuid, offset) ({ -1; }) >> +#define cpu_add_extra_memory_mapping(list) ({ 0; }) >> #endif >> >> #endif /* CPU_ALL_H */ >> diff --git a/target-i386/arch-dump.c b/target-i386/arch-dump.c >> index 4c0ff77..d96f6ae 100644 >> --- a/target-i386/arch-dump.c >> +++ b/target-i386/arch-dump.c >> @@ -495,3 +495,46 @@ int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >> { >> return x86_write_elf32_note(fd, env, cpuid, offset); >> } >> + >> +/* This function is copied from crash */ > > And what does it do there and here? I suppose it is Linux-specific - any > version? This should be documented and encoded in the function name. > >> +static target_ulong get_phys_base_addr(CPUState *env, target_ulong *base_vaddr) >> +{ >> + int i; >> + target_ulong kernel_base = -1; >> + target_ulong last, mask; >> + >> + for (i = 30, last = -1; (kernel_base == -1) && (i >= 20); i--) { >> + mask = ~((1LL << i) - 1); >> + *base_vaddr = env->idt.base & mask; >> + if (*base_vaddr == last) { >> + continue; >> + } >> + >> + kernel_base = cpu_get_phys_page_debug(env, *base_vaddr); >> + last = *base_vaddr; >> + } >> + >> + return kernel_base; >> +} >> + >> +int cpu_add_extra_memory_mapping(MemoryMappingList *list) > > Again, what does "extra" mean? Probably guest-specific, no? crash will calculate the phys_base according to the virtual address and physical address stored in the PT_LOAD. If the vmcore is generated by 'virsh dump'(use migration to implement dumping), crash calculates the phys_base according to idt.base. The function get_phys_base_addr() uses the same way to calculates the phys_base. I think crash may work without this. I will verify it. Thanks Wen Congyang > >> +{ >> +#ifdef TARGET_X86_64 >> + target_phys_addr_t kernel_base = -1; >> + target_ulong base_vaddr; >> + bool lma = !!(first_cpu->hflags & HF_LMA_MASK); >> + >> + if (!lma) { >> + return 0; >> + } >> + >> + kernel_base = get_phys_base_addr(first_cpu, &base_vaddr); >> + if (kernel_base == -1) { >> + return -1; >> + } >> + >> + create_new_memory_mapping_head(list, kernel_base, base_vaddr, >> + TARGET_PAGE_SIZE); >> +#endif >> + return 0; >> +} > > Jan >
On 2012-02-15 06:19, Wen Congyang wrote: > At 02/15/2012 01:35 AM, Jan Kiszka Wrote: >> On 2012-02-09 04:24, Wen Congyang wrote: >>> Crash needs extra memory mapping to determine phys_base. >>> >>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>> --- >>> cpu-all.h | 2 ++ >>> target-i386/arch-dump.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 45 insertions(+), 0 deletions(-) >>> >>> diff --git a/cpu-all.h b/cpu-all.h >>> index efb5ba3..290c43a 100644 >>> --- a/cpu-all.h >>> +++ b/cpu-all.h >>> @@ -530,10 +530,12 @@ int cpu_write_elf64_note(int fd, CPUState *env, int cpuid, >>> target_phys_addr_t *offset); >>> int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>> target_phys_addr_t *offset); >>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list); >>> #else >>> #define cpu_get_memory_mapping(list, env) >>> #define cpu_write_elf64_note(fd, env, cpuid, offset) ({ -1; }) >>> #define cpu_write_elf32_note(fd, env, cpuid, offset) ({ -1; }) >>> +#define cpu_add_extra_memory_mapping(list) ({ 0; }) >>> #endif >>> >>> #endif /* CPU_ALL_H */ >>> diff --git a/target-i386/arch-dump.c b/target-i386/arch-dump.c >>> index 4c0ff77..d96f6ae 100644 >>> --- a/target-i386/arch-dump.c >>> +++ b/target-i386/arch-dump.c >>> @@ -495,3 +495,46 @@ int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>> { >>> return x86_write_elf32_note(fd, env, cpuid, offset); >>> } >>> + >>> +/* This function is copied from crash */ >> >> And what does it do there and here? I suppose it is Linux-specific - any >> version? This should be documented and encoded in the function name. >> >>> +static target_ulong get_phys_base_addr(CPUState *env, target_ulong *base_vaddr) >>> +{ >>> + int i; >>> + target_ulong kernel_base = -1; >>> + target_ulong last, mask; >>> + >>> + for (i = 30, last = -1; (kernel_base == -1) && (i >= 20); i--) { >>> + mask = ~((1LL << i) - 1); >>> + *base_vaddr = env->idt.base & mask; >>> + if (*base_vaddr == last) { >>> + continue; >>> + } >>> + >>> + kernel_base = cpu_get_phys_page_debug(env, *base_vaddr); >>> + last = *base_vaddr; >>> + } >>> + >>> + return kernel_base; >>> +} >>> + >>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list) >> >> Again, what does "extra" mean? Probably guest-specific, no? > > crash will calculate the phys_base according to the virtual address and physical > address stored in the PT_LOAD. Crash is a Linux-only tool, dump must not be restricted to that guest - but could contain transparent extensions of the file format if needed. > > If the vmcore is generated by 'virsh dump'(use migration to implement dumping), > crash calculates the phys_base according to idt.base. The function get_phys_base_addr() > uses the same way to calculates the phys_base. Hmm, where are those special registers (idt, gdt, tr etc.) stored in the vmcore file, BTW? > > I think crash may work without this. I will verify it. Does gdb require this? Jan
At 02/15/2012 05:21 PM, Jan Kiszka Wrote: > On 2012-02-15 06:19, Wen Congyang wrote: >> At 02/15/2012 01:35 AM, Jan Kiszka Wrote: >>> On 2012-02-09 04:24, Wen Congyang wrote: >>>> Crash needs extra memory mapping to determine phys_base. >>>> >>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>>> --- >>>> cpu-all.h | 2 ++ >>>> target-i386/arch-dump.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 45 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/cpu-all.h b/cpu-all.h >>>> index efb5ba3..290c43a 100644 >>>> --- a/cpu-all.h >>>> +++ b/cpu-all.h >>>> @@ -530,10 +530,12 @@ int cpu_write_elf64_note(int fd, CPUState *env, int cpuid, >>>> target_phys_addr_t *offset); >>>> int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>>> target_phys_addr_t *offset); >>>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list); >>>> #else >>>> #define cpu_get_memory_mapping(list, env) >>>> #define cpu_write_elf64_note(fd, env, cpuid, offset) ({ -1; }) >>>> #define cpu_write_elf32_note(fd, env, cpuid, offset) ({ -1; }) >>>> +#define cpu_add_extra_memory_mapping(list) ({ 0; }) >>>> #endif >>>> >>>> #endif /* CPU_ALL_H */ >>>> diff --git a/target-i386/arch-dump.c b/target-i386/arch-dump.c >>>> index 4c0ff77..d96f6ae 100644 >>>> --- a/target-i386/arch-dump.c >>>> +++ b/target-i386/arch-dump.c >>>> @@ -495,3 +495,46 @@ int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>>> { >>>> return x86_write_elf32_note(fd, env, cpuid, offset); >>>> } >>>> + >>>> +/* This function is copied from crash */ >>> >>> And what does it do there and here? I suppose it is Linux-specific - any >>> version? This should be documented and encoded in the function name. >>> >>>> +static target_ulong get_phys_base_addr(CPUState *env, target_ulong *base_vaddr) >>>> +{ >>>> + int i; >>>> + target_ulong kernel_base = -1; >>>> + target_ulong last, mask; >>>> + >>>> + for (i = 30, last = -1; (kernel_base == -1) && (i >= 20); i--) { >>>> + mask = ~((1LL << i) - 1); >>>> + *base_vaddr = env->idt.base & mask; >>>> + if (*base_vaddr == last) { >>>> + continue; >>>> + } >>>> + >>>> + kernel_base = cpu_get_phys_page_debug(env, *base_vaddr); >>>> + last = *base_vaddr; >>>> + } >>>> + >>>> + return kernel_base; >>>> +} >>>> + >>>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list) >>> >>> Again, what does "extra" mean? Probably guest-specific, no? >> >> crash will calculate the phys_base according to the virtual address and physical >> address stored in the PT_LOAD. > > Crash is a Linux-only tool, dump must not be restricted to that guest - > but could contain transparent extensions of the file format if needed. > >> >> If the vmcore is generated by 'virsh dump'(use migration to implement dumping), >> crash calculates the phys_base according to idt.base. The function get_phys_base_addr() >> uses the same way to calculates the phys_base. > > Hmm, where are those special registers (idt, gdt, tr etc.) stored in the > vmcore file, BTW? 'virsh dump' uses mirgation to implement dumping now. So the vmcore has all registers. > >> >> I think crash may work without this. I will verify it. I want to modify crash to make it work without this. I am discussing it with Dave Anderson in crash community now. > > Does gdb require this? IIRC, the answer is no. Thanks Wen Congyang > > Jan >
On 2012-02-15 10:44, Wen Congyang wrote: > At 02/15/2012 05:21 PM, Jan Kiszka Wrote: >> On 2012-02-15 06:19, Wen Congyang wrote: >>> At 02/15/2012 01:35 AM, Jan Kiszka Wrote: >>>> On 2012-02-09 04:24, Wen Congyang wrote: >>>>> Crash needs extra memory mapping to determine phys_base. >>>>> >>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>>>> --- >>>>> cpu-all.h | 2 ++ >>>>> target-i386/arch-dump.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 45 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/cpu-all.h b/cpu-all.h >>>>> index efb5ba3..290c43a 100644 >>>>> --- a/cpu-all.h >>>>> +++ b/cpu-all.h >>>>> @@ -530,10 +530,12 @@ int cpu_write_elf64_note(int fd, CPUState *env, int cpuid, >>>>> target_phys_addr_t *offset); >>>>> int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>>>> target_phys_addr_t *offset); >>>>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list); >>>>> #else >>>>> #define cpu_get_memory_mapping(list, env) >>>>> #define cpu_write_elf64_note(fd, env, cpuid, offset) ({ -1; }) >>>>> #define cpu_write_elf32_note(fd, env, cpuid, offset) ({ -1; }) >>>>> +#define cpu_add_extra_memory_mapping(list) ({ 0; }) >>>>> #endif >>>>> >>>>> #endif /* CPU_ALL_H */ >>>>> diff --git a/target-i386/arch-dump.c b/target-i386/arch-dump.c >>>>> index 4c0ff77..d96f6ae 100644 >>>>> --- a/target-i386/arch-dump.c >>>>> +++ b/target-i386/arch-dump.c >>>>> @@ -495,3 +495,46 @@ int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>>>> { >>>>> return x86_write_elf32_note(fd, env, cpuid, offset); >>>>> } >>>>> + >>>>> +/* This function is copied from crash */ >>>> >>>> And what does it do there and here? I suppose it is Linux-specific - any >>>> version? This should be documented and encoded in the function name. >>>> >>>>> +static target_ulong get_phys_base_addr(CPUState *env, target_ulong *base_vaddr) >>>>> +{ >>>>> + int i; >>>>> + target_ulong kernel_base = -1; >>>>> + target_ulong last, mask; >>>>> + >>>>> + for (i = 30, last = -1; (kernel_base == -1) && (i >= 20); i--) { >>>>> + mask = ~((1LL << i) - 1); >>>>> + *base_vaddr = env->idt.base & mask; >>>>> + if (*base_vaddr == last) { >>>>> + continue; >>>>> + } >>>>> + >>>>> + kernel_base = cpu_get_phys_page_debug(env, *base_vaddr); >>>>> + last = *base_vaddr; >>>>> + } >>>>> + >>>>> + return kernel_base; >>>>> +} >>>>> + >>>>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list) >>>> >>>> Again, what does "extra" mean? Probably guest-specific, no? >>> >>> crash will calculate the phys_base according to the virtual address and physical >>> address stored in the PT_LOAD. >> >> Crash is a Linux-only tool, dump must not be restricted to that guest - >> but could contain transparent extensions of the file format if needed. >> >>> >>> If the vmcore is generated by 'virsh dump'(use migration to implement dumping), >>> crash calculates the phys_base according to idt.base. The function get_phys_base_addr() >>> uses the same way to calculates the phys_base. >> >> Hmm, where are those special registers (idt, gdt, tr etc.) stored in the >> vmcore file, BTW? > > 'virsh dump' uses mirgation to implement dumping now. So the vmcore has all > registers. This is about the new format. And there we are lacking those special registers. At some point, gdb will understand and need them to do proper system-level debugging. I don't know the format structure here: can we add sections to the core file in a way that consumers that don't know them simply ignore them? Jan
At 02/15/2012 06:21 PM, Jan Kiszka Wrote: > On 2012-02-15 10:44, Wen Congyang wrote: >> At 02/15/2012 05:21 PM, Jan Kiszka Wrote: >>> On 2012-02-15 06:19, Wen Congyang wrote: >>>> At 02/15/2012 01:35 AM, Jan Kiszka Wrote: >>>>> On 2012-02-09 04:24, Wen Congyang wrote: >>>>>> Crash needs extra memory mapping to determine phys_base. >>>>>> >>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>>>>> --- >>>>>> cpu-all.h | 2 ++ >>>>>> target-i386/arch-dump.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 45 insertions(+), 0 deletions(-) >>>>>> >>>>>> diff --git a/cpu-all.h b/cpu-all.h >>>>>> index efb5ba3..290c43a 100644 >>>>>> --- a/cpu-all.h >>>>>> +++ b/cpu-all.h >>>>>> @@ -530,10 +530,12 @@ int cpu_write_elf64_note(int fd, CPUState *env, int cpuid, >>>>>> target_phys_addr_t *offset); >>>>>> int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>>>>> target_phys_addr_t *offset); >>>>>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list); >>>>>> #else >>>>>> #define cpu_get_memory_mapping(list, env) >>>>>> #define cpu_write_elf64_note(fd, env, cpuid, offset) ({ -1; }) >>>>>> #define cpu_write_elf32_note(fd, env, cpuid, offset) ({ -1; }) >>>>>> +#define cpu_add_extra_memory_mapping(list) ({ 0; }) >>>>>> #endif >>>>>> >>>>>> #endif /* CPU_ALL_H */ >>>>>> diff --git a/target-i386/arch-dump.c b/target-i386/arch-dump.c >>>>>> index 4c0ff77..d96f6ae 100644 >>>>>> --- a/target-i386/arch-dump.c >>>>>> +++ b/target-i386/arch-dump.c >>>>>> @@ -495,3 +495,46 @@ int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>>>>> { >>>>>> return x86_write_elf32_note(fd, env, cpuid, offset); >>>>>> } >>>>>> + >>>>>> +/* This function is copied from crash */ >>>>> >>>>> And what does it do there and here? I suppose it is Linux-specific - any >>>>> version? This should be documented and encoded in the function name. >>>>> >>>>>> +static target_ulong get_phys_base_addr(CPUState *env, target_ulong *base_vaddr) >>>>>> +{ >>>>>> + int i; >>>>>> + target_ulong kernel_base = -1; >>>>>> + target_ulong last, mask; >>>>>> + >>>>>> + for (i = 30, last = -1; (kernel_base == -1) && (i >= 20); i--) { >>>>>> + mask = ~((1LL << i) - 1); >>>>>> + *base_vaddr = env->idt.base & mask; >>>>>> + if (*base_vaddr == last) { >>>>>> + continue; >>>>>> + } >>>>>> + >>>>>> + kernel_base = cpu_get_phys_page_debug(env, *base_vaddr); >>>>>> + last = *base_vaddr; >>>>>> + } >>>>>> + >>>>>> + return kernel_base; >>>>>> +} >>>>>> + >>>>>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list) >>>>> >>>>> Again, what does "extra" mean? Probably guest-specific, no? >>>> >>>> crash will calculate the phys_base according to the virtual address and physical >>>> address stored in the PT_LOAD. >>> >>> Crash is a Linux-only tool, dump must not be restricted to that guest - >>> but could contain transparent extensions of the file format if needed. >>> >>>> >>>> If the vmcore is generated by 'virsh dump'(use migration to implement dumping), >>>> crash calculates the phys_base according to idt.base. The function get_phys_base_addr() >>>> uses the same way to calculates the phys_base. >>> >>> Hmm, where are those special registers (idt, gdt, tr etc.) stored in the >>> vmcore file, BTW? >> >> 'virsh dump' uses mirgation to implement dumping now. So the vmcore has all >> registers. > > This is about the new format. And there we are lacking those special Yes, this file can be processed with crash. gdb cannot process such file. > registers. At some point, gdb will understand and need them to do proper > system-level debugging. I don't know the format structure here: can we > add sections to the core file in a way that consumers that don't know > them simply ignore them? I donot find such section now. If there is such section, I think it is better to store all cpu's register in the core file. I try to let the core file can be processed with crash and gdb. But crash still does not work well sometimes. I think we can add some option to let user choose whether to store memory mapping in the core file. Because crash does not need such mapping. If the p_vaddr in all PT_LOAD segments is 0, crash know the file is generated by qemu dump, and use another way to calculate phys_base. If you agree with it, please ignore this patch. Thanks Wen Congyang > > Jan >
From: Wen Congyang <wency@cn.fujitsu.com> Subject: Re: [RFC][PATCH 07/16 v6] target-i386: Add API to add extra memory mapping Date: Fri, 17 Feb 2012 17:32:56 +0800 > At 02/15/2012 06:21 PM, Jan Kiszka Wrote: >> On 2012-02-15 10:44, Wen Congyang wrote: >>> At 02/15/2012 05:21 PM, Jan Kiszka Wrote: >>>> On 2012-02-15 06:19, Wen Congyang wrote: >>>>> At 02/15/2012 01:35 AM, Jan Kiszka Wrote: >>>>>> On 2012-02-09 04:24, Wen Congyang wrote: >>>>>>> Crash needs extra memory mapping to determine phys_base. >>>>>>> >>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>>>>>> --- >>>>>>> cpu-all.h | 2 ++ >>>>>>> target-i386/arch-dump.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>>>>>> 2 files changed, 45 insertions(+), 0 deletions(-) >>>>>>> >>>>>>> diff --git a/cpu-all.h b/cpu-all.h >>>>>>> index efb5ba3..290c43a 100644 >>>>>>> --- a/cpu-all.h >>>>>>> +++ b/cpu-all.h >>>>>>> @@ -530,10 +530,12 @@ int cpu_write_elf64_note(int fd, CPUState *env, int cpuid, >>>>>>> target_phys_addr_t *offset); >>>>>>> int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>>>>>> target_phys_addr_t *offset); >>>>>>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list); >>>>>>> #else >>>>>>> #define cpu_get_memory_mapping(list, env) >>>>>>> #define cpu_write_elf64_note(fd, env, cpuid, offset) ({ -1; }) >>>>>>> #define cpu_write_elf32_note(fd, env, cpuid, offset) ({ -1; }) >>>>>>> +#define cpu_add_extra_memory_mapping(list) ({ 0; }) >>>>>>> #endif >>>>>>> >>>>>>> #endif /* CPU_ALL_H */ >>>>>>> diff --git a/target-i386/arch-dump.c b/target-i386/arch-dump.c >>>>>>> index 4c0ff77..d96f6ae 100644 >>>>>>> --- a/target-i386/arch-dump.c >>>>>>> +++ b/target-i386/arch-dump.c >>>>>>> @@ -495,3 +495,46 @@ int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, >>>>>>> { >>>>>>> return x86_write_elf32_note(fd, env, cpuid, offset); >>>>>>> } >>>>>>> + >>>>>>> +/* This function is copied from crash */ >>>>>> >>>>>> And what does it do there and here? I suppose it is Linux-specific - any >>>>>> version? This should be documented and encoded in the function name. >>>>>> >>>>>>> +static target_ulong get_phys_base_addr(CPUState *env, target_ulong *base_vaddr) >>>>>>> +{ >>>>>>> + int i; >>>>>>> + target_ulong kernel_base = -1; >>>>>>> + target_ulong last, mask; >>>>>>> + >>>>>>> + for (i = 30, last = -1; (kernel_base == -1) && (i >= 20); i--) { >>>>>>> + mask = ~((1LL << i) - 1); >>>>>>> + *base_vaddr = env->idt.base & mask; >>>>>>> + if (*base_vaddr == last) { >>>>>>> + continue; >>>>>>> + } >>>>>>> + >>>>>>> + kernel_base = cpu_get_phys_page_debug(env, *base_vaddr); >>>>>>> + last = *base_vaddr; >>>>>>> + } >>>>>>> + >>>>>>> + return kernel_base; >>>>>>> +} >>>>>>> + >>>>>>> +int cpu_add_extra_memory_mapping(MemoryMappingList *list) >>>>>> >>>>>> Again, what does "extra" mean? Probably guest-specific, no? >>>>> >>>>> crash will calculate the phys_base according to the virtual address and physical >>>>> address stored in the PT_LOAD. >>>> >>>> Crash is a Linux-only tool, dump must not be restricted to that guest - >>>> but could contain transparent extensions of the file format if needed. >>>> >>>>> >>>>> If the vmcore is generated by 'virsh dump'(use migration to implement dumping), >>>>> crash calculates the phys_base according to idt.base. The function get_phys_base_addr() >>>>> uses the same way to calculates the phys_base. >>>> >>>> Hmm, where are those special registers (idt, gdt, tr etc.) stored in the >>>> vmcore file, BTW? >>> >>> 'virsh dump' uses mirgation to implement dumping now. So the vmcore has all >>> registers. >> >> This is about the new format. And there we are lacking those special > > Yes, this file can be processed with crash. gdb cannot process such file. > >> registers. At some point, gdb will understand and need them to do proper >> system-level debugging. I don't know the format structure here: can we >> add sections to the core file in a way that consumers that don't know >> them simply ignore them? > > I donot find such section now. If there is such section, I think it is > better to store all cpu's register in the core file. > > I try to let the core file can be processed with crash and gdb. But crash > still does not work well sometimes. > > I think we can add some option to let user choose whether to store memory > mapping in the core file. Because crash does not need such mapping. If > the p_vaddr in all PT_LOAD segments is 0, crash know the file is generated > by qemu dump, and use another way to calculate phys_base. > If you store cpu registers in the core file, checking if the information is contained in the core file is better. Thanks. HATAYAMA, Daisuke > If you agree with it, please ignore this patch. > > Thanks > Wen Congyang > >> >> Jan >> > >
diff --git a/cpu-all.h b/cpu-all.h index efb5ba3..290c43a 100644 --- a/cpu-all.h +++ b/cpu-all.h @@ -530,10 +530,12 @@ int cpu_write_elf64_note(int fd, CPUState *env, int cpuid, target_phys_addr_t *offset); int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, target_phys_addr_t *offset); +int cpu_add_extra_memory_mapping(MemoryMappingList *list); #else #define cpu_get_memory_mapping(list, env) #define cpu_write_elf64_note(fd, env, cpuid, offset) ({ -1; }) #define cpu_write_elf32_note(fd, env, cpuid, offset) ({ -1; }) +#define cpu_add_extra_memory_mapping(list) ({ 0; }) #endif #endif /* CPU_ALL_H */ diff --git a/target-i386/arch-dump.c b/target-i386/arch-dump.c index 4c0ff77..d96f6ae 100644 --- a/target-i386/arch-dump.c +++ b/target-i386/arch-dump.c @@ -495,3 +495,46 @@ int cpu_write_elf32_note(int fd, CPUState *env, int cpuid, { return x86_write_elf32_note(fd, env, cpuid, offset); } + +/* This function is copied from crash */ +static target_ulong get_phys_base_addr(CPUState *env, target_ulong *base_vaddr) +{ + int i; + target_ulong kernel_base = -1; + target_ulong last, mask; + + for (i = 30, last = -1; (kernel_base == -1) && (i >= 20); i--) { + mask = ~((1LL << i) - 1); + *base_vaddr = env->idt.base & mask; + if (*base_vaddr == last) { + continue; + } + + kernel_base = cpu_get_phys_page_debug(env, *base_vaddr); + last = *base_vaddr; + } + + return kernel_base; +} + +int cpu_add_extra_memory_mapping(MemoryMappingList *list) +{ +#ifdef TARGET_X86_64 + target_phys_addr_t kernel_base = -1; + target_ulong base_vaddr; + bool lma = !!(first_cpu->hflags & HF_LMA_MASK); + + if (!lma) { + return 0; + } + + kernel_base = get_phys_base_addr(first_cpu, &base_vaddr); + if (kernel_base == -1) { + return -1; + } + + create_new_memory_mapping_head(list, kernel_base, base_vaddr, + TARGET_PAGE_SIZE); +#endif + return 0; +}
Crash needs extra memory mapping to determine phys_base. Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> --- cpu-all.h | 2 ++ target-i386/arch-dump.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 0 deletions(-)