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: <aJpbLX_0WP5jXn7o@yury>
Date: Mon, 11 Aug 2025 17:05:49 -0400
From: Yury Norov <yury.norov@...il.com>
To: Sean Christopherson <seanjc@...gle.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 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).

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?