Re: [PATCH v2 3/4] xen: introduce new function update_dirty_bitmap

[Tim Deegan <tim@xen.org>]

At 09:57 +0800 on 20 Jun (1340186266), Xudong Hao wrote:
> @@ -83,19 +84,31 @@ static int hap_enable_vram_tracking(struct domain *d)
>  static int hap_disable_vram_tracking(struct domain *d)
>  {
>      struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
> +    p2m_type_t p2mt = p2m_ram_rw;
>  
>      if ( !dirty_vram )
>          return -EINVAL;
>  
> +    /* With hap dirty bit, p2m type cannot be changed from p2m_ram_logdirty
> +     * to p2m_ram_rw when first fault is met. Actually, there is no such
> +     * fault occurred.
> +     */
> +    if ( hap_has_dirty_bit )
> +        p2mt = p2m_ram_logdirty;
> +
>      paging_lock(d);
>      d->arch.paging.mode &= ~PG_log_dirty;
>      paging_unlock(d);
>  
>      /* set l1e entries of P2M table with normal mode */
>      p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
> -                          p2m_ram_logdirty, p2m_ram_rw);
> +                          p2mt, p2m_ram_rw);

What's the intent of this change?  

AFAICS it will break the hap_has_dirty_bit==0 case, by basically making
that p2m_change_type_range() into a noop.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -148,6 +148,15 @@ void p2m_change_entry_type_global(struct domain *d,
>      p2m_unlock(p2m);
>  }
>  
> +void p2m_query_entry_global(struct domain *d, int mask)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    ASSERT(p2m);
> +    p2m_lock(p2m);
> +    p2m->query_entry_global(p2m, mask);
> +    p2m_unlock(p2m);
> +}
> +

Given that when you implement this, it's basically just the same as
p2m_change_type_global() with p2m_ram_logdirty for both types:

 - Why not just use p2m_change_type_global() instead of adding the new
   function pointer?
 - If you do need a new function pointer, this name is very confusing:
   it's not doing any kind of query.

> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -335,9 +335,24 @@ int paging_log_dirty_op(struct domain *d, struct xen_domctl_shadow_op *sc)
>      int i4, i3, i2;
>  
>      domain_pause(d);
> -    paging_lock(d);
>  
>      clean = (sc->op == XEN_DOMCTL_SHADOW_OP_CLEAN);
> +    if ( clean )
> +    {
> +        /* We need to further call clean_dirty_bitmap() functions of specific
> +         * paging modes (shadow or hap).  Safe because the domain is paused.
> +         * And this call must be made before actually transferring the dirty
> +         * bitmap since with HW hap dirty bit support, dirty bitmap is
> +         * produced by hooking on this call. */
> +        d->arch.paging.log_dirty.clean_dirty_bitmap(d);
> +    }

> +    if ( peek && d->arch.paging.log_dirty.update_dirty_bitmap)
> +    {
> +        d->arch.paging.log_dirty.update_dirty_bitmap(d);
> +    }

Doesn't this end up doing _two_ passes over the p2m table?

I think it would be better to leave the clean() op at the end of the
function, and arrange for it to be a noop in the EPT/D-bit case.  Then
with the addition of this update_dirty_bitmap() call, the p2m only gets
walked once, for all configurations.

Cheers,

Tim.