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

["Hao, Xudong" <xudong.hao@xxxxxxxxx>]

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: Thursday, June 28, 2012 9:42 PM
> To: Hao, Xudong
> Cc: xen-devel@xxxxxxxxxxxxx; Shan, Haitao; keir@xxxxxxx; Zhang, Xiantao;
> JBeulich@xxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2 3/4] xen: introduce new function
> update_dirty_bitmap
> 
> 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.
> 

Thanks for the review, it's a code mistake indeed.
It should be: 
      p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
-                          p2m_ram_logdirty, p2m_ram_rw);
+                          p2m_ram_logdirty, p2mt);


> > --- 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?

Because it's need to add a mask to indentify D bit or not, I do not want to 
change current ept_change_entry_type_global() interface. So add a new function 
pointer.

>  - If you do need a new function pointer, this name is very confusing:
>    it's not doing any kind of query.
> 

The new function query dirty page in whole p2m table and update the dirty 
bitmap. 

> > --- 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?
> 

Oh, it does 2 passes over the p2m table. I wanted to differentiate the CLEAN 
and PEEK op by variable "clean" and "peek", seems peek=1 even when it's a CLEAN 
hapercall.
I do the following modification: 
+    if ( (sc->op == XEN_DOMCTL_SHADOW_OP_PEEK) && 
d->arch.paging.log_dirty.update_dirty_bitmap)
+    {
+        d->arch.paging.log_dirty.update_dirty_bitmap(d);
+    }

> 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

We need to clean D bit in clean() op for the EPT/D bit case. How about the 
modification above?
 
> with the addition of this update_dirty_bitmap() call, the p2m only gets
> walked once, for all configurations.
> 
> Cheers,
> 
> Tim.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel