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

[Anthony Liguori]

On 03/16/2010 08:57 AM, Avi Kivity wrote:
> 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?

Yes, and is what tlb_protect_code() does and it's called from 
tb_alloc_page() which is what's code when a TB is created.

Regards,

Anthony Liguori