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 06:28:37PM +0200, Paolo Bonzini wrote:
> Il 29/07/2013 18:14, Gleb Natapov ha scritto:
> >>>>>> > >>>>                 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 :)
> 
> The comment is clear: "fold the dirty bit into accessed_dirty by
> shifting it one place right".  In the EPT case the comment makes no
> sense and it is not obvious that you rely on accessed_dirty=0 even
> before that line.
It is not obvious that the code relies on accessed_dirty been initialized
to the bits the code wants to track at the start of the function. It
wasn't for me. With if() it would have been much clearer, but the
current way is faster. 

> 
> That's why I'd rather have that code out of the PT_GUEST_DIRTY_MASK==0 case.
> 
What problem current code has that you are trying to fix? What _technical_
justification you provide? There is no point adding ifdefs where they
are clearly not needed just because.

--
			Gleb.