Re: [Qemu-devel] [RFC][PATCH 07/16 v6] target-i386: Add API to add extra memory mapping

[Jan Kiszka]

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 <address@hidden>
>>>>> ---
>>>>>  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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux