Messages in this thread | | | Date | Tue, 16 Apr 2024 23:47:06 -0700 | From | Isaku Yamahata <> | Subject | Re: [PATCH v19 059/130] KVM: x86/tdp_mmu: Don't zap private pages for unsupported cases |
| |
On Wed, Apr 17, 2024 at 10:21:16AM +0800, Chao Gao <chao.gao@intel.com> wrote:
> On Mon, Feb 26, 2024 at 12:26:01AM -0800, isaku.yamahata@intel.com wrote: > >@@ -779,6 +780,10 @@ static bool tdp_mmu_zap_leafs(struct kvm *kvm, struct kvm_mmu_page *root, > > > > lockdep_assert_held_write(&kvm->mmu_lock); > > > >+ WARN_ON_ONCE(zap_private && !is_private_sp(root)); > >+ if (!zap_private && is_private_sp(root)) > >+ return false; > > Should be "return flush;". > > Fengwei and I spent one week chasing a bug where virtio-net in the TD guest may > stop working at some point after bootup if the host enables numad. We finally > found that the bug was introduced by the 'return false' statement, which left > some stale EPT entries unflushed.
Thank you for chasing it down.
> I am wondering if we can refactor related functions slightly to make it harder > to make such mistakes and make it easier to identify them. e.g., we could make > "@flush" an in/out parameter of tdp_mmu_zap_leafs(), kvm_tdp_mmu_zap_leafs() > and kvm_tdp_mmu_unmap_gfn_range(). It looks more apparent that "*flush = false" > below could be problematic if the changes were something like: > > if (!zap_private && is_private_sp(root)) { > *flush = false; > return; > }
Yes, let me look into it. -- Isaku Yamahata <isaku.yamahata@intel.com>
| |