[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-devel] Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper func

From: Anthony Liguori
Subject: [Qemu-devel] Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper functions to access phys_ram_dirty.
Date: Tue, 16 Mar 2010 08:51:30 -0500
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20091209 Fedora/3.0-4.fc12 Lightning/1.0pre Thunderbird/3.0

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
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)
+ if (phys_ram_migration_dirty[index]& mask)
+ 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];
    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.


Anthony Liguori

reply via email to

[Prev in Thread] Current Thread [Next in Thread]