From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v4 06/13] nEPT: Add EPT tables support to paging_tmpl.h Date: Mon, 29 Jul 2013 15:19:01 +0200 Message-ID: <51F66BC5.6030002@redhat.com> References: <1374750001-28527-1-git-send-email-gleb@redhat.com> <1374750001-28527-7-git-send-email-gleb@redhat.com> <51F63A56.4080900@redhat.com> <20130729113354.GF18009@redhat.com> <51F65842.30103@redhat.com> <20130729122417.GH18009@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Xiao Guangrong , Jun Nakajima , Yang Zhang To: Gleb Natapov Return-path: Received: from mail-we0-f170.google.com ([74.125.82.170]:62372 "EHLO mail-we0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756056Ab3G2NTV (ORCPT ); Mon, 29 Jul 2013 09:19:21 -0400 Received: by mail-we0-f170.google.com with SMTP id w60so4015172wes.1 for ; Mon, 29 Jul 2013 06:19:20 -0700 (PDT) In-Reply-To: <20130729122417.GH18009@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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