Re: [Xen-devel] [PATCH v2 4/4] xen: enable EPT dirty bit for guest live migration

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

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: Thursday, June 28, 2012 9:52 PM
> To: Hao, Xudong
> Cc: xen-devel@xxxxxxxxxxxxx; Shan, Haitao; keir@xxxxxxx; Zhang, Xiantao;
> JBeulich@xxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2 4/4] xen: enable EPT dirty bit for guest 
> live
> migration
> 
> At 09:57 +0800 on 20 Jun (1340186267), Xudong Hao wrote:
> > @@ -69,6 +70,11 @@ static void ept_p2m_type_to_flags(ept_entry_t
> *entry, p2m_type_t type, p2m_acces
> >
> entry->mfn);
> >              break;
> >          case p2m_ram_logdirty:
> > +            entry->w = hap_has_dirty_bit;
> > +            entry->r = entry->x = 1;
> > +            /* Not necessarily need to clear A bit, but it is safe anyway 
> > */
> > +            entry->a = entry->d = 0;
> > +            break;
> 
> I think it's better not to clear the A bit - it will just cause the
> hardware to issue extra writes to set it.
> 

It's ok for me to not clear A bit here.

> > @@ -760,15 +770,33 @@ static void ept_change_entry_type_page(mfn_t
> ept_page_mfn, int ept_page_level,
> >
> >          if ( (ept_page_level > 0) && !is_epte_superpage(epte + i) )
> >              ept_change_entry_type_page(_mfn(epte[i].mfn),
> > -                                       ept_page_level - 1, ot, nt);
> > +                                       ept_page_level - 1, ot, nt, d,
> mask);
> >          else
> >          {
> >              e = atomic_read_ept_entry(&epte[i]);
> >              if ( e.sa_p2mt != ot )
> >                  continue;
> >
> > +            if ( e.d && (e.sa_p2mt == p2m_ram_logdirty) )
> > +            {
> > +                int j, nr_pages;
> > +                struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +                for ( j = 0, nr_pages = 1; j < ept_page_level;
> > +                      j++, nr_pages *= 512 ) {}
> > +                for ( j = 0; j < nr_pages; j++ )
> > +                    paging_mark_dirty(d, e.mfn + j);
> > +
> > +                /* split super page to 4k page, so that dirty bitmap can
> > +                 * map the dirty page
> > +                 */
> > +                if ( !ept_split_super_page(p2m, &e, ept_page_level, 0) )
> > +                    continue;
> 
> AIUI this code splits log-dirty superpages _after_ they are first seen
> to be dirty.  So the first time they're dirtied, the entire superpage is
> marked, but OTOH if they're never dirtied you don't need to split.  Is
> that right?  

Right, it did need to split superpage which aren't dirtyied.

> If so, I think it needs a comment explaining it.  It took

I will add comment in code.

> me a few minutes to understand this code and convince myself it was
> safe. :)
> 
> > +                atomic_write_ept_entry(&epte[i], e);
> > +            }
> >              e.sa_p2mt = nt;
> >              ept_p2m_type_to_flags(&e, nt, e.access);
> > +            if (!mask)
> > +                e.a = e.d = 0;
> 
> Again, probably best to leave the A-bit set.
> 

Will remove A bit clearing here.

> Cheers,
> 
> Tim.

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