Re: [Qemu-devel] Re: [PATCH v3 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Yoshiaki Tamura]

2010/4/19 Avi Kivity <address@hidden>:
> On 04/19/2010 02:52 PM, Yoshiaki Tamura wrote:
>>
>> Avi Kivity wrote:
>>>
>>> On 04/19/2010 02:31 PM, Yoshiaki Tamura wrote:
>>>>
>>>>>>> typedef struct RAMBlock {
>>>>>>> @@ -2825,10 +2825,16 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>>>>>>> new_block->next = ram_blocks;
>>>>>>> ram_blocks = new_block;
>>>>>>>
>>>>>>> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
>>>>>>> - (last_ram_offset + size)>> TARGET_PAGE_BITS);
>>>>>>> - memset(phys_ram_dirty + (last_ram_offset>> TARGET_PAGE_BITS),
>>>>>>> - 0xff, size>> TARGET_PAGE_BITS);
>>>>>>> + if (BITMAP_SIZE(last_ram_offset + size) !=
>>>>>>> BITMAP_SIZE(last_ram_offset)) {
>>>>>>
>>>>>> This check is unneeded - the code will work fine even if the bitmap
>>>>>> size
>>>>>> doesn't change.
>>>>>
>>>>> OK. I'll remove it.
>>>>
>>>> I have a problem here.
>>>> If I remove this check, glibc reports an error as below.
>>>>
>>>> *** glibc detected *** /usr/local/qemu/bin/qemu-system-x86_64:
>>>> realloc(): invalid pointer: 0x0000000001f0e450 ***
>>>> ======= Backtrace: =========
>>>> /lib64/libc.so.6[0x369fa75a96]
>>>> /lib64/libc.so.6(realloc+0x2a1)[0x369fa7b881]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x437d93]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x4f03f6]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x5b052c]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x5b0d8b]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x41ec2b]
>>>> /lib64/libc.so.6(__libc_start_main+0xfd)[0x369fa1ea2d]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x406479]
>>>> ======= Memory map: ========
>>>>
>>>> I reminded that I put this check to avoid reallocating same size to
>>>> the bitmap.
>>>> qemu goes this routine at start up, and extends last_ram_offset at
>>>> small numbers.
>>>> The error above is reported at the extension phase.
>>>>
>>>
>>> This probably means that an old bitmap pointer leaked somewhere, and we
>>> realloc() it after free? Or perhaps a glibc bug.
>>
>> Original qemu doesn't have a code the frees phys_ram_dirty, and I didn't
>> either.
>> Hmmm.
>
> I meant, after we realloc() something we keep using the old pointer.
>  realloc() is equivalent to free(), after all.
>
> It might also be memory corruption - bits set outside the bitmap and hitting
> glibc malloc metadata.

The latter seems to be the problem.  I was calling memset() too
aggressively when BITMAP_SIZE didn't grow.  I think It should be as
following.

memset((uint8_t *)phys_ram_dirty[i] + BITMAP_SIZE(last_ram_offset), 0xff,
               BITMAP_SIZE(last_ram_offset + size) -
BITMAP_SIZE(last_ram_offset));