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 03:19:01PM +0200, Paolo Bonzini wrote:
> Il 29/07/2013 14:24, Gleb Natapov ha scritto:
> >> My initial impression to this patch was "everything's ready after the
> >> previous patch, you just have to set the mask to 0".  Which is not quite
> >> true.  Maybe you need three patches instead of two.
> >>
> > Or change commit message for patch 5 to make it more clear that it is a
> > preparation patch?
> 
> Or both.  Just give it a try.
> 
It is not hard to imaging without trying :) Will do.

> >>
> >> Something like this:
> >>
> >> +       /* if dirty bit is not supported, no need to track it */
> >> +#if PT_GUEST_DIRTY_MASK == 0
> >>         if (!write_fault)
> >>                  protect_clean_gpte(&pte_access, pte);
> >> ...
> >>         if (unlikely(!accessed_dirty)) {
> >> ...
> >>         }
> >> +#endif
> >>
> > I will have to do the same for update_accessed_dirty_bits(). The problem
> > of idfdefs they spread around.
> 
> Putting update_accessed_dirty_bits() with "#ifdef do we have
> accessed_dirty_bits at all" sounds just fine.
> 
> But if you do not like #ifdefs you can use __maybe_unused and the
> compiler will elide it.
> 
Those this is unnecessary. That's the point. If #ifdef is unavoidable I
have not problem using it even though I dislike it, but in this case it
is just unnecessary.

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

--
			Gleb.