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.
>> 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?
> 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.
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
