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

[Wen Congyang]

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.

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