linux-kernel - Re: [PATCH 1/2] KVM: SVM: don't check have_run_cpus in sev_writeback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aJpe6GM_3edwJXuX@google.com>
Date: Mon, 11 Aug 2025 14:21:44 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Yury Norov <yury.norov@...il.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Zheyun Shen <szy0127@...u.edu.cn>
Subject: Re: [PATCH 1/2] KVM: SVM: don't check have_run_cpus in sev_writeback_caches()

On Mon, Aug 11, 2025, Yury Norov wrote:
> On Mon, Aug 11, 2025 at 01:50:15PM -0700, Sean Christopherson wrote:
> > On Mon, Aug 11, 2025, Yury Norov wrote:
> > > From: Yury Norov (NVIDIA) <yury.norov@...il.com>
> > > 
> > > Before calling wbnoinvd_on_cpus_mask(), the function checks the cpumask
> > > for emptiness. It's useless, as the following wbnoinvd_on_cpus_mask()
> > > ends up with smp_call_function_many_cond(), which handles empty cpumask
> > > correctly.
> > 
> > I don't agree that it's useless.  The early check avoids disabling/enabling
> > preemption (which is cheap, but still), and IMO it makes the KVM code more obviously
> > correct.  E.g. it takes quite a bit of digging to understand that invoking
> > wbnoinvd_on_cpus_mask() with an empty mask is ok/fine.
> > 
> > I'm not completely opposed to this change, but I also don't see the point.
> 
> So, there's a tradeoff between useless preemption cycling, which is
> O(1) and cpumask_empty(), which is O(N).

Oh, that argument I buy.  I had it in my head that the check is going to be O(1)
in practice, because never running vCPU0 would be all kinds of bizarre.  But the
mask tracks physical CPUs, not virtual CPUs.  E.g. a 2-vCPU VM that's pinned to
the last 2 pCPUs in the system could indeed trigger several superfluous loads and
checks.

> I have no measurements that can support one vs another. But the
> original patch doesn't discuss it anyhow, as well. So, with the
> lack of any information on performance impact, I'd stick with the 
> version that brings less code.
> 
> Agree?

Not sure I agree that less code is always better, but I do agree that dropping
the check makes sense.  :-)

How about this?  No need for a v2 (unless you disagree on the tweaks), I'll happily
fixup when applying.

--
From: Yury Norov <yury.norov@...il.com>
Date: Mon, 11 Aug 2025 16:30:39 -0400
Subject: [PATCH] KVM: SEV: don't check have_run_cpus in sev_writeback_caches()

Drop KVM's check on an empty cpumask when flushing caches when memory is
being reclaimed from an SEV VM, as smp_call_function_many_cond() naturally
(and correctly) handles and empty cpumask.  This avoids an extra O(n)
lookup in the common case where at least one pCPU has enterred the guest,
which could be noticeable in some setups, e.g. if a small VM is pinned to
the last few pCPUs in the system.

Fixes: 6f38f8c57464 ("KVM: SVM: Flush cache only on CPUs running SEV guest")
Signed-off-by: Yury Norov (NVIDIA) <yury.norov@...il.com>
[sean: rewrite changelog to capture performance angle]
Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 arch/x86/kvm/svm/sev.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 2fbdebf79fbb..0635bd71c10e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -718,13 +718,6 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
 
 static void sev_writeback_caches(struct kvm *kvm)
 {
-	/*
-	 * Note, the caller is responsible for ensuring correctness if the mask
-	 * can be modified, e.g. if a CPU could be doing VMRUN.
-	 */
-	if (cpumask_empty(to_kvm_sev_info(kvm)->have_run_cpus))
-		return;
-
 	/*
 	 * Ensure that all dirty guest tagged cache entries are written back
 	 * before releasing the pages back to the system for use.  CLFLUSH will
@@ -739,6 +732,9 @@ static void sev_writeback_caches(struct kvm *kvm)
 	 * serializing multiple calls and having responding CPUs (to the IPI)
 	 * mark themselves as still running if they are running (or about to
 	 * run) a vCPU for the VM.
+	 *
+	 * Note, the caller is responsible for ensuring correctness if the mask
+	 * can be modified, e.g. if a CPU could be doing VMRUN.
 	 */
 	wbnoinvd_on_cpus_mask(to_kvm_sev_info(kvm)->have_run_cpus);
 }

base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
--