On 03/25/2016 09:56 PM, Paolo Bonzini wrote:
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++;
Yes, exactly. [ actually, we can keep mmu_pages_clear_parents() unchanged ]
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)...
The sentinel is NULL forever so it can not go to the inner loop anyway... Okay, i am not strong opinion on it, it is not a big deal. Let's happily drop it if you really dislike it. :)
