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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

That's why I'd rather have that code out of the PT_GUEST_DIRTY_MASK==0 case.

Paolo