Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper functions to access phys_ram_dirty.

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/16/2010 08:29 AM, Avi Kivity wrote:
> On 03/16/2010 03:17 PM, Yoshiaki Tamura wrote:
>> Avi Kivity wrote:
>>> On 03/16/2010 12:53 PM, Yoshiaki Tamura wrote:
>>>> Modifies wrapper functions for byte-based phys_ram_dirty bitmap to
>>>> bit-based phys_ram_dirty bitmap, and adds more wrapper functions to
>>>> prevent
>>>> direct access to the phys_ram_dirty bitmap.
>>>
>>>> +
>>>> +static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t 
>>>> addr)
>>>> +{
>>>> + unsigned long mask;
>>>> + int index = (addr>> TARGET_PAGE_BITS) / HOST_LONG_BITS;
>>>> + int offset = (addr>> TARGET_PAGE_BITS)& (HOST_LONG_BITS - 1);
>>>> + int ret = 0;
>>>> +
>>>> + mask = 1UL<< offset;
>>>> + if (phys_ram_vga_dirty[index]& mask)
>>>> + ret |= VGA_DIRTY_FLAG;
>>>> + if (phys_ram_code_dirty[index]& mask)
>>>> + ret |= CODE_DIRTY_FLAG;
>>>> + if (phys_ram_migration_dirty[index]& mask)
>>>> + ret |= MIGRATION_DIRTY_FLAG;
>>>> +
>>>> + return ret;
>>>> }
>>>>
>>>> static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
>>>> int dirty_flags)
>>>> {
>>>> - return phys_ram_dirty[addr>> TARGET_PAGE_BITS]& dirty_flags;
>>>> + return cpu_physical_memory_get_dirty_flags(addr)& dirty_flags;
>>>> }
>>>
>>> This turns one cacheline access into three. If the dirty bitmaps 
>>> were in
>>> an array, you could do
>>>
>>> return dirty_bitmaps[dirty_index][addr >> (TARGET_PAGE_BITS +
>>> BITS_IN_LONG)] & mask;
>>>
>>> with one cacheline access.
>>
>> If I'm understanding the existing code correctly,
>> int dirty_flags can be combined, like VGA + MIGRATION.
>> If we only have to worry about a single dirty flag, I agree with your 
>> idea.
>
> From a quick grep it seems flags are not combined, except for 
> something strange with CODE_DIRTY_FLAG:
>
>> static void notdirty_mem_writel(void *opaque, target_phys_addr_t 
>> ram_addr,
>>                                 uint32_t val)
>> {
>>     int dirty_flags;
>>     dirty_flags = phys_ram_dirty[ram_addr >> TARGET_PAGE_BITS];
>>     if (!(dirty_flags & CODE_DIRTY_FLAG)) {
>> #if !defined(CONFIG_USER_ONLY)
>>         tb_invalidate_phys_page_fast(ram_addr, 4);
>>         dirty_flags = phys_ram_dirty[ram_addr >> TARGET_PAGE_BITS];
>> #endif
>>     }
>>     stl_p(qemu_get_ram_ptr(ram_addr), val);
>>     dirty_flags |= (0xff & ~CODE_DIRTY_FLAG);
>>     phys_ram_dirty[ram_addr >> TARGET_PAGE_BITS] = dirty_flags;
>>     /* we remove the notdirty callback only if the code has been
>>        flushed */
>>     if (dirty_flags == 0xff)
>>         tlb_set_dirty(cpu_single_env, cpu_single_env->mem_io_vaddr);
>> }
>
> I can't say I understand what it does.

The semantics of CODE_DIRTY_FLAG are a little counter intuitive.  
CODE_DIRTY_FLAG means that we know that something isn't code so writes 
do not need checking for self modifying code.

notdirty_mem_write() is called for any ram that is in the virtual TLB 
that has not been updated yet and once a write has occurred, we can 
switch to faster access functions (provided we've invalidated any 
translation blocks).

That's why the check is if (!(dirty_flags & CODE_DIRTY_FLAG)), if it 
hasn't been set yet, we have to assume that it could be a TB so we need 
to invalidate it.  tb_invalidate_phys_page_fast() will set the 
CODE_DIRTY_FLAG if no code is present in that memory area which is why 
we fetch dirty_flags again.

We do the store, and then set the dirty bits to mark that the page is 
now dirty taking care to not change the CODE_DIRTY_FLAG bit.

At the very end, we check to see if CODE_DIRTY_FLAG which indicates that 
we no longer need to trap writes.  If so, we call tlb_set_dirty() which 
will ultimately remove the notdirty callback in favor of a faster access 
mechanism.

With respect patch series, there should be no problem having a separate 
code bitmap that gets updated along with a main bitmap provided that the 
semantics of CODE_DIRTY_FLAG are preserved.

>> Sounds good to me.
>> So we're going to introduce 4 (VGA, CODE, MIGRATION, master) 
>> bit-based bitmaps in total.
>>
>
> Yeah, except CODE doesn't behave like the others.  Would be best to 
> understand what it's requirements are before making the change.  Maybe 
> CODE will need separate handling (so master will only feed VGA and 
> MIGRATION).

Generally speaking, cpu_physical_memory_set_dirty() is called by the 
device model.  Any writes by the device model that results in 
self-modifying code are not going to have predictable semantics which is 
why it can set CODE_DIRTY_FLAG.

CODE_DIRTY_FLAG doesn't need to get updated from a master bitmap.  It 
should be treated as a separate bitmap that is strictly dealt with by 
the virtual TLB.

Regards,

Anthony Liguori