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