Re: [Qemu-devel] [PATCH 11/11 v10] introduce a new monitor command 'dump-guest-memory' to dump guest's memory

[HATAYAMA Daisuke]

From: Wen Congyang <address@hidden>
Subject: Re: [PATCH 11/11 v10] introduce a new monitor command 
'dump-guest-memory' to dump guest's memory
Date: Mon, 26 Mar 2012 09:39:24 +0800

> At 03/23/2012 05:40 PM, HATAYAMA Daisuke Wrote:
>> From: Wen Congyang <address@hidden>
>> Subject: [PATCH 11/11 v10] introduce a new monitor command 
>> 'dump-guest-memory' to dump guest's memory
>> Date: Tue, 20 Mar 2012 11:57:43 +0800
>> 
>>> +/* get the memory's offset in the vmcore */
>>> +static target_phys_addr_t get_offset(target_phys_addr_t phys_addr,
>>> +                                     DumpState *s)
>>> +{
>>> +    RAMBlock *block;
>>> +    target_phys_addr_t offset = s->memory_offset;
>>> +    int64_t size_in_block, start;
>>> +
>>> +    if (s->has_filter) {
>>> +        if (phys_addr < s->begin || phys_addr >= s->begin + s->length) {
>>> +            return -1;
>>> +        }
>>> +    }
>>> +
>>> +    QLIST_FOREACH(block, &ram_list.blocks, next) {
>>> +        if (s->has_filter) {
>>> +            if (block->offset >= s->begin + s->length ||
>>> +                block->offset + block->length <= s->begin) {
>>> +                /* This block is out of the range */
>>> +                continue;
>>> +            }
>>> +
>>> +            if (s->begin <= block->offset) {
>>> +                start = block->offset;
>>> +            } else {
>>> +                start = s->begin;
>>> +            }
>>> +
>>> +            size_in_block = block->length - (start - block->offset);
>>> +            if (s->begin + s->length < block->offset + block->length) {
>>> +                size_in_block -= block->offset + block->length -
>>> +                                 (s->begin + s->length);
>>> +            }
>>> +        } else {
>>> +            start = block->offset;
>>> +            size_in_block = block->length;
>>> +        }
>>> +
>>> +        if (phys_addr >= start && phys_addr < start + size_in_block) {
>>> +            return phys_addr - start + offset;
>>> +        }
>>> +
>>> +        offset += size_in_block;
>>> +    }
>>> +
>>> +    return -1;
>>> +}
>> 
>> OK. -1 is assigned to offset when the corresponding memory is filtered
>> and so not contained in dumpfile. But please give -1 a name. I want
>> the name to tell me the data is filterred.
>> 
>> Also, I couldn't imagine get_offset() does filtering processing. This
>> is important for the purspective of dump because there's data not
>> saved in dumpfile. Could you claify this in some way? By moving
>> filtering processing to the outside, or splitting it into anotehr
>> funciton.
> 
> offset should not be -1. I remove the PT_LOAD in the function 
> memory_mapping_filter().
> In the previous patchset, Luiz said he wants to squash the filtering
> feature to this patch.
> 

I didn't noticed that the patch was moved... _I_ think it should be
presented in a single patch.

Looking at memory_mapping_filter(), PT_LOAD is configured so that
memory that is filtered doesn't exist. I think it better to keep such
mapping with p_filesz == 0 to indicate to users the mapping is
filtered.

Also, get_offset() loops on ram_list.blocks each time it is
called. Because dump_completed() loops memory_mapping list, offset can
be calculated incrementally.

Thanks.
HATAYAMA, Daisuke