Patchwork [RFC,07/16,v6] target-i386: Add API to add extra memory mapping

login
register
mail settings
Submitter Wen Congyang
Date Feb. 9, 2012, 3:24 a.m.
Message ID <4F333C82.4070003@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/140297/
State New
Headers show

Comments

Wen Congyang - Feb. 9, 2012, 3:24 a.m.
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(-)
Jan Kiszka - Feb. 14, 2012, 5:35 p.m.
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
Wen Congyang - Feb. 15, 2012, 5:19 a.m.
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
>
Jan Kiszka - Feb. 15, 2012, 9:21 a.m.
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
Wen Congyang - Feb. 15, 2012, 9:44 a.m.
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
>
Jan Kiszka - Feb. 15, 2012, 10:21 a.m.
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
Wen Congyang - Feb. 17, 2012, 9:32 a.m.
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
>
HATAYAMA Daisuke - Feb. 17, 2012, 11:34 a.m.
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
>> 
> 
>

Patch

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;
+}