Received-SPF: pass (google.com: domain of linux-kernel+bounces-92711-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 5 Mar 2024 08:49:35 -0800
In-Reply-To: <722904540.5000784.1709650623262.JavaMail.zimbra@sjtu.edu.cn>
Precedence: bulk
Mime-Version: 1.0
References: <722904540.5000784.1709650623262.JavaMail.zimbra@sjtu.edu.cn>
Message-ID: <ZedNH2DFpsg5RvHC@google.com>
Subject: Re: [PATCH] KVM:SVM: Flush cache only on CPUs running SEV guest
From: Sean Christopherson <seanjc@google.com>
To: Zheyun Shen <szy0127@sjtu.edu.cn>
Cc: pbonzini <pbonzini@redhat.com>, tglx <tglx@linutronix.de>, 
	thomas lendacky <thomas.lendacky@amd.com>, kvm <kvm@vger.kernel.org>, 
	linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"

On Tue, Mar 05, 2024, Zheyun Shen wrote:
> On Mon, Mar 04, 2024, Sean Christopherson wrote:
> > Instead of copy+paste WBINVD+cpumask_clear() everywhere, add a prep patch to
> > replace relevant open coded calls to wbinvd_on_all_cpus() with calls to
> > sev_guest_memory_reclaimed().  Then only sev_guest_memory_reclaimed() needs to
> > updated, and IMO it helps document why KVM is blasting WBINVD.
> 
> > I'm also pretty sure this should be a cpumask_var_t, and dynamically allocated
> > as appropriate.  And at that point, it should be allocated and filled if and only
> > if the CPU doesn't have X86_FEATURE_SME_COHERENT
> 
> I notice that several callers of wbinvd_on_all_cpus() must use wbinvd to flush cache
> instead of using clflush or just doing nothing if the CPU has X86_FEATURE_SME_COHERENT,
> according to https://github.com/AMDESE/linux/commit/2e2409afe5f0c284c7dfe5504058e8d115806a7d
> Therefore, I think the flush operation should be divided into two functions. One is the 
> optimized wbinvd, which does not consider X86_FEATURE_SME_COHERENT, and the other is 
> sev_guest_memory_reclaimed(), which should use clflush instead of wbinvd in case of 
> X86_FEATURE_SME_COHERENT. Thus the cpumask struct should be exist whether the CPU has
> X86_FEATURE_SME_COHERENT or not.

FWIW, the usage of sev_flush_asids() isn't tied to a single VM, i.e. KVM can't use
per-VM tracking in that case.  But...

> Besides, if we consider X86_FEATURE_SME_COHERENT to get rid of wbinvd in sev_guest_memory_reclaimed(),
> we should ensure the clflush is called on corresponding addresses, as mentioned in  
> https://github.com/AMDESE/linux/commit/d45829b351ee6ec5f54dd55e6aca1f44fe239fe6 
> However, caller of sev_guest_memory_reclaimed() (e.g., kvm_mmu_notifier_invalidate_range_start()) 
> only get HVA belongs to userspace(e.g., qemu), so calling clflush with this HVA may 
> lead to a page fault in kernel. I was wondering if notifying userspace applications to 
> do clflush themselves is the only solution here. But for the sake of safety, maybe KVM 
> cannot left the work for untrusted userspace applications?

Ugh, right, I forgot the whole mess with userspace virtual addresses.  Bummer.

> Or should I just temporarily ignore the X86_FEATURE_SME_COHERENT scenario
> which is hard to implement, and just refine the patch only for
> wbinvd_on_all_cpus() ?

Ignore X86_FEATURE_SME_COHERENT and just refine the patch to optimize WBINVDs that
are tied to a specific VM.  I simply forgot that KVM only uses CLFLUSHOPT when
purging VMSA pages.