Re: [PATCH v4 06/13] nEPT: Add EPT tables support to paging_tmpl.h

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jul 29, 2013 at 04:15:16PM +0200, Paolo Bonzini wrote:
> Il 29/07/2013 15:27, Gleb Natapov ha scritto:
> >>>> doesn't look bad at all.  With the old check on EPT it looked ugly, but
> >>>> with the new check on PT_GUEST_DIRTY_MASK it is quite natural.  Also
> >>>> because you have anyway a reference to PT_GUEST_DIRTY_MASK in the "if".
> >>>>  If I see
> >>>>
> >>>>         if (!write_fault)
> >>>>                 protect_clean_gpte(&pte_access, pte);
> >>>>         else
> >>>>                 /*
> >>>>                  * On a write fault, fold the dirty bit into
> >>>> 		 * accessed_dirty by
> >>>>                  * shifting it one place right.
> >>>>                  */
> >>>>                 accessed_dirty &=
> >>>> 			pte >> (PT_DIRTY_SHIFT - PT_ACCESSED_SHIFT);
> >>>>
> >>>>         if (PT_GUEST_DIRTY_MASK != 0 && unlikely(!accessed_dirty)) {
> >>>>
> >>>> the obvious reaction is "what, is there a case where I'm using
> >>>> accessed_dirty if PT_GUEST_DIRTY_MASK == 0?"  Of course it makes sense
> >>> In this case accessed_dirty has correct value of 0 :) The if() bellow just
> >>> tells you that since A/D is not supported there is nothing to be done
> >>> about zero value of accessed_dirty, but the value itself is correct!
> >>
> >> It is correct because accessed_dirty is initialized to 0.  But the "&"
> >> with a bit taken out of thin air (bit 0 of the PTE)?  That's just
> >> disgusting. :)
> >>
> > Sorry to disgust you, but the code relies on this "&" trick with or
> > without the patch. It clears all unrelated bits from pte this way. No
> > new disgusting tricks are added by the patch.
> 
> Oh the code is not disgusting at all!  It is very nice to follow.
> 
> The new disgusting ;) trick is that here in the EPT case you're
> effectively doing
> 
> 	accessed_dirty &= pte;
> 
> where bit 0 is the "R" bit (iirc) and has absolutely nothing to do with
> dirty or accessed.
> 
What bit 0 has to do with anything? Non ept code after shift also has
random bits and random places in ept (R at P place, U at R place), the
trick is that accessed_dirty masks bits we are not interesting in and
capture only those we want to follow (accessed in regular case, non in
ept case). This is exactly what original code is doing, so they are
either both disgusting or both very nice to follow :)

--
			Gleb.