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

[Avi Kivity]

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