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

[Paolo Bonzini <pbonzini@redhat.com>]

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