From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path Date: Fri, 25 Mar 2016 14:56:39 +0100 Message-ID: <56F54397.9050809@redhat.com> References: <1458911978-19430-1-git-send-email-guangrong.xiao@linux.intel.com> <1458911978-19430-3-git-send-email-guangrong.xiao@linux.intel.com> <56F54103.6020508@redhat.com> <56F541C5.6090904@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: gleb@kernel.org, mtosatti@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org To: Xiao Guangrong Return-path: In-Reply-To: <56F541C5.6090904@linux.intel.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 25/03/2016 14:48, Xiao Guangrong wrote: >>> >> >> This patch and the previous one are basically redoing commit >> 0a47cd85833e ("KVM: MMU: Fix ubsan warnings", 2016-03-04). While you >> find your version easier to understand, I of course find mine easier. >> >> Rather than getting stuck in a ko fight, the solution is to stick with >> the code in KVM and add comments. I'll give it a try... > > If you do not like this one, we can just make the .index is > [PT64_ROOT_LEVEL - 1] and keep the sentinel in .parents[], that little > change and nice code shape. I suppose you'd have something like this then: diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 70e95d097ef1..15e1735a2e3a 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -1980,7 +1980,7 @@ static bool kvm_sync_pages(struct kvm_vcpu *vcpu, gfn_t gfn, struct mmu_page_path { struct kvm_mmu_page *parent[PT64_ROOT_LEVEL]; - unsigned int idx[PT64_ROOT_LEVEL]; + unsigned int idx[PT64_ROOT_LEVEL-1]; }; #define for_each_sp(pvec, sp, parents, i) \ @@ -2037,13 +2037,14 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) { struct kvm_mmu_page *sp; unsigned int level = 0; + unsigned int idx; do { - unsigned int idx = parents->idx[level]; sp = parents->parent[level]; - if (!sp) + if (!sp || WARN_ON(level == PT64_ROOT_LEVEL-1)) return; + idx = parents->idx[level]; WARN_ON(idx == INVALID_INDEX); clear_unsync_child_bit(sp, idx); level++; By making the arrays the same size, the effect of the sentinel seems clearer to me. It doesn't seem worth 4 bytes (and strictly speaking those 4 bytes would be there anyway due to padding)... Paolo