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

Avi Kivity
2010-03-16T13:57:42Z

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