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

[Avi Kivity <avi@redhat.com>]

On 03/16/2010 03:51 PM, Anthony Liguori wrote:
> 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.

So the hardware equivalent is, when the Instruction TLB loads a page 
address, clear CODE_DIRTY_FLAG?

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

Ok.

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

Thanks.

-- 
error compiling committee.c: too many arguments to function