From: Wen Congyang <we...@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 <we...@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
>>
>
>
