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 07:06:21PM +0200, Paolo Bonzini wrote:
> Il 29/07/2013 18:43, Gleb Natapov ha scritto:
> > 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. 
> 
> Sure it is not obvious.  But relying on the mask being zero is a whole
> different level of non-obviousness.
> 
I disagree.

> >> 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?
> 
> Of course there is no technical justification.  Did I ever say otherwise?
> 
I just want to be absolutely sure that we are bikeshedding here.

> > There is no point adding ifdefs where they
> > are clearly not needed just because.
> 
> If you loathe ifdefs so much, you can of course wrap the whole code
> we're talking about with an if().  That takes an extra level of
> indentation, of course.
> 
And that point of doing it is...?

--
			Gleb.