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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

Paolo