On Mon, Jul 29, 2013 at 03:19:01PM +0200, Paolo Bonzini wrote:
> 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.
>
It is not hard to imaging without trying :) Will do.
> >>
> >> 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.
>
Those this is unnecessary. That's the point. If #ifdef is unavoidable I
have not problem using it even though I dislike it, but in this case it
is just unnecessary.
> >> 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.
--
Gleb.
--
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
