[PATCH v2 0/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[PATCH v2 0/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning all
existing cache lines with the recycled ASID must be flushed), the
cache invalidation triggered by WBINVD is unnecessary; only the
writeback is needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches.

First, provide helper functions to use WBNOINVD similar to how WBINVD
is invoked. Second, check for WBNOINVD support and execute WBNOINVD if
possible in lieu of WBINVD to avoid cache invalidations.

Changelog
---
v2: rebase to tip @ dffeaed35cef, drop unnecessary Xen changes, reword.
---
Kevin Loughlin (2):
  x86, lib: Add WBNOINVD helper functions
  KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

 arch/x86/include/asm/smp.h           |  7 ++++++
 arch/x86/include/asm/special_insns.h |  7 +++++-
 arch/x86/kvm/svm/sev.c               | 35 +++++++++++++++++-----------
 arch/x86/lib/cache-smp.c             | 12 ++++++++++
 4 files changed, 47 insertions(+), 14 deletions(-)

-- 
2.47.1.688.g23fc6f90ad-goog

[PATCH v2 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

In line with WBINVD usage, add WBONINVD helper functions.

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
---
 arch/x86/include/asm/smp.h           |  7 +++++++
 arch/x86/include/asm/special_insns.h |  7 ++++++-
 arch/x86/lib/cache-smp.c             | 12 ++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ca073f40698f..ecf93a243b83 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -112,6 +112,7 @@ void native_play_dead(void);
 void play_dead_common(void);
 void wbinvd_on_cpu(int cpu);
 int wbinvd_on_all_cpus(void);
+int wbnoinvd_on_all_cpus(void);
 
 void smp_kick_mwait_play_dead(void);
 
@@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void)
 	return 0;
 }
 
+static inline int wbnoinvd_on_all_cpus(void)
+{
+	wbnoinvd();
+	return 0;
+}
+
 static inline struct cpumask *cpu_llc_shared_mask(int cpu)
 {
 	return (struct cpumask *)cpumask_of(0);
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index fab7c8af27a4..3db7bf86f81f 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -117,7 +117,12 @@ static inline void wrpkru(u32 pkru)
 
 static __always_inline void wbinvd(void)
 {
-	asm volatile("wbinvd": : :"memory");
+	asm volatile("wbinvd" : : : "memory");
+}
+
+static __always_inline void wbnoinvd(void)
+{
+	asm volatile("wbnoinvd" : : : "memory");
 }
 
 static inline unsigned long __read_cr4(void)
diff --git a/arch/x86/lib/cache-smp.c b/arch/x86/lib/cache-smp.c
index 7af743bd3b13..7ac5cca53031 100644
--- a/arch/x86/lib/cache-smp.c
+++ b/arch/x86/lib/cache-smp.c
@@ -20,3 +20,15 @@ int wbinvd_on_all_cpus(void)
 	return 0;
 }
 EXPORT_SYMBOL(wbinvd_on_all_cpus);
+
+static void __wbnoinvd(void *dummy)
+{
+	wbnoinvd();
+}
+
+int wbnoinvd_on_all_cpus(void)
+{
+	on_each_cpu(__wbnoinvd, NULL, 1);
+	return 0;
+}
+EXPORT_SYMBOL(wbnoinvd_on_all_cpus);
-- 
2.47.1.688.g23fc6f90ad-goog

[PATCH v2 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning all
existing cache lines with the recycled ASID must be flushed), the
cache invalidation triggered by WBINVD is unnecessary; only the
writeback is needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches.

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
---
 arch/x86/kvm/svm/sev.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index fe6cc763fd51..a413b2299d30 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -116,6 +116,7 @@ static int sev_flush_asids(unsigned int min_asid, unsigned int max_asid)
 	 */
 	down_write(&sev_deactivate_lock);
 
+	/* Use WBINVD for ASID recycling. */
 	wbinvd_on_all_cpus();
 
 	if (sev_snp_enabled)
@@ -710,6 +711,14 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
 	}
 }
 
+static void sev_wb_on_all_cpus(void)
+{
+	if (boot_cpu_has(X86_FEATURE_WBNOINVD))
+		wbnoinvd_on_all_cpus();
+	else
+		wbinvd_on_all_cpus();
+}
+
 static unsigned long get_num_contig_pages(unsigned long idx,
 				struct page **inpages, unsigned long npages)
 {
@@ -2774,11 +2783,11 @@ int sev_mem_enc_unregister_region(struct kvm *kvm,
 	}
 
 	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
+	 * Ensure that all dirty guest tagged cache entries are written back
+	 * before releasing the pages back to the system for use. CLFLUSH will
+	 * not do this without SME_COHERENT, so issue a WB[NO]INVD.
 	 */
-	wbinvd_on_all_cpus();
+	sev_wb_on_all_cpus();
 
 	__unregister_enc_region_locked(kvm, region);
 
@@ -2900,11 +2909,11 @@ void sev_vm_destroy(struct kvm *kvm)
 	}
 
 	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
+	 * Ensure that all dirty guest tagged cache entries are written back
+	 * before releasing the pages back to the system for use. CLFLUSH will
+	 * not do this without SME_COHERENT, so issue a WB[NO]INVD.
 	 */
-	wbinvd_on_all_cpus();
+	sev_wb_on_all_cpus();
 
 	/*
 	 * if userspace was terminated before unregistering the memory regions
@@ -3130,12 +3139,12 @@ static void sev_flush_encrypted_page(struct kvm_vcpu *vcpu, void *va)
 	 * by leaving stale encrypted data in the cache.
 	 */
 	if (WARN_ON_ONCE(wrmsrl_safe(MSR_AMD64_VM_PAGE_FLUSH, addr | asid)))
-		goto do_wbinvd;
+		goto do_wb_on_all_cpus;
 
 	return;
 
-do_wbinvd:
-	wbinvd_on_all_cpus();
+do_wb_on_all_cpus:
+	sev_wb_on_all_cpus();
 }
 
 void sev_guest_memory_reclaimed(struct kvm *kvm)
@@ -3149,7 +3158,7 @@ void sev_guest_memory_reclaimed(struct kvm *kvm)
 	if (!sev_guest(kvm) || sev_snp_guest(kvm))
 		return;
 
-	wbinvd_on_all_cpus();
+	sev_wb_on_all_cpus();
 }
 
 void sev_free_vcpu(struct kvm_vcpu *vcpu)
@@ -3858,7 +3867,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 		 * guest-mapped page rather than the initial one allocated
 		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
 		 * could be free'd and cleaned up here, but that involves
-		 * cleanups like wbinvd_on_all_cpus() which would ideally
+		 * cleanups like sev_wb_on_all_cpus() which would ideally
 		 * be handled during teardown rather than guest boot.
 		 * Deferring that also allows the existing logic for SEV-ES
 		 * VMSAs to be re-used with minimal SNP-specific changes.
-- 
2.47.1.688.g23fc6f90ad-goog

Re: [PATCH v2 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kirill A. Shutemov]

On Thu, Jan 09, 2025 at 10:55:33PM +0000, Kevin Loughlin wrote:
> @@ -710,6 +711,14 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
>  	}
>  }
>  
> +static void sev_wb_on_all_cpus(void)
> +{
> +	if (boot_cpu_has(X86_FEATURE_WBNOINVD))
> +		wbnoinvd_on_all_cpus();
> +	else
> +		wbinvd_on_all_cpus();

I think the X86_FEATURE_WBNOINVD check should be inside wbnoinvd().
wbnoinvd() should fallback to WBINVD if the instruction is not supported
rather than trigger #UD.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

On Fri, Jan 10, 2025 at 12:23 AM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Thu, Jan 09, 2025 at 10:55:33PM +0000, Kevin Loughlin wrote:
> > @@ -710,6 +711,14 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
> >       }
> >  }
> >
> > +static void sev_wb_on_all_cpus(void)
> > +{
> > +     if (boot_cpu_has(X86_FEATURE_WBNOINVD))
> > +             wbnoinvd_on_all_cpus();
> > +     else
> > +             wbinvd_on_all_cpus();
>
> I think the X86_FEATURE_WBNOINVD check should be inside wbnoinvd().
> wbnoinvd() should fallback to WBINVD if the instruction is not supported
> rather than trigger #UD.

I debated this as well and am open to doing it that way. One argument
against silently falling back to WBINVD within wbnoinvd() (in general,
not referring to this specific case) is that frequent WBINVD can cause
soft lockups, whereas WBNOINVD is much less likely to do so. As such,
there are potentially use cases where falling back to WBINVD would be
undesirable (and would potentially be non-obvious behavior to the
programmer calling wbnoinvd()), hence why I left the decision outside
wbnoinvd().

That said, open to either way, especially since that "potential" use
case doesn't apply here; just lemme know if you still have a strong
preference for doing the check within wbnoinvd().

Re: [PATCH v2 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Mingwei Zhang]

On Thu, Jan 09, 2025, Kevin Loughlin wrote:
> AMD CPUs currently execute WBINVD in the host when unregistering SEV
> guest memory or when deactivating SEV guests. Such cache maintenance is
> performed to prevent data corruption, wherein the encrypted (C=1)
> version of a dirty cache line might otherwise only be written back
> after the memory is written in a different context (ex: C=0), yielding
> corruption. However, WBINVD is performance-costly, especially because
> it invalidates processor caches.
> 
> Strictly-speaking, unless the SEV ASID is being recycled (meaning all
> existing cache lines with the recycled ASID must be flushed), the
> cache invalidation triggered by WBINVD is unnecessary; only the
> writeback is needed to prevent data corruption in remaining scenarios.
> 
> To improve performance in these scenarios, use WBNOINVD when available
> instead of WBINVD. WBNOINVD still writes back all dirty lines
> (preventing host data corruption by SEV guests) but does *not*
> invalidate processor caches.

This looks reasonable to me. I assume when WBNOINVD writes back the
content, those cache lines are no longer "dirty" and any subsequent
WBINVD invoked in other locations (for other reasons) won't write back
these cache lines again. Thus it avoids corruption.

> 
> Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
Reviewed-by: Mingwei Zhang <mizhang@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index fe6cc763fd51..a413b2299d30 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -116,6 +116,7 @@ static int sev_flush_asids(unsigned int min_asid, unsigned int max_asid)
>  	 */
>  	down_write(&sev_deactivate_lock);
>  
> +	/* Use WBINVD for ASID recycling. */
>  	wbinvd_on_all_cpus();

IIUC, using WBINVD is essentially needed by architecture if software
want to successfully recycle ASIDs. This is not related with flushing
pages, but updating the "state" in the Data Fabric.


>  
>  	if (sev_snp_enabled)
> @@ -710,6 +711,14 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
>  	}
>  }
>  
> +static void sev_wb_on_all_cpus(void)
> +{
> +	if (boot_cpu_has(X86_FEATURE_WBNOINVD))
> +		wbnoinvd_on_all_cpus();
> +	else
> +		wbinvd_on_all_cpus();
> +}
> +
>  static unsigned long get_num_contig_pages(unsigned long idx,
>  				struct page **inpages, unsigned long npages)
>  {
> @@ -2774,11 +2783,11 @@ int sev_mem_enc_unregister_region(struct kvm *kvm,
>  	}
>  
>  	/*
> -	 * Ensure that all guest tagged cache entries are flushed before
> -	 * releasing the pages back to the system for use. CLFLUSH will
> -	 * not do this, so issue a WBINVD.
> +	 * Ensure that all dirty guest tagged cache entries are written back
> +	 * before releasing the pages back to the system for use. CLFLUSH will
> +	 * not do this without SME_COHERENT, so issue a WB[NO]INVD.
>  	 */
> -	wbinvd_on_all_cpus();
> +	sev_wb_on_all_cpus();
>  
>  	__unregister_enc_region_locked(kvm, region);
>  
> @@ -2900,11 +2909,11 @@ void sev_vm_destroy(struct kvm *kvm)
>  	}
>  
>  	/*
> -	 * Ensure that all guest tagged cache entries are flushed before
> -	 * releasing the pages back to the system for use. CLFLUSH will
> -	 * not do this, so issue a WBINVD.
> +	 * Ensure that all dirty guest tagged cache entries are written back
> +	 * before releasing the pages back to the system for use. CLFLUSH will
> +	 * not do this without SME_COHERENT, so issue a WB[NO]INVD.
>  	 */
> -	wbinvd_on_all_cpus();
> +	sev_wb_on_all_cpus();
>  
>  	/*
>  	 * if userspace was terminated before unregistering the memory regions
> @@ -3130,12 +3139,12 @@ static void sev_flush_encrypted_page(struct kvm_vcpu *vcpu, void *va)
>  	 * by leaving stale encrypted data in the cache.
>  	 */
>  	if (WARN_ON_ONCE(wrmsrl_safe(MSR_AMD64_VM_PAGE_FLUSH, addr | asid)))
> -		goto do_wbinvd;
> +		goto do_wb_on_all_cpus;
>  
>  	return;
>  
> -do_wbinvd:
> -	wbinvd_on_all_cpus();
> +do_wb_on_all_cpus:
> +	sev_wb_on_all_cpus();
>  }
>  
>  void sev_guest_memory_reclaimed(struct kvm *kvm)
> @@ -3149,7 +3158,7 @@ void sev_guest_memory_reclaimed(struct kvm *kvm)
>  	if (!sev_guest(kvm) || sev_snp_guest(kvm))
>  		return;
>  
> -	wbinvd_on_all_cpus();
> +	sev_wb_on_all_cpus();
>  }
>  
>  void sev_free_vcpu(struct kvm_vcpu *vcpu)
> @@ -3858,7 +3867,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
>  		 * guest-mapped page rather than the initial one allocated
>  		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
>  		 * could be free'd and cleaned up here, but that involves
> -		 * cleanups like wbinvd_on_all_cpus() which would ideally
> +		 * cleanups like sev_wb_on_all_cpus() which would ideally
>  		 * be handled during teardown rather than guest boot.
>  		 * Deferring that also allows the existing logic for SEV-ES
>  		 * VMSAs to be re-used with minimal SNP-specific changes.
> -- 
> 2.47.1.688.g23fc6f90ad-goog
> 

Re: [PATCH v2 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kirill A. Shutemov]

On Mon, Jan 13, 2025 at 10:47:57AM -0800, Kevin Loughlin wrote:
> On Fri, Jan 10, 2025 at 12:23 AM Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >
> > On Thu, Jan 09, 2025 at 10:55:33PM +0000, Kevin Loughlin wrote:
> > > @@ -710,6 +711,14 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
> > >       }
> > >  }
> > >
> > > +static void sev_wb_on_all_cpus(void)
> > > +{
> > > +     if (boot_cpu_has(X86_FEATURE_WBNOINVD))
> > > +             wbnoinvd_on_all_cpus();
> > > +     else
> > > +             wbinvd_on_all_cpus();
> >
> > I think the X86_FEATURE_WBNOINVD check should be inside wbnoinvd().
> > wbnoinvd() should fallback to WBINVD if the instruction is not supported
> > rather than trigger #UD.
> 
> I debated this as well and am open to doing it that way. One argument
> against silently falling back to WBINVD within wbnoinvd() (in general,
> not referring to this specific case) is that frequent WBINVD can cause
> soft lockups, whereas WBNOINVD is much less likely to do so. As such,
> there are potentially use cases where falling back to WBINVD would be
> undesirable (and would potentially be non-obvious behavior to the
> programmer calling wbnoinvd()), hence why I left the decision outside
> wbnoinvd().
> 
> That said, open to either way, especially since that "potential" use
> case doesn't apply here; just lemme know if you still have a strong
> preference for doing the check within wbnoinvd().

An alternative would be to fail wbnoinvd() with an error code and
possibly a WARN(). Crash on #UD is not helpful.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Sean Christopherson]

On Tue, Jan 14, 2025, Kirill A. Shutemov wrote:
> On Mon, Jan 13, 2025 at 10:47:57AM -0800, Kevin Loughlin wrote:
> > On Fri, Jan 10, 2025 at 12:23 AM Kirill A. Shutemov
> > <kirill.shutemov@linux.intel.com> wrote:
> > >
> > > On Thu, Jan 09, 2025 at 10:55:33PM +0000, Kevin Loughlin wrote:
> > > > @@ -710,6 +711,14 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
> > > >       }
> > > >  }
> > > >
> > > > +static void sev_wb_on_all_cpus(void)

In anticipation of landing the targeted flushing optimizations[*] in the near-ish
future, I would prefer to avoid the "on_all_cpus" aspect of the name.

Maybe sev_writeback_caches(), to kinda sorta pair with sev_clflush_pages()?
Definitely open to other names.

[*] https://lore.kernel.org/all/20240411140445.1038319-1-szy0127@sjtu.edu.cn

> > > > +{
> > > > +     if (boot_cpu_has(X86_FEATURE_WBNOINVD))
> > > > +             wbnoinvd_on_all_cpus();
> > > > +     else
> > > > +             wbinvd_on_all_cpus();
> > >
> > > I think the X86_FEATURE_WBNOINVD check should be inside wbnoinvd().
> > > wbnoinvd() should fallback to WBINVD if the instruction is not supported
> > > rather than trigger #UD.
> > 
> > I debated this as well and am open to doing it that way. One argument
> > against silently falling back to WBINVD within wbnoinvd() (in general,
> > not referring to this specific case) is that frequent WBINVD can cause
> > soft lockups, whereas WBNOINVD is much less likely to do so. As such,
> > there are potentially use cases where falling back to WBINVD would be
> > undesirable (and would potentially be non-obvious behavior to the
> > programmer calling wbnoinvd()), hence why I left the decision outside
> > wbnoinvd().

Practically speaking, I highly doubt there will ever be a scenario where not
falling back to WBINVD is a viable option.  The alternatives I see are:

  1. Crash
  2. Data Corruption
  3. CLFLUSH{OPT}

(1) and (2) are laughably bad, and (3) would add significant complexity in most
situations (to enumerate all addresses), and would be outright impossible in some.

And if someone opts for WBNOINVD, they darn well better have done their homework,
because there should be precious few reasons to need to flush all caches, and as
evidenced by lack of usage in the kernel, even fewer reasons to write back dirty
data while preserving entries.  I don't think it's at all unreasonable to put the
onus on future developers to look at the implementation of wbnoinvd() and
understand the implications.

> > That said, open to either way, especially since that "potential" use
> > case doesn't apply here; just lemme know if you still have a strong
> > preference for doing the check within wbnoinvd().
> 
> An alternative would be to fail wbnoinvd() with an error code and
> possibly a WARN(). Crash on #UD is not helpful.

Re: [PATCH v2 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

On Tue, Jan 14, 2025 at 8:13 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Jan 14, 2025, Kirill A. Shutemov wrote:
> > On Mon, Jan 13, 2025 at 10:47:57AM -0800, Kevin Loughlin wrote:
> > > On Fri, Jan 10, 2025 at 12:23 AM Kirill A. Shutemov
> > > <kirill.shutemov@linux.intel.com> wrote:
> > > >
> > > > On Thu, Jan 09, 2025 at 10:55:33PM +0000, Kevin Loughlin wrote:
> > > > > @@ -710,6 +711,14 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
> > > > >       }
> > > > >  }
> > > > >
> > > > > +static void sev_wb_on_all_cpus(void)
>
> In anticipation of landing the targeted flushing optimizations[*] in the near-ish
> future, I would prefer to avoid the "on_all_cpus" aspect of the name.
>
> Maybe sev_writeback_caches(), to kinda sorta pair with sev_clflush_pages()?
> Definitely open to other names.
>
> [*] https://lore.kernel.org/all/20240411140445.1038319-1-szy0127@sjtu.edu.cn

Ack, will do.

> > > > > +{
> > > > > +     if (boot_cpu_has(X86_FEATURE_WBNOINVD))
> > > > > +             wbnoinvd_on_all_cpus();
> > > > > +     else
> > > > > +             wbinvd_on_all_cpus();
> > > >
> > > > I think the X86_FEATURE_WBNOINVD check should be inside wbnoinvd().
> > > > wbnoinvd() should fallback to WBINVD if the instruction is not supported
> > > > rather than trigger #UD.
> > >
> > > I debated this as well and am open to doing it that way. One argument
> > > against silently falling back to WBINVD within wbnoinvd() (in general,
> > > not referring to this specific case) is that frequent WBINVD can cause
> > > soft lockups, whereas WBNOINVD is much less likely to do so. As such,
> > > there are potentially use cases where falling back to WBINVD would be
> > > undesirable (and would potentially be non-obvious behavior to the
> > > programmer calling wbnoinvd()), hence why I left the decision outside
> > > wbnoinvd().
>
> Practically speaking, I highly doubt there will ever be a scenario where not
> falling back to WBINVD is a viable option.  The alternatives I see are:
>
>   1. Crash
>   2. Data Corruption
>   3. CLFLUSH{OPT}
>
> (1) and (2) are laughably bad, and (3) would add significant complexity in most
> situations (to enumerate all addresses), and would be outright impossible in some.
>
> And if someone opts for WBNOINVD, they darn well better have done their homework,
> because there should be precious few reasons to need to flush all caches, and as
> evidenced by lack of usage in the kernel, even fewer reasons to write back dirty
> data while preserving entries.  I don't think it's at all unreasonable to put the
> onus on future developers to look at the implementation of wbnoinvd() and
> understand the implications.

Yeah, you and Kirill have convinced me that falling back to WBINVD is
the way to go. I'll do that in v3 shortly here; thanks!

[PATCH v3 0/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning all
existing cache lines with the recycled ASID must be flushed), the
cache invalidation triggered by WBINVD is unnecessary; only the
writeback is needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches.

First, provide helper functions to use WBNOINVD similar to how WBINVD
is invoked. Second, check for WBNOINVD support and execute WBNOINVD if
possible in lieu of WBINVD to avoid cache invalidations.

Note that I have *not* rebased this series atop proposed targeted
flushing optimizations [0], since the optimizations do not yet appear
to be finalized. However, I'm happy to do a rebase if that would be
helpful.

[0] https://lore.kernel.org/kvm/85frlcvjyo.fsf@amd.com/T/

Changelog
---
v3:
- rebase to tip @ e6609f8bea4a
- use WBINVD in wbnoinvd() if X86_FEATURE_WBNOINVD is not present
- provide sev_writeback_caches() wrapper function in anticipation of
  aforementioned [0] targeted flushing optimizations
- add Reviewed-by from Mingwei
- reword commits/comments
v2:
- rebase to tip @ dffeaed35cef
- drop unnecessary Xen changes
- reword commits/comments
---
Kevin Loughlin (2):
  x86, lib: Add WBNOINVD helper functions
  KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

 arch/x86/include/asm/smp.h           |  7 +++++
 arch/x86/include/asm/special_insns.h |  7 ++++-
 arch/x86/kvm/svm/sev.c               | 41 ++++++++++++++--------------
 arch/x86/lib/cache-smp.c             | 12 ++++++++
 4 files changed, 46 insertions(+), 21 deletions(-)

-- 
2.48.1.262.g85cc9f2d1e-goog

[PATCH v3 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

In line with WBINVD usage, add WBONINVD helper functions. For the
wbnoinvd() helper, fall back to WBINVD if X86_FEATURE_WBNOINVD is not
present.

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
---
 arch/x86/include/asm/smp.h           |  7 +++++++
 arch/x86/include/asm/special_insns.h |  7 ++++++-
 arch/x86/lib/cache-smp.c             | 12 ++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ca073f40698f..ecf93a243b83 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -112,6 +112,7 @@ void native_play_dead(void);
 void play_dead_common(void);
 void wbinvd_on_cpu(int cpu);
 int wbinvd_on_all_cpus(void);
+int wbnoinvd_on_all_cpus(void);
 
 void smp_kick_mwait_play_dead(void);
 
@@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void)
 	return 0;
 }
 
+static inline int wbnoinvd_on_all_cpus(void)
+{
+	wbnoinvd();
+	return 0;
+}
+
 static inline struct cpumask *cpu_llc_shared_mask(int cpu)
 {
 	return (struct cpumask *)cpumask_of(0);
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 03e7c2d49559..bd2eb7430cd6 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -117,7 +117,12 @@ static inline void wrpkru(u32 pkru)
 
 static __always_inline void wbinvd(void)
 {
-	asm volatile("wbinvd": : :"memory");
+	asm volatile("wbinvd" : : : "memory");
+}
+
+static __always_inline void wbnoinvd(void)
+{
+	alternative("wbinvd", "wbnoinvd", X86_FEATURE_WBNOINVD);
 }
 
 static inline unsigned long __read_cr4(void)
diff --git a/arch/x86/lib/cache-smp.c b/arch/x86/lib/cache-smp.c
index 7af743bd3b13..7ac5cca53031 100644
--- a/arch/x86/lib/cache-smp.c
+++ b/arch/x86/lib/cache-smp.c
@@ -20,3 +20,15 @@ int wbinvd_on_all_cpus(void)
 	return 0;
 }
 EXPORT_SYMBOL(wbinvd_on_all_cpus);
+
+static void __wbnoinvd(void *dummy)
+{
+	wbnoinvd();
+}
+
+int wbnoinvd_on_all_cpus(void)
+{
+	on_each_cpu(__wbnoinvd, NULL, 1);
+	return 0;
+}
+EXPORT_SYMBOL(wbnoinvd_on_all_cpus);
-- 
2.48.1.262.g85cc9f2d1e-goog

[PATCH v3 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning the
SNP firmware requires the use of WBINVD prior to DF_FLUSH), the cache
invalidation triggered by WBINVD is unnecessary; only the writeback is
needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches. Note that the implementation of wbnoinvd()
ensures fall back to WBINVD if WBNOINVD is unavailable.

In anticipation of forthcoming optimizations to limit the WBNOINVD only
to physical CPUs that have executed SEV guests, place the call to
wbnoinvd_on_all_cpus() in a wrapper function sev_writeback_caches().

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
Reviewed-by: Mingwei Zhang <mizhang@google.com>
---
 arch/x86/kvm/svm/sev.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index fe6cc763fd51..f10f1c53345e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -116,6 +116,7 @@ static int sev_flush_asids(unsigned int min_asid, unsigned int max_asid)
 	 */
 	down_write(&sev_deactivate_lock);
 
+	/* SNP firmware requires use of WBINVD for ASID recycling. */
 	wbinvd_on_all_cpus();
 
 	if (sev_snp_enabled)
@@ -710,6 +711,16 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
 	}
 }
 
+static inline void sev_writeback_caches(void)
+{
+	/*
+	 * Ensure that all dirty guest tagged cache entries are written back
+	 * before releasing the pages back to the system for use. CLFLUSH will
+	 * not do this without SME_COHERENT, so issue a WBNOINVD.
+	 */
+	wbnoinvd_on_all_cpus();
+}
+
 static unsigned long get_num_contig_pages(unsigned long idx,
 				struct page **inpages, unsigned long npages)
 {
@@ -2773,12 +2784,7 @@ int sev_mem_enc_unregister_region(struct kvm *kvm,
 		goto failed;
 	}
 
-	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
-	 */
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 
 	__unregister_enc_region_locked(kvm, region);
 
@@ -2899,12 +2905,7 @@ void sev_vm_destroy(struct kvm *kvm)
 		return;
 	}
 
-	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
-	 */
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 
 	/*
 	 * if userspace was terminated before unregistering the memory regions
@@ -3126,16 +3127,16 @@ static void sev_flush_encrypted_page(struct kvm_vcpu *vcpu, void *va)
 
 	/*
 	 * VM Page Flush takes a host virtual address and a guest ASID.  Fall
-	 * back to WBINVD if this faults so as not to make any problems worse
+	 * back to WBNOINVD if this faults so as not to make any problems worse
 	 * by leaving stale encrypted data in the cache.
 	 */
 	if (WARN_ON_ONCE(wrmsrl_safe(MSR_AMD64_VM_PAGE_FLUSH, addr | asid)))
-		goto do_wbinvd;
+		goto do_sev_writeback_caches;
 
 	return;
 
-do_wbinvd:
-	wbinvd_on_all_cpus();
+do_sev_writeback_caches:
+	sev_writeback_caches();
 }
 
 void sev_guest_memory_reclaimed(struct kvm *kvm)
@@ -3144,12 +3145,12 @@ void sev_guest_memory_reclaimed(struct kvm *kvm)
 	 * With SNP+gmem, private/encrypted memory is unreachable via the
 	 * hva-based mmu notifiers, so these events are only actually
 	 * pertaining to shared pages where there is no need to perform
-	 * the WBINVD to flush associated caches.
+	 * the WBNOINVD to flush associated caches.
 	 */
 	if (!sev_guest(kvm) || sev_snp_guest(kvm))
 		return;
 
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 }
 
 void sev_free_vcpu(struct kvm_vcpu *vcpu)
@@ -3858,7 +3859,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 		 * guest-mapped page rather than the initial one allocated
 		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
 		 * could be free'd and cleaned up here, but that involves
-		 * cleanups like wbinvd_on_all_cpus() which would ideally
+		 * cleanups like sev_writeback_caches() which would ideally
 		 * be handled during teardown rather than guest boot.
 		 * Deferring that also allows the existing logic for SEV-ES
 		 * VMSAs to be re-used with minimal SNP-specific changes.
@@ -4910,7 +4911,7 @@ void sev_gmem_invalidate(kvm_pfn_t start, kvm_pfn_t end)
 
 		/*
 		 * SEV-ES avoids host/guest cache coherency issues through
-		 * WBINVD hooks issued via MMU notifiers during run-time, and
+		 * WBNOINVD hooks issued via MMU notifiers during run-time, and
 		 * KVM's VM destroy path at shutdown. Those MMU notifier events
 		 * don't cover gmem since there is no requirement to map pages
 		 * to a HVA in order to use them for a running guest. While the
-- 
2.48.1.262.g85cc9f2d1e-goog

Re: [PATCH v3 1/2] x86, lib: Add WBNOINVD helper functions

[Dave Hansen]

On 1/21/25 16:13, Kevin Loughlin wrote:
> +static __always_inline void wbnoinvd(void)
> +{
> +	alternative("wbinvd", "wbnoinvd", X86_FEATURE_WBNOINVD);
>  }

Could we please comment this a _bit_?

/*
 * Cheaper version of wbinvd(). Call when caches
 * need to be written back but not invalidated.
 */
static __always_inline void wbnoinvd(void)
{
	/*
	 * Use the compatible but more destructuve "invalidate"
	 * variant when no-invalidate is unavailable:
	 */
	alternative("wbinvd", "wbnoinvd", X86_FEATURE_WBNOINVD);
}

Sure, folks can read the instruction reference, but it doesn't give you
much of the story of why you should use one over the other or why it's
OK to call one when you ask for the other.

Re: [PATCH v3 1/2] x86, lib: Add WBNOINVD helper functions

[Dave Hansen]

On 1/21/25 16:30, Dave Hansen wrote:
> 	 * Use the compatible but more destructuve "invalidate"

With my stupid spelling mistakes fixed, if possible. ;)

Re: [PATCH v3 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

On Tue, Jan 21, 2025 at 4:32 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 1/21/25 16:13, Kevin Loughlin wrote:
> > +static __always_inline void wbnoinvd(void)
> > +{
> > +     alternative("wbinvd", "wbnoinvd", X86_FEATURE_WBNOINVD);
> >  }
>
> Could we please comment this a _bit_?
>
> /*
>  * Cheaper version of wbinvd(). Call when caches
>  * need to be written back but not invalidated.
>  */
> static __always_inline void wbnoinvd(void)
> {
>         /*
>          * Use the compatible but more destructuve "invalidate"
>          * variant when no-invalidate is unavailable:
>          */
>         alternative("wbinvd", "wbnoinvd", X86_FEATURE_WBNOINVD);
> }
>
> Sure, folks can read the instruction reference, but it doesn't give you
> much of the story of why you should use one over the other or why it's
> OK to call one when you ask for the other.

Yeah, good point. Incoming in v4; thanks!

[PATCH v4 0/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning all
existing cache lines with the recycled ASID must be flushed), the
cache invalidation triggered by WBINVD is unnecessary; only the
writeback is needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches.

First, provide helper functions to use WBNOINVD similar to how WBINVD
is invoked. Second, check for WBNOINVD support and execute WBNOINVD if
possible in lieu of WBINVD to avoid cache invalidations.

Note that I have *not* rebased this series atop proposed targeted
flushing optimizations [0], since the optimizations do not yet appear
to be finalized. However, I'm happy to do a rebase if that would be
helpful.

[0] https://lore.kernel.org/kvm/85frlcvjyo.fsf@amd.com/T/

Changelog
---
v4:
- add comments to wbnoinvd() for clarity on when to use and behavior
v3:
- rebase to tip @ e6609f8bea4a
- use WBINVD in wbnoinvd() if X86_FEATURE_WBNOINVD is not present
- provide sev_writeback_caches() wrapper function in anticipation of
  aforementioned [0] targeted flushing optimizations
- add Reviewed-by from Mingwei
- reword commits/comments
v2:
- rebase to tip @ dffeaed35cef
- drop unnecessary Xen changes
- reword commits/comments
---
Kevin Loughlin (2):
  x86, lib: Add WBNOINVD helper functions
  KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

 arch/x86/include/asm/smp.h           |  7 +++++
 arch/x86/include/asm/special_insns.h | 15 +++++++++-
 arch/x86/kvm/svm/sev.c               | 41 ++++++++++++++--------------
 arch/x86/lib/cache-smp.c             | 12 ++++++++
 4 files changed, 54 insertions(+), 21 deletions(-)

-- 
2.48.1.262.g85cc9f2d1e-goog

[PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

In line with WBINVD usage, add WBONINVD helper functions. For the
wbnoinvd() helper, fall back to WBINVD if X86_FEATURE_WBNOINVD is not
present.

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
---
 arch/x86/include/asm/smp.h           |  7 +++++++
 arch/x86/include/asm/special_insns.h | 15 ++++++++++++++-
 arch/x86/lib/cache-smp.c             | 12 ++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ca073f40698f..ecf93a243b83 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -112,6 +112,7 @@ void native_play_dead(void);
 void play_dead_common(void);
 void wbinvd_on_cpu(int cpu);
 int wbinvd_on_all_cpus(void);
+int wbnoinvd_on_all_cpus(void);
 
 void smp_kick_mwait_play_dead(void);
 
@@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void)
 	return 0;
 }
 
+static inline int wbnoinvd_on_all_cpus(void)
+{
+	wbnoinvd();
+	return 0;
+}
+
 static inline struct cpumask *cpu_llc_shared_mask(int cpu)
 {
 	return (struct cpumask *)cpumask_of(0);
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 03e7c2d49559..94640c3491d7 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -117,7 +117,20 @@ static inline void wrpkru(u32 pkru)
 
 static __always_inline void wbinvd(void)
 {
-	asm volatile("wbinvd": : :"memory");
+	asm volatile("wbinvd" : : : "memory");
+}
+
+/*
+ * Cheaper version of wbinvd(). Call when caches
+ * need to be written back but not invalidated.
+ */
+static __always_inline void wbnoinvd(void)
+{
+	/*
+	 * Use the compatible but more destructive "invalidate"
+	 * variant when no-invalidate is unavailable.
+	 */
+	alternative("wbinvd", "wbnoinvd", X86_FEATURE_WBNOINVD);
 }
 
 static inline unsigned long __read_cr4(void)
diff --git a/arch/x86/lib/cache-smp.c b/arch/x86/lib/cache-smp.c
index 7af743bd3b13..7ac5cca53031 100644
--- a/arch/x86/lib/cache-smp.c
+++ b/arch/x86/lib/cache-smp.c
@@ -20,3 +20,15 @@ int wbinvd_on_all_cpus(void)
 	return 0;
 }
 EXPORT_SYMBOL(wbinvd_on_all_cpus);
+
+static void __wbnoinvd(void *dummy)
+{
+	wbnoinvd();
+}
+
+int wbnoinvd_on_all_cpus(void)
+{
+	on_each_cpu(__wbnoinvd, NULL, 1);
+	return 0;
+}
+EXPORT_SYMBOL(wbnoinvd_on_all_cpus);
-- 
2.48.1.262.g85cc9f2d1e-goog

[PATCH v4 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning the
SNP firmware requires the use of WBINVD prior to DF_FLUSH), the cache
invalidation triggered by WBINVD is unnecessary; only the writeback is
needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches. Note that the implementation of wbnoinvd()
ensures fall back to WBINVD if WBNOINVD is unavailable.

In anticipation of forthcoming optimizations to limit the WBNOINVD only
to physical CPUs that have executed SEV guests, place the call to
wbnoinvd_on_all_cpus() in a wrapper function sev_writeback_caches().

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
Reviewed-by: Mingwei Zhang <mizhang@google.com>
---
 arch/x86/kvm/svm/sev.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index fe6cc763fd51..f10f1c53345e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -116,6 +116,7 @@ static int sev_flush_asids(unsigned int min_asid, unsigned int max_asid)
 	 */
 	down_write(&sev_deactivate_lock);
 
+	/* SNP firmware requires use of WBINVD for ASID recycling. */
 	wbinvd_on_all_cpus();
 
 	if (sev_snp_enabled)
@@ -710,6 +711,16 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
 	}
 }
 
+static inline void sev_writeback_caches(void)
+{
+	/*
+	 * Ensure that all dirty guest tagged cache entries are written back
+	 * before releasing the pages back to the system for use. CLFLUSH will
+	 * not do this without SME_COHERENT, so issue a WBNOINVD.
+	 */
+	wbnoinvd_on_all_cpus();
+}
+
 static unsigned long get_num_contig_pages(unsigned long idx,
 				struct page **inpages, unsigned long npages)
 {
@@ -2773,12 +2784,7 @@ int sev_mem_enc_unregister_region(struct kvm *kvm,
 		goto failed;
 	}
 
-	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
-	 */
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 
 	__unregister_enc_region_locked(kvm, region);
 
@@ -2899,12 +2905,7 @@ void sev_vm_destroy(struct kvm *kvm)
 		return;
 	}
 
-	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
-	 */
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 
 	/*
 	 * if userspace was terminated before unregistering the memory regions
@@ -3126,16 +3127,16 @@ static void sev_flush_encrypted_page(struct kvm_vcpu *vcpu, void *va)
 
 	/*
 	 * VM Page Flush takes a host virtual address and a guest ASID.  Fall
-	 * back to WBINVD if this faults so as not to make any problems worse
+	 * back to WBNOINVD if this faults so as not to make any problems worse
 	 * by leaving stale encrypted data in the cache.
 	 */
 	if (WARN_ON_ONCE(wrmsrl_safe(MSR_AMD64_VM_PAGE_FLUSH, addr | asid)))
-		goto do_wbinvd;
+		goto do_sev_writeback_caches;
 
 	return;
 
-do_wbinvd:
-	wbinvd_on_all_cpus();
+do_sev_writeback_caches:
+	sev_writeback_caches();
 }
 
 void sev_guest_memory_reclaimed(struct kvm *kvm)
@@ -3144,12 +3145,12 @@ void sev_guest_memory_reclaimed(struct kvm *kvm)
 	 * With SNP+gmem, private/encrypted memory is unreachable via the
 	 * hva-based mmu notifiers, so these events are only actually
 	 * pertaining to shared pages where there is no need to perform
-	 * the WBINVD to flush associated caches.
+	 * the WBNOINVD to flush associated caches.
 	 */
 	if (!sev_guest(kvm) || sev_snp_guest(kvm))
 		return;
 
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 }
 
 void sev_free_vcpu(struct kvm_vcpu *vcpu)
@@ -3858,7 +3859,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 		 * guest-mapped page rather than the initial one allocated
 		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
 		 * could be free'd and cleaned up here, but that involves
-		 * cleanups like wbinvd_on_all_cpus() which would ideally
+		 * cleanups like sev_writeback_caches() which would ideally
 		 * be handled during teardown rather than guest boot.
 		 * Deferring that also allows the existing logic for SEV-ES
 		 * VMSAs to be re-used with minimal SNP-specific changes.
@@ -4910,7 +4911,7 @@ void sev_gmem_invalidate(kvm_pfn_t start, kvm_pfn_t end)
 
 		/*
 		 * SEV-ES avoids host/guest cache coherency issues through
-		 * WBINVD hooks issued via MMU notifiers during run-time, and
+		 * WBNOINVD hooks issued via MMU notifiers during run-time, and
 		 * KVM's VM destroy path at shutdown. Those MMU notifier events
 		 * don't cover gmem since there is no requirement to map pages
 		 * to a HVA in order to use them for a running guest. While the
-- 
2.48.1.262.g85cc9f2d1e-goog

Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

[Kirill A. Shutemov]

On Wed, Jan 22, 2025 at 01:34:37AM +0000, Kevin Loughlin wrote:
> In line with WBINVD usage, add WBONINVD helper functions. For the
> wbnoinvd() helper, fall back to WBINVD if X86_FEATURE_WBNOINVD is not
> present.
> 
> Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
> ---
>  arch/x86/include/asm/smp.h           |  7 +++++++
>  arch/x86/include/asm/special_insns.h | 15 ++++++++++++++-
>  arch/x86/lib/cache-smp.c             | 12 ++++++++++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index ca073f40698f..ecf93a243b83 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -112,6 +112,7 @@ void native_play_dead(void);
>  void play_dead_common(void);
>  void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
> +int wbnoinvd_on_all_cpus(void);
>  
>  void smp_kick_mwait_play_dead(void);
>  
> @@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void)
>  	return 0;
>  }
>  
> +static inline int wbnoinvd_on_all_cpus(void)
> +{
> +	wbnoinvd();
> +	return 0;
> +}
> +
>  static inline struct cpumask *cpu_llc_shared_mask(int cpu)
>  {
>  	return (struct cpumask *)cpumask_of(0);
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 03e7c2d49559..94640c3491d7 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -117,7 +117,20 @@ static inline void wrpkru(u32 pkru)
>  
>  static __always_inline void wbinvd(void)
>  {
> -	asm volatile("wbinvd": : :"memory");
> +	asm volatile("wbinvd" : : : "memory");
> +}
> +
> +/*
> + * Cheaper version of wbinvd(). Call when caches
> + * need to be written back but not invalidated.
> + */
> +static __always_inline void wbnoinvd(void)
> +{
> +	/*
> +	 * Use the compatible but more destructive "invalidate"
> +	 * variant when no-invalidate is unavailable.
> +	 */
> +	alternative("wbinvd", "wbnoinvd", X86_FEATURE_WBNOINVD);

The minimal version of binutils kernel supports is 2.25 which doesn't
know about WBNOINVD.

I think you need to do something like.

	alternative("wbinvd", ".byte 0xf3; wbinvd", X86_FEATURE_WBNOINVD);

Or propose to bump minimal binutils version.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

[Tom Lendacky]

On 1/22/25 01:32, Kirill A. Shutemov wrote:
> On Wed, Jan 22, 2025 at 01:34:37AM +0000, Kevin Loughlin wrote:
>> In line with WBINVD usage, add WBONINVD helper functions. For the
>> wbnoinvd() helper, fall back to WBINVD if X86_FEATURE_WBNOINVD is not
>> present.
>>
>> Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
>> ---
>>  arch/x86/include/asm/smp.h           |  7 +++++++
>>  arch/x86/include/asm/special_insns.h | 15 ++++++++++++++-
>>  arch/x86/lib/cache-smp.c             | 12 ++++++++++++
>>  3 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
>> index ca073f40698f..ecf93a243b83 100644
>> --- a/arch/x86/include/asm/smp.h
>> +++ b/arch/x86/include/asm/smp.h
>> @@ -112,6 +112,7 @@ void native_play_dead(void);
>>  void play_dead_common(void);
>>  void wbinvd_on_cpu(int cpu);
>>  int wbinvd_on_all_cpus(void);
>> +int wbnoinvd_on_all_cpus(void);
>>  
>>  void smp_kick_mwait_play_dead(void);
>>  
>> @@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void)
>>  	return 0;
>>  }
>>  
>> +static inline int wbnoinvd_on_all_cpus(void)
>> +{
>> +	wbnoinvd();
>> +	return 0;
>> +}
>> +
>>  static inline struct cpumask *cpu_llc_shared_mask(int cpu)
>>  {
>>  	return (struct cpumask *)cpumask_of(0);
>> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
>> index 03e7c2d49559..94640c3491d7 100644
>> --- a/arch/x86/include/asm/special_insns.h
>> +++ b/arch/x86/include/asm/special_insns.h
>> @@ -117,7 +117,20 @@ static inline void wrpkru(u32 pkru)
>>  
>>  static __always_inline void wbinvd(void)
>>  {
>> -	asm volatile("wbinvd": : :"memory");
>> +	asm volatile("wbinvd" : : : "memory");
>> +}
>> +
>> +/*
>> + * Cheaper version of wbinvd(). Call when caches
>> + * need to be written back but not invalidated.
>> + */
>> +static __always_inline void wbnoinvd(void)
>> +{
>> +	/*
>> +	 * Use the compatible but more destructive "invalidate"
>> +	 * variant when no-invalidate is unavailable.
>> +	 */
>> +	alternative("wbinvd", "wbnoinvd", X86_FEATURE_WBNOINVD);
> 
> The minimal version of binutils kernel supports is 2.25 which doesn't
> know about WBNOINVD.
> 
> I think you need to do something like.
> 
> 	alternative("wbinvd", ".byte 0xf3; wbinvd", X86_FEATURE_WBNOINVD);

I think "rep; wbinvd" would work as well.

Thanks,
Tom

> 
> Or propose to bump minimal binutils version.
> 

Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

[Dave Hansen]

On 1/22/25 11:39, Tom Lendacky wrote:
>> I think you need to do something like.
>>
>> 	alternative("wbinvd", ".byte 0xf3; wbinvd", X86_FEATURE_WBNOINVD);
> I think "rep; wbinvd" would work as well.

I don't want to bike shed this too much.

But, please, no.

The fact that wbnoinvd can be expressed as "rep; wbinvd" is a
implementation detail. Exposing how it's encoded in the ISA is only
going to add confusion. This:

static __always_inline void wbnoinvd(void)
{
        asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
}

is perfectly fine and a billion times less confusing than:

        asm volatile(".byte 0xf3; wbinvd\n\t": : :"memory");
or
        asm volatile("rep; wbinvd\n\t": : :"memory");

It only gets worse if it's mixed in an alternative() with the _actual_
wbinvd.

BTW, I don't think you should be compelled to use alternative() as
opposed to a good old:

	if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
		...

Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

On Wed, Jan 22, 2025 at 3:16 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 1/22/25 11:39, Tom Lendacky wrote:
> >> I think you need to do something like.
> >>
> >>      alternative("wbinvd", ".byte 0xf3; wbinvd", X86_FEATURE_WBNOINVD);
> > I think "rep; wbinvd" would work as well.
>
> I don't want to bike shed this too much.
>
> But, please, no.
>
> The fact that wbnoinvd can be expressed as "rep; wbinvd" is a
> implementation detail. Exposing how it's encoded in the ISA is only
> going to add confusion. This:
>
> static __always_inline void wbnoinvd(void)
> {
>         asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> }
>
> is perfectly fine and a billion times less confusing than:
>
>         asm volatile(".byte 0xf3; wbinvd\n\t": : :"memory");
> or
>         asm volatile("rep; wbinvd\n\t": : :"memory");
>
> It only gets worse if it's mixed in an alternative() with the _actual_
> wbinvd.

Works for me. I will explicitly use 0xf3,0x0f,0x09 in v5, which I will
send shortly.

> BTW, I don't think you should be compelled to use alternative() as
> opposed to a good old:
>
>         if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
>                 ...

Agreed, though I'm leaving as alternative() for now (both because it
results in fewer checks and because that's what is used in the rest of
the file); please holler if you prefer otherwise. If so, my slight
preference in that case would be to update the whole file
stylistically in a separate commit.

[PATCH v5 0/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning all
existing cache lines with the recycled ASID must be flushed), the
cache invalidation triggered by WBINVD is unnecessary; only the
writeback is needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches.

First, provide helper functions to use WBNOINVD similar to how WBINVD
is invoked. Second, check for WBNOINVD support and execute WBNOINVD if
possible in lieu of WBINVD to avoid cache invalidations.

Note that I have *not* rebased this series atop proposed targeted
flushing optimizations [0], since the optimizations do not yet appear
to be finalized. However, I'm happy to do a rebase if that would be
helpful.

[0] https://lore.kernel.org/kvm/85frlcvjyo.fsf@amd.com/T/

Changelog
---
v5:
- explicitly encode wbnoinvd as 0xf3 0x0f 0x09 for binutils backwards compatibility
v4:
- add comments to wbnoinvd() for clarity on when to use and behavior
v3:
- rebase to tip @ e6609f8bea4a
- use WBINVD in wbnoinvd() if X86_FEATURE_WBNOINVD is not present
- provide sev_writeback_caches() wrapper function in anticipation of
  aforementioned [0] targeted flushing optimizations
- add Reviewed-by from Mingwei
- reword commits/comments
v2:
- rebase to tip @ dffeaed35cef
- drop unnecessary Xen changes
- reword commits/comments
---
Kevin Loughlin (2):
  x86, lib: Add WBNOINVD helper functions
  KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

 arch/x86/include/asm/smp.h           |  7 +++++
 arch/x86/include/asm/special_insns.h | 20 +++++++++++++-
 arch/x86/kvm/svm/sev.c               | 41 ++++++++++++++--------------
 arch/x86/lib/cache-smp.c             | 12 ++++++++
 4 files changed, 59 insertions(+), 21 deletions(-)

-- 
2.48.1.262.g85cc9f2d1e-goog

[PATCH v5 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

In line with WBINVD usage, add WBONINVD helper functions. For the
wbnoinvd() helper, fall back to WBINVD if X86_FEATURE_WBNOINVD is not
present.

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
---
 arch/x86/include/asm/smp.h           |  7 +++++++
 arch/x86/include/asm/special_insns.h | 20 +++++++++++++++++++-
 arch/x86/lib/cache-smp.c             | 12 ++++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ca073f40698f..ecf93a243b83 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -112,6 +112,7 @@ void native_play_dead(void);
 void play_dead_common(void);
 void wbinvd_on_cpu(int cpu);
 int wbinvd_on_all_cpus(void);
+int wbnoinvd_on_all_cpus(void);
 
 void smp_kick_mwait_play_dead(void);
 
@@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void)
 	return 0;
 }
 
+static inline int wbnoinvd_on_all_cpus(void)
+{
+	wbnoinvd();
+	return 0;
+}
+
 static inline struct cpumask *cpu_llc_shared_mask(int cpu)
 {
 	return (struct cpumask *)cpumask_of(0);
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 03e7c2d49559..fb3ddb2ddea9 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -117,7 +117,25 @@ static inline void wrpkru(u32 pkru)
 
 static __always_inline void wbinvd(void)
 {
-	asm volatile("wbinvd": : :"memory");
+	asm volatile("wbinvd" : : : "memory");
+}
+
+/*
+ * Cheaper version of wbinvd(). Call when caches
+ * need to be written back but not invalidated.
+ */
+static __always_inline void wbnoinvd(void)
+{
+	/*
+	 * WBNOINVD is encoded as 0xf3 0x0f 0x09. Making this
+	 * encoding explicit ensures compatibility with older versions of
+	 * binutils, which may not know about WBNOINVD.
+	 *
+	 * If WBNOINVD is unavailable, fall back to the compatible but
+	 * more destructive WBINVD (which still writes the caches back
+	 * but also invalidates them).
+	 */
+	alternative("wbinvd", ".byte 0xf3,0x0f,0x09", X86_FEATURE_WBNOINVD);
 }
 
 static inline unsigned long __read_cr4(void)
diff --git a/arch/x86/lib/cache-smp.c b/arch/x86/lib/cache-smp.c
index 7af743bd3b13..7ac5cca53031 100644
--- a/arch/x86/lib/cache-smp.c
+++ b/arch/x86/lib/cache-smp.c
@@ -20,3 +20,15 @@ int wbinvd_on_all_cpus(void)
 	return 0;
 }
 EXPORT_SYMBOL(wbinvd_on_all_cpus);
+
+static void __wbnoinvd(void *dummy)
+{
+	wbnoinvd();
+}
+
+int wbnoinvd_on_all_cpus(void)
+{
+	on_each_cpu(__wbnoinvd, NULL, 1);
+	return 0;
+}
+EXPORT_SYMBOL(wbnoinvd_on_all_cpus);
-- 
2.48.1.262.g85cc9f2d1e-goog

[PATCH v5 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning the
SNP firmware requires the use of WBINVD prior to DF_FLUSH), the cache
invalidation triggered by WBINVD is unnecessary; only the writeback is
needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches. Note that the implementation of wbnoinvd()
ensures fall back to WBINVD if WBNOINVD is unavailable.

In anticipation of forthcoming optimizations to limit the WBNOINVD only
to physical CPUs that have executed SEV guests, place the call to
wbnoinvd_on_all_cpus() in a wrapper function sev_writeback_caches().

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
Reviewed-by: Mingwei Zhang <mizhang@google.com>
---
 arch/x86/kvm/svm/sev.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index fe6cc763fd51..f10f1c53345e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -116,6 +116,7 @@ static int sev_flush_asids(unsigned int min_asid, unsigned int max_asid)
 	 */
 	down_write(&sev_deactivate_lock);
 
+	/* SNP firmware requires use of WBINVD for ASID recycling. */
 	wbinvd_on_all_cpus();
 
 	if (sev_snp_enabled)
@@ -710,6 +711,16 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
 	}
 }
 
+static inline void sev_writeback_caches(void)
+{
+	/*
+	 * Ensure that all dirty guest tagged cache entries are written back
+	 * before releasing the pages back to the system for use. CLFLUSH will
+	 * not do this without SME_COHERENT, so issue a WBNOINVD.
+	 */
+	wbnoinvd_on_all_cpus();
+}
+
 static unsigned long get_num_contig_pages(unsigned long idx,
 				struct page **inpages, unsigned long npages)
 {
@@ -2773,12 +2784,7 @@ int sev_mem_enc_unregister_region(struct kvm *kvm,
 		goto failed;
 	}
 
-	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
-	 */
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 
 	__unregister_enc_region_locked(kvm, region);
 
@@ -2899,12 +2905,7 @@ void sev_vm_destroy(struct kvm *kvm)
 		return;
 	}
 
-	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
-	 */
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 
 	/*
 	 * if userspace was terminated before unregistering the memory regions
@@ -3126,16 +3127,16 @@ static void sev_flush_encrypted_page(struct kvm_vcpu *vcpu, void *va)
 
 	/*
 	 * VM Page Flush takes a host virtual address and a guest ASID.  Fall
-	 * back to WBINVD if this faults so as not to make any problems worse
+	 * back to WBNOINVD if this faults so as not to make any problems worse
 	 * by leaving stale encrypted data in the cache.
 	 */
 	if (WARN_ON_ONCE(wrmsrl_safe(MSR_AMD64_VM_PAGE_FLUSH, addr | asid)))
-		goto do_wbinvd;
+		goto do_sev_writeback_caches;
 
 	return;
 
-do_wbinvd:
-	wbinvd_on_all_cpus();
+do_sev_writeback_caches:
+	sev_writeback_caches();
 }
 
 void sev_guest_memory_reclaimed(struct kvm *kvm)
@@ -3144,12 +3145,12 @@ void sev_guest_memory_reclaimed(struct kvm *kvm)
 	 * With SNP+gmem, private/encrypted memory is unreachable via the
 	 * hva-based mmu notifiers, so these events are only actually
 	 * pertaining to shared pages where there is no need to perform
-	 * the WBINVD to flush associated caches.
+	 * the WBNOINVD to flush associated caches.
 	 */
 	if (!sev_guest(kvm) || sev_snp_guest(kvm))
 		return;
 
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 }
 
 void sev_free_vcpu(struct kvm_vcpu *vcpu)
@@ -3858,7 +3859,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 		 * guest-mapped page rather than the initial one allocated
 		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
 		 * could be free'd and cleaned up here, but that involves
-		 * cleanups like wbinvd_on_all_cpus() which would ideally
+		 * cleanups like sev_writeback_caches() which would ideally
 		 * be handled during teardown rather than guest boot.
 		 * Deferring that also allows the existing logic for SEV-ES
 		 * VMSAs to be re-used with minimal SNP-specific changes.
@@ -4910,7 +4911,7 @@ void sev_gmem_invalidate(kvm_pfn_t start, kvm_pfn_t end)
 
 		/*
 		 * SEV-ES avoids host/guest cache coherency issues through
-		 * WBINVD hooks issued via MMU notifiers during run-time, and
+		 * WBNOINVD hooks issued via MMU notifiers during run-time, and
 		 * KVM's VM destroy path at shutdown. Those MMU notifier events
 		 * don't cover gmem since there is no requirement to map pages
 		 * to a HVA in order to use them for a running guest. While the
-- 
2.48.1.262.g85cc9f2d1e-goog

Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

[Dave Hansen]

On 1/22/25 16:06, Kevin Loughlin wrote:
>> BTW, I don't think you should be compelled to use alternative() as
>> opposed to a good old:
>>
>>         if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
>>                 ...
> Agreed, though I'm leaving as alternative() for now (both because it
> results in fewer checks and because that's what is used in the rest of
> the file); please holler if you prefer otherwise. If so, my slight
> preference in that case would be to update the whole file
> stylistically in a separate commit.

alternative() can make a _lot_ of sense.  It's extremely compact in the
code it generates. It messes with compiler optimization, of course, just
like any assembly. But, overall, it's great.

In this case, though, we don't care one bit about code generation or
performance. We're running the world's slowest instruction from an IPI.

As for consistency, special_insns.h is gloriously inconsistent. But only
two instructions use alternatives, and they *need* the asm syntax
because they're passing registers and meaningful constraints in.

The wbinvds don't get passed registers and their constraints are
trivial. This conditional:

        alternative_io(".byte 0x3e; clflush %0",
                       ".byte 0x66; clflush %0",
                       X86_FEATURE_CLFLUSHOPT,
                       "+m" (*(volatile char __force *)__p));

could be written like this:

	if (cpu_feature_enabled(X86_FEATURE_CLFLUSHOPT))
	        asm volatile(".byte 0x3e; clflush %0",
                       "+m" (*(volatile char __force *)__p));
	else
		asm volatile(".byte 0x66; clflush %0",
                       "+m" (*(volatile char __force *)__p));

But that's _actively_ ugly.  alternative() syntax there makes sense.
Here, it's not ugly at all:

	if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
		asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
	else
		wbinvd();

and it's actually more readable with alternative() syntax.

So, please just do what makes the code look most readable. Performance
and consistency aren't important. I see absolutely nothing wrong with:

static __always_inline void raw_wbnoinvd(void)
{
        asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
}

void wbnoinvd(void)
{
	if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
		raw_wbnoinvd();
	else
		wbinvd();
}

... except the fact that cpu_feature_enabled() kinda sucks and needs
some work, but that's a whole other can of worms we can leave closed today.

Re: [PATCH v5 1/2] x86, lib: Add WBNOINVD helper functions

[Dave Hansen]

On 1/22/25 16:24, Kevin Loughlin wrote:
> +static __always_inline void wbnoinvd(void)
> +{
> +	/*
> +	 * WBNOINVD is encoded as 0xf3 0x0f 0x09. Making this
> +	 * encoding explicit ensures compatibility with older versions of
> +	 * binutils, which may not know about WBNOINVD.

This kinda pokes at one of my pet peeves. It's writing a comment where
code would do. I'd *much* rather write a function that explains to you
in code that "WBNOINVD is encoded as 0xf3 0x0f 0x09":

static __always_inline void native_wbnoinvd(void)
{
        asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
}

instead of writing out a comment. It's kinda silly to have to write out
the encoding explicitly in a comment and then have to rewrite it in the
code.

Re: [PATCH v5 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

On Wed, Jan 22, 2025 at 4:36 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 1/22/25 16:24, Kevin Loughlin wrote:
> > +static __always_inline void wbnoinvd(void)
> > +{
> > +     /*
> > +      * WBNOINVD is encoded as 0xf3 0x0f 0x09. Making this
> > +      * encoding explicit ensures compatibility with older versions of
> > +      * binutils, which may not know about WBNOINVD.
>
> This kinda pokes at one of my pet peeves. It's writing a comment where
> code would do. I'd *much* rather write a function that explains to you
> in code that "WBNOINVD is encoded as 0xf3 0x0f 0x09":
>
> static __always_inline void native_wbnoinvd(void)
> {
>         asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> }
>
> instead of writing out a comment. It's kinda silly to have to write out
> the encoding explicitly in a comment and then have to rewrite it in the
> code.

Yeah, I see your point. I will add this native_wbnoinvd() wrapper in v6; thanks!

Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

On Wed, Jan 22, 2025 at 4:33 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 1/22/25 16:06, Kevin Loughlin wrote:
> >> BTW, I don't think you should be compelled to use alternative() as
> >> opposed to a good old:
> >>
> >>         if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> >>                 ...
> > Agreed, though I'm leaving as alternative() for now (both because it
> > results in fewer checks and because that's what is used in the rest of
> > the file); please holler if you prefer otherwise. If so, my slight
> > preference in that case would be to update the whole file
> > stylistically in a separate commit.
>
> alternative() can make a _lot_ of sense.  It's extremely compact in the
> code it generates. It messes with compiler optimization, of course, just
> like any assembly. But, overall, it's great.
>
> In this case, though, we don't care one bit about code generation or
> performance. We're running the world's slowest instruction from an IPI.
>
> As for consistency, special_insns.h is gloriously inconsistent. But only
> two instructions use alternatives, and they *need* the asm syntax
> because they're passing registers and meaningful constraints in.
>
> The wbinvds don't get passed registers and their constraints are
> trivial. This conditional:
>
>         alternative_io(".byte 0x3e; clflush %0",
>                        ".byte 0x66; clflush %0",
>                        X86_FEATURE_CLFLUSHOPT,
>                        "+m" (*(volatile char __force *)__p));
>
> could be written like this:
>
>         if (cpu_feature_enabled(X86_FEATURE_CLFLUSHOPT))
>                 asm volatile(".byte 0x3e; clflush %0",
>                        "+m" (*(volatile char __force *)__p));
>         else
>                 asm volatile(".byte 0x66; clflush %0",
>                        "+m" (*(volatile char __force *)__p));
>
> But that's _actively_ ugly.  alternative() syntax there makes sense.
> Here, it's not ugly at all:
>
>         if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
>                 asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
>         else
>                 wbinvd();
>
> and it's actually more readable with alternative() syntax.
>
> So, please just do what makes the code look most readable. Performance
> and consistency aren't important. I see absolutely nothing wrong with:
>
> static __always_inline void raw_wbnoinvd(void)
> {
>         asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> }
>
> void wbnoinvd(void)
> {
>         if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
>                 raw_wbnoinvd();
>         else
>                 wbinvd();
> }
>
> ... except the fact that cpu_feature_enabled() kinda sucks and needs
> some work, but that's a whole other can of worms we can leave closed today.

Thanks for the detailed explanation; you've convinced me. v6 coming up
shortly (using native_wbnoinvd() instead of raw_wbnoinvd(), as you
named the proposed wrapper in your reply to v5).

Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

On Wed, Jan 22, 2025 at 4:58 PM Kevin Loughlin <kevinloughlin@google.com> wrote:
>
> On Wed, Jan 22, 2025 at 4:33 PM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 1/22/25 16:06, Kevin Loughlin wrote:
> > >> BTW, I don't think you should be compelled to use alternative() as
> > >> opposed to a good old:
> > >>
> > >>         if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> > >>                 ...
> > > Agreed, though I'm leaving as alternative() for now (both because it
> > > results in fewer checks and because that's what is used in the rest of
> > > the file); please holler if you prefer otherwise. If so, my slight
> > > preference in that case would be to update the whole file
> > > stylistically in a separate commit.
> >
> > alternative() can make a _lot_ of sense.  It's extremely compact in the
> > code it generates. It messes with compiler optimization, of course, just
> > like any assembly. But, overall, it's great.
> >
> > In this case, though, we don't care one bit about code generation or
> > performance. We're running the world's slowest instruction from an IPI.
> >
> > As for consistency, special_insns.h is gloriously inconsistent. But only
> > two instructions use alternatives, and they *need* the asm syntax
> > because they're passing registers and meaningful constraints in.
> >
> > The wbinvds don't get passed registers and their constraints are
> > trivial. This conditional:
> >
> >         alternative_io(".byte 0x3e; clflush %0",
> >                        ".byte 0x66; clflush %0",
> >                        X86_FEATURE_CLFLUSHOPT,
> >                        "+m" (*(volatile char __force *)__p));
> >
> > could be written like this:
> >
> >         if (cpu_feature_enabled(X86_FEATURE_CLFLUSHOPT))
> >                 asm volatile(".byte 0x3e; clflush %0",
> >                        "+m" (*(volatile char __force *)__p));
> >         else
> >                 asm volatile(".byte 0x66; clflush %0",
> >                        "+m" (*(volatile char __force *)__p));
> >
> > But that's _actively_ ugly.  alternative() syntax there makes sense.
> > Here, it's not ugly at all:
> >
> >         if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> >                 asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> >         else
> >                 wbinvd();
> >
> > and it's actually more readable with alternative() syntax.
> >
> > So, please just do what makes the code look most readable. Performance
> > and consistency aren't important. I see absolutely nothing wrong with:
> >
> > static __always_inline void raw_wbnoinvd(void)
> > {
> >         asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> > }
> >
> > void wbnoinvd(void)
> > {
> >         if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> >                 raw_wbnoinvd();
> >         else
> >                 wbinvd();
> > }
> >
> > ... except the fact that cpu_feature_enabled() kinda sucks and needs
> > some work, but that's a whole other can of worms we can leave closed today.
>
> Thanks for the detailed explanation; you've convinced me. v6 coming up
> shortly (using native_wbnoinvd() instead of raw_wbnoinvd(), as you
> named the proposed wrapper in your reply to v5).

Actually, we may still want to use alternative() for the following reason:

Kirill noted in ad3fe525b950 ("x86/mm: Unify pgtable_l5_enabled usage
in early boot code") that cpu_feature_enabled() can't be used in early
boot code, which would mean using it would make the wbnoinvd()
implementation incompatible with early boot code if desired there. In
contrast, I believe alternative() will just fall back to WBINVD until
apply_alternatives() runs, at which point it will be replaced with
WBNOINVD if the processor supports it.

I'll still use the native_wbnoinvd() wrapper to clarify the encoding
as you suggested, but does this reasoning for keeping alternative()
make sense to you Dave? Or am I missing something?

[PATCH v6 0/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning all
existing cache lines with the recycled ASID must be flushed), the
cache invalidation triggered by WBINVD is unnecessary; only the
writeback is needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches.

First, provide helper functions to use WBNOINVD similar to how WBINVD
is invoked. Second, check for WBNOINVD support and execute WBNOINVD if
possible in lieu of WBINVD to avoid cache invalidations.

Note that I have *not* rebased this series atop proposed targeted
flushing optimizations [0], since the optimizations do not yet appear
to be finalized. However, I'm happy to do a rebase if that would be
helpful.

[0] https://lore.kernel.org/kvm/85frlcvjyo.fsf@amd.com/T/

Changelog
---
v6:
- hoist WBNONINVD encoding into macro for readability; shorten comment
v5:
- explicitly encode wbnoinvd as 0xf3 0x0f 0x09 for binutils backwards compatibility
v4:
- add comments to wbnoinvd() for clarity on when to use and behavior
v3:
- rebase to tip @ e6609f8bea4a
- use WBINVD in wbnoinvd() if X86_FEATURE_WBNOINVD is not present
- provide sev_writeback_caches() wrapper function in anticipation of
  aforementioned [0] targeted flushing optimizations
- add Reviewed-by from Mingwei
- reword commits/comments
v2:
- rebase to tip @ dffeaed35cef
- drop unnecessary Xen changes
- reword commits/comments
---
Kevin Loughlin (2):
  x86, lib: Add WBNOINVD helper functions
  KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

 arch/x86/include/asm/smp.h           |  7 +++++
 arch/x86/include/asm/special_insns.h | 19 ++++++++++++-
 arch/x86/kvm/svm/sev.c               | 41 ++++++++++++++--------------
 arch/x86/lib/cache-smp.c             | 12 ++++++++
 4 files changed, 58 insertions(+), 21 deletions(-)

-- 
2.48.1.362.g079036d154-goog

[PATCH v6 1/2] x86, lib: Add WBNOINVD helper functions

[Kevin Loughlin]

In line with WBINVD usage, add WBONINVD helper functions. For the
wbnoinvd() helper, fall back to WBINVD if via alternative() if
X86_FEATURE_WBNOINVD is not present. alternative() ensures
compatibility with early boot code if needed.

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
---
 arch/x86/include/asm/smp.h           |  7 +++++++
 arch/x86/include/asm/special_insns.h | 19 ++++++++++++++++++-
 arch/x86/lib/cache-smp.c             | 12 ++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ca073f40698f..ecf93a243b83 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -112,6 +112,7 @@ void native_play_dead(void);
 void play_dead_common(void);
 void wbinvd_on_cpu(int cpu);
 int wbinvd_on_all_cpus(void);
+int wbnoinvd_on_all_cpus(void);
 
 void smp_kick_mwait_play_dead(void);
 
@@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void)
 	return 0;
 }
 
+static inline int wbnoinvd_on_all_cpus(void)
+{
+	wbnoinvd();
+	return 0;
+}
+
 static inline struct cpumask *cpu_llc_shared_mask(int cpu)
 {
 	return (struct cpumask *)cpumask_of(0);
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 03e7c2d49559..86a903742139 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -117,7 +117,24 @@ static inline void wrpkru(u32 pkru)
 
 static __always_inline void wbinvd(void)
 {
-	asm volatile("wbinvd": : :"memory");
+	asm volatile("wbinvd" : : : "memory");
+}
+
+/* Instruction encoding provided for binutils backwards compatibility. */
+#define WBNOINVD ".byte 0xf3,0x0f,0x09"
+
+/*
+ * Cheaper version of wbinvd(). Call when caches
+ * need to be written back but not invalidated.
+ */
+static __always_inline void wbnoinvd(void)
+{
+	/*
+	 * If WBNOINVD is unavailable, fall back to the compatible but
+	 * more destructive WBINVD (which still writes the caches back
+	 * but also invalidates them).
+	 */
+	alternative("wbinvd", WBNOINVD, X86_FEATURE_WBNOINVD);
 }
 
 static inline unsigned long __read_cr4(void)
diff --git a/arch/x86/lib/cache-smp.c b/arch/x86/lib/cache-smp.c
index 7af743bd3b13..7ac5cca53031 100644
--- a/arch/x86/lib/cache-smp.c
+++ b/arch/x86/lib/cache-smp.c
@@ -20,3 +20,15 @@ int wbinvd_on_all_cpus(void)
 	return 0;
 }
 EXPORT_SYMBOL(wbinvd_on_all_cpus);
+
+static void __wbnoinvd(void *dummy)
+{
+	wbnoinvd();
+}
+
+int wbnoinvd_on_all_cpus(void)
+{
+	on_each_cpu(__wbnoinvd, NULL, 1);
+	return 0;
+}
+EXPORT_SYMBOL(wbnoinvd_on_all_cpus);
-- 
2.48.1.362.g079036d154-goog

[PATCH v6 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Kevin Loughlin]

AMD CPUs currently execute WBINVD in the host when unregistering SEV
guest memory or when deactivating SEV guests. Such cache maintenance is
performed to prevent data corruption, wherein the encrypted (C=1)
version of a dirty cache line might otherwise only be written back
after the memory is written in a different context (ex: C=0), yielding
corruption. However, WBINVD is performance-costly, especially because
it invalidates processor caches.

Strictly-speaking, unless the SEV ASID is being recycled (meaning the
SNP firmware requires the use of WBINVD prior to DF_FLUSH), the cache
invalidation triggered by WBINVD is unnecessary; only the writeback is
needed to prevent data corruption in remaining scenarios.

To improve performance in these scenarios, use WBNOINVD when available
instead of WBINVD. WBNOINVD still writes back all dirty lines
(preventing host data corruption by SEV guests) but does *not*
invalidate processor caches. Note that the implementation of wbnoinvd()
ensures fall back to WBINVD if WBNOINVD is unavailable.

In anticipation of forthcoming optimizations to limit the WBNOINVD only
to physical CPUs that have executed SEV guests, place the call to
wbnoinvd_on_all_cpus() in a wrapper function sev_writeback_caches().

Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
Reviewed-by: Mingwei Zhang <mizhang@google.com>
---
 arch/x86/kvm/svm/sev.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index fe6cc763fd51..f10f1c53345e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -116,6 +116,7 @@ static int sev_flush_asids(unsigned int min_asid, unsigned int max_asid)
 	 */
 	down_write(&sev_deactivate_lock);
 
+	/* SNP firmware requires use of WBINVD for ASID recycling. */
 	wbinvd_on_all_cpus();
 
 	if (sev_snp_enabled)
@@ -710,6 +711,16 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
 	}
 }
 
+static inline void sev_writeback_caches(void)
+{
+	/*
+	 * Ensure that all dirty guest tagged cache entries are written back
+	 * before releasing the pages back to the system for use. CLFLUSH will
+	 * not do this without SME_COHERENT, so issue a WBNOINVD.
+	 */
+	wbnoinvd_on_all_cpus();
+}
+
 static unsigned long get_num_contig_pages(unsigned long idx,
 				struct page **inpages, unsigned long npages)
 {
@@ -2773,12 +2784,7 @@ int sev_mem_enc_unregister_region(struct kvm *kvm,
 		goto failed;
 	}
 
-	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
-	 */
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 
 	__unregister_enc_region_locked(kvm, region);
 
@@ -2899,12 +2905,7 @@ void sev_vm_destroy(struct kvm *kvm)
 		return;
 	}
 
-	/*
-	 * Ensure that all guest tagged cache entries are flushed before
-	 * releasing the pages back to the system for use. CLFLUSH will
-	 * not do this, so issue a WBINVD.
-	 */
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 
 	/*
 	 * if userspace was terminated before unregistering the memory regions
@@ -3126,16 +3127,16 @@ static void sev_flush_encrypted_page(struct kvm_vcpu *vcpu, void *va)
 
 	/*
 	 * VM Page Flush takes a host virtual address and a guest ASID.  Fall
-	 * back to WBINVD if this faults so as not to make any problems worse
+	 * back to WBNOINVD if this faults so as not to make any problems worse
 	 * by leaving stale encrypted data in the cache.
 	 */
 	if (WARN_ON_ONCE(wrmsrl_safe(MSR_AMD64_VM_PAGE_FLUSH, addr | asid)))
-		goto do_wbinvd;
+		goto do_sev_writeback_caches;
 
 	return;
 
-do_wbinvd:
-	wbinvd_on_all_cpus();
+do_sev_writeback_caches:
+	sev_writeback_caches();
 }
 
 void sev_guest_memory_reclaimed(struct kvm *kvm)
@@ -3144,12 +3145,12 @@ void sev_guest_memory_reclaimed(struct kvm *kvm)
 	 * With SNP+gmem, private/encrypted memory is unreachable via the
 	 * hva-based mmu notifiers, so these events are only actually
 	 * pertaining to shared pages where there is no need to perform
-	 * the WBINVD to flush associated caches.
+	 * the WBNOINVD to flush associated caches.
 	 */
 	if (!sev_guest(kvm) || sev_snp_guest(kvm))
 		return;
 
-	wbinvd_on_all_cpus();
+	sev_writeback_caches();
 }
 
 void sev_free_vcpu(struct kvm_vcpu *vcpu)
@@ -3858,7 +3859,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 		 * guest-mapped page rather than the initial one allocated
 		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
 		 * could be free'd and cleaned up here, but that involves
-		 * cleanups like wbinvd_on_all_cpus() which would ideally
+		 * cleanups like sev_writeback_caches() which would ideally
 		 * be handled during teardown rather than guest boot.
 		 * Deferring that also allows the existing logic for SEV-ES
 		 * VMSAs to be re-used with minimal SNP-specific changes.
@@ -4910,7 +4911,7 @@ void sev_gmem_invalidate(kvm_pfn_t start, kvm_pfn_t end)
 
 		/*
 		 * SEV-ES avoids host/guest cache coherency issues through
-		 * WBINVD hooks issued via MMU notifiers during run-time, and
+		 * WBNOINVD hooks issued via MMU notifiers during run-time, and
 		 * KVM's VM destroy path at shutdown. Those MMU notifier events
 		 * don't cover gmem since there is no requirement to map pages
 		 * to a HVA in order to use them for a running guest. While the
-- 
2.48.1.362.g079036d154-goog

Re: [PATCH v6 1/2] x86, lib: Add WBNOINVD helper functions

[Tom Lendacky]

On 1/31/25 18:02, Kevin Loughlin wrote:
> In line with WBINVD usage, add WBONINVD helper functions. For the
> wbnoinvd() helper, fall back to WBINVD if via alternative() if
> X86_FEATURE_WBNOINVD is not present. alternative() ensures
> compatibility with early boot code if needed.
> 
> Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/include/asm/smp.h           |  7 +++++++
>  arch/x86/include/asm/special_insns.h | 19 ++++++++++++++++++-
>  arch/x86/lib/cache-smp.c             | 12 ++++++++++++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index ca073f40698f..ecf93a243b83 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -112,6 +112,7 @@ void native_play_dead(void);
>  void play_dead_common(void);
>  void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
> +int wbnoinvd_on_all_cpus(void);
>  
>  void smp_kick_mwait_play_dead(void);
>  
> @@ -160,6 +161,12 @@ static inline int wbinvd_on_all_cpus(void)
>  	return 0;
>  }
>  
> +static inline int wbnoinvd_on_all_cpus(void)
> +{
> +	wbnoinvd();
> +	return 0;
> +}
> +
>  static inline struct cpumask *cpu_llc_shared_mask(int cpu)
>  {
>  	return (struct cpumask *)cpumask_of(0);
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 03e7c2d49559..86a903742139 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -117,7 +117,24 @@ static inline void wrpkru(u32 pkru)
>  
>  static __always_inline void wbinvd(void)
>  {
> -	asm volatile("wbinvd": : :"memory");
> +	asm volatile("wbinvd" : : : "memory");
> +}
> +
> +/* Instruction encoding provided for binutils backwards compatibility. */
> +#define WBNOINVD ".byte 0xf3,0x0f,0x09"
> +
> +/*
> + * Cheaper version of wbinvd(). Call when caches
> + * need to be written back but not invalidated.
> + */
> +static __always_inline void wbnoinvd(void)
> +{
> +	/*
> +	 * If WBNOINVD is unavailable, fall back to the compatible but
> +	 * more destructive WBINVD (which still writes the caches back
> +	 * but also invalidates them).
> +	 */
> +	alternative("wbinvd", WBNOINVD, X86_FEATURE_WBNOINVD);
>  }
>  
>  static inline unsigned long __read_cr4(void)
> diff --git a/arch/x86/lib/cache-smp.c b/arch/x86/lib/cache-smp.c
> index 7af743bd3b13..7ac5cca53031 100644
> --- a/arch/x86/lib/cache-smp.c
> +++ b/arch/x86/lib/cache-smp.c
> @@ -20,3 +20,15 @@ int wbinvd_on_all_cpus(void)
>  	return 0;
>  }
>  EXPORT_SYMBOL(wbinvd_on_all_cpus);
> +
> +static void __wbnoinvd(void *dummy)
> +{
> +	wbnoinvd();
> +}
> +
> +int wbnoinvd_on_all_cpus(void)
> +{
> +	on_each_cpu(__wbnoinvd, NULL, 1);
> +	return 0;
> +}
> +EXPORT_SYMBOL(wbnoinvd_on_all_cpus);

Re: [PATCH v6 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Tom Lendacky]

On 1/31/25 18:02, Kevin Loughlin wrote:
> AMD CPUs currently execute WBINVD in the host when unregistering SEV
> guest memory or when deactivating SEV guests. Such cache maintenance is
> performed to prevent data corruption, wherein the encrypted (C=1)
> version of a dirty cache line might otherwise only be written back
> after the memory is written in a different context (ex: C=0), yielding
> corruption. However, WBINVD is performance-costly, especially because
> it invalidates processor caches.
> 
> Strictly-speaking, unless the SEV ASID is being recycled (meaning the
> SNP firmware requires the use of WBINVD prior to DF_FLUSH), the cache
> invalidation triggered by WBINVD is unnecessary; only the writeback is
> needed to prevent data corruption in remaining scenarios.
> 
> To improve performance in these scenarios, use WBNOINVD when available
> instead of WBINVD. WBNOINVD still writes back all dirty lines
> (preventing host data corruption by SEV guests) but does *not*
> invalidate processor caches. Note that the implementation of wbnoinvd()
> ensures fall back to WBINVD if WBNOINVD is unavailable.
> 
> In anticipation of forthcoming optimizations to limit the WBNOINVD only
> to physical CPUs that have executed SEV guests, place the call to
> wbnoinvd_on_all_cpus() in a wrapper function sev_writeback_caches().
> 
> Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
> Reviewed-by: Mingwei Zhang <mizhang@google.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/kvm/svm/sev.c | 41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index fe6cc763fd51..f10f1c53345e 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -116,6 +116,7 @@ static int sev_flush_asids(unsigned int min_asid, unsigned int max_asid)
>  	 */
>  	down_write(&sev_deactivate_lock);
>  
> +	/* SNP firmware requires use of WBINVD for ASID recycling. */
>  	wbinvd_on_all_cpus();
>  
>  	if (sev_snp_enabled)
> @@ -710,6 +711,16 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
>  	}
>  }
>  
> +static inline void sev_writeback_caches(void)
> +{
> +	/*
> +	 * Ensure that all dirty guest tagged cache entries are written back
> +	 * before releasing the pages back to the system for use. CLFLUSH will
> +	 * not do this without SME_COHERENT, so issue a WBNOINVD.
> +	 */
> +	wbnoinvd_on_all_cpus();
> +}
> +
>  static unsigned long get_num_contig_pages(unsigned long idx,
>  				struct page **inpages, unsigned long npages)
>  {
> @@ -2773,12 +2784,7 @@ int sev_mem_enc_unregister_region(struct kvm *kvm,
>  		goto failed;
>  	}
>  
> -	/*
> -	 * Ensure that all guest tagged cache entries are flushed before
> -	 * releasing the pages back to the system for use. CLFLUSH will
> -	 * not do this, so issue a WBINVD.
> -	 */
> -	wbinvd_on_all_cpus();
> +	sev_writeback_caches();
>  
>  	__unregister_enc_region_locked(kvm, region);
>  
> @@ -2899,12 +2905,7 @@ void sev_vm_destroy(struct kvm *kvm)
>  		return;
>  	}
>  
> -	/*
> -	 * Ensure that all guest tagged cache entries are flushed before
> -	 * releasing the pages back to the system for use. CLFLUSH will
> -	 * not do this, so issue a WBINVD.
> -	 */
> -	wbinvd_on_all_cpus();
> +	sev_writeback_caches();
>  
>  	/*
>  	 * if userspace was terminated before unregistering the memory regions
> @@ -3126,16 +3127,16 @@ static void sev_flush_encrypted_page(struct kvm_vcpu *vcpu, void *va)
>  
>  	/*
>  	 * VM Page Flush takes a host virtual address and a guest ASID.  Fall
> -	 * back to WBINVD if this faults so as not to make any problems worse
> +	 * back to WBNOINVD if this faults so as not to make any problems worse
>  	 * by leaving stale encrypted data in the cache.
>  	 */
>  	if (WARN_ON_ONCE(wrmsrl_safe(MSR_AMD64_VM_PAGE_FLUSH, addr | asid)))
> -		goto do_wbinvd;
> +		goto do_sev_writeback_caches;
>  
>  	return;
>  
> -do_wbinvd:
> -	wbinvd_on_all_cpus();
> +do_sev_writeback_caches:
> +	sev_writeback_caches();
>  }
>  
>  void sev_guest_memory_reclaimed(struct kvm *kvm)
> @@ -3144,12 +3145,12 @@ void sev_guest_memory_reclaimed(struct kvm *kvm)
>  	 * With SNP+gmem, private/encrypted memory is unreachable via the
>  	 * hva-based mmu notifiers, so these events are only actually
>  	 * pertaining to shared pages where there is no need to perform
> -	 * the WBINVD to flush associated caches.
> +	 * the WBNOINVD to flush associated caches.
>  	 */
>  	if (!sev_guest(kvm) || sev_snp_guest(kvm))
>  		return;
>  
> -	wbinvd_on_all_cpus();
> +	sev_writeback_caches();
>  }
>  
>  void sev_free_vcpu(struct kvm_vcpu *vcpu)
> @@ -3858,7 +3859,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
>  		 * guest-mapped page rather than the initial one allocated
>  		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
>  		 * could be free'd and cleaned up here, but that involves
> -		 * cleanups like wbinvd_on_all_cpus() which would ideally
> +		 * cleanups like sev_writeback_caches() which would ideally
>  		 * be handled during teardown rather than guest boot.
>  		 * Deferring that also allows the existing logic for SEV-ES
>  		 * VMSAs to be re-used with minimal SNP-specific changes.
> @@ -4910,7 +4911,7 @@ void sev_gmem_invalidate(kvm_pfn_t start, kvm_pfn_t end)
>  
>  		/*
>  		 * SEV-ES avoids host/guest cache coherency issues through
> -		 * WBINVD hooks issued via MMU notifiers during run-time, and
> +		 * WBNOINVD hooks issued via MMU notifiers during run-time, and
>  		 * KVM's VM destroy path at shutdown. Those MMU notifier events
>  		 * don't cover gmem since there is no requirement to map pages
>  		 * to a HVA in order to use them for a running guest. While the

Re: [PATCH v6 1/2] x86, lib: Add WBNOINVD helper functions

[Sean Christopherson]

On Sat, Feb 01, 2025, Kevin Loughlin wrote:
> +static inline int wbnoinvd_on_all_cpus(void)
> +{
> +	wbnoinvd();
> +	return 0;

Returning anything is silly.  I'll prepend a patch (I'm going to send a combined
version of this and the targeted flushing series) to remove the return value
from wbinvd_on_all_cpus(), which I'm guessing is the source of the silliness.

>  static inline struct cpumask *cpu_llc_shared_mask(int cpu)
>  {
>  	return (struct cpumask *)cpumask_of(0);
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 03e7c2d49559..86a903742139 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -117,7 +117,24 @@ static inline void wrpkru(u32 pkru)
>  
>  static __always_inline void wbinvd(void)
>  {
> -	asm volatile("wbinvd": : :"memory");
> +	asm volatile("wbinvd" : : : "memory");
> +}
> +
> +/* Instruction encoding provided for binutils backwards compatibility. */
> +#define WBNOINVD ".byte 0xf3,0x0f,0x09"

Argh.  This causes problems for KVM, because KVM's newfangled CPUID macros heavily
use token pasting with X86_FEATURE_xxx, and so KVM's usage of:

	F(WBNOINVD)

causes explosions.  Somewhat amusingly, this is the second time today I ran into
this problem, as WRMSRNS as the safe issue.

Dave (and others),

Any thoughts on the best way forward?  I hacked around a similar collision in
commit 8d862c270bf1 ("KVM: x86: #undef SPEC_CTRL_SSBD in cpuid.c to avoid macro
collisions"), but (a) that scares me less because KVM should never use the
SPEC_CTRL_SSBD macro, and (b) I really, really don't want that to be the long-term
solution.  The only reason I committed the hack was because it was the only
blocking issue for a massive rework, and I couldn't get traction on renaming
the MSR macro.

For WBNOINVD, WRMSRNS, and any other instructions that come along, what about this?
Quite ugly, but it's at least descriptive.  And more importantly, the chances of
unwanted behavior are quite low.


/* Instruction encoding provided for binutils backwards compatibility. */
#define ASM_WBNOINVD ".byte 0xf3,0x0f,0x09"

/*
 * Cheaper version of wbinvd(). Call when caches need to be written back but
 * not invalidated.
 */
static __always_inline void wbnoinvd(void)
{
	/*
	 * If WBNOINVD is unavailable, fall back to the compatible but
	 * more destructive WBINVD (which still writes the caches back
	 * but also invalidates them).
	 */
	alternative("wbinvd", ASM_WBNOINVD, X86_FEATURE_WBNOINVD);
}

Re: [PATCH v5 2/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Sean Christopherson]

On Thu, Jan 23, 2025, Kevin Loughlin wrote:
> +static inline void sev_writeback_caches(void)
> +{
> +	/*
> +	 * Ensure that all dirty guest tagged cache entries are written back
> +	 * before releasing the pages back to the system for use. CLFLUSH will
> +	 * not do this without SME_COHERENT, so issue a WBNOINVD.

This is somewhat misleading.  A naive reading of this would interpret the above
as saying that KVM _should_ do CLFLUSH on SME_COHERENT CPUs, which begs the
question of why KVM _doesn't_ do that.  I also think this is the right place to
call out that WBNOINVD support isn't guaranteed.

	 * Ensure that all dirty guest tagged cache entries are written back
	 * before releasing the pages back to the system for use.  CLFLUSH will
	 * not do this without SME_COHERENT, and flushing many cache lines
	 * individually is slower than blasting WBINVD for large VMs, so issue
	 * WBNOINVD (or WBINVD if the "no invalidate" variant is unsupported)
	 * on CPUs that have done VMRUN, i.e. may have dirtied data using the
	 * VM's ASID.

> +	 */
> +	wbnoinvd_on_all_cpus();
> +}
> +
>  static unsigned long get_num_contig_pages(unsigned long idx,
>  				struct page **inpages, unsigned long npages)
>  {
> @@ -2773,12 +2784,7 @@ int sev_mem_enc_unregister_region(struct kvm *kvm,
>  		goto failed;
>  	}
>  
> -	/*
> -	 * Ensure that all guest tagged cache entries are flushed before
> -	 * releasing the pages back to the system for use. CLFLUSH will
> -	 * not do this, so issue a WBINVD.
> -	 */
> -	wbinvd_on_all_cpus();
> +	sev_writeback_caches();
>  
>  	__unregister_enc_region_locked(kvm, region);
>  
> @@ -2899,12 +2905,7 @@ void sev_vm_destroy(struct kvm *kvm)
>  		return;
>  	}
>  
> -	/*
> -	 * Ensure that all guest tagged cache entries are flushed before
> -	 * releasing the pages back to the system for use. CLFLUSH will
> -	 * not do this, so issue a WBINVD.
> -	 */
> -	wbinvd_on_all_cpus();
> +	sev_writeback_caches();
>  
>  	/*
>  	 * if userspace was terminated before unregistering the memory regions
> @@ -3126,16 +3127,16 @@ static void sev_flush_encrypted_page(struct kvm_vcpu *vcpu, void *va)
>  
>  	/*
>  	 * VM Page Flush takes a host virtual address and a guest ASID.  Fall
> -	 * back to WBINVD if this faults so as not to make any problems worse
> +	 * back to WBNOINVD if this faults so as not to make any problems worse

I don't like replacing WBINVD with WBNOINVD everywhere.  It incorrectly implies
that WBNOINVD is guaranteed.  Easiest thing is just to avoid mnemonics and describe
the behavior in conversational language (well, as conversational as cache flushin
gets :-D).
 
> @@ -3858,7 +3859,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
>  		 * guest-mapped page rather than the initial one allocated
>  		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
>  		 * could be free'd and cleaned up here, but that involves
> -		 * cleanups like wbinvd_on_all_cpus() which would ideally
> +		 * cleanups like sev_writeback_caches() which would ideally

Similarly, avoid function names in comments, because they too become stale.

>  		 * be handled during teardown rather than guest boot.
>  		 * Deferring that also allows the existing logic for SEV-ES
>  		 * VMSAs to be re-used with minimal SNP-specific changes.

Re: [PATCH v5 0/2] KVM: SEV: Prefer WBNOINVD over WBINVD for cache maintenance efficiency

[Sean Christopherson]

On Thu, Jan 23, 2025, Kevin Loughlin wrote:
> v5:
> - explicitly encode wbnoinvd as 0xf3 0x0f 0x09 for binutils backwards compatibility

Please, please, please do not send new series with In-Reply-To.  Trying to sort
through the different versions in my workflow was painful.  From
Documentation/process/maintainer-kvm-x86.rst:

Links
~~~~~
Do not explicitly reference bug reports, prior versions of a patch/series, etc.
via ``In-Reply-To:`` headers.  Using ``In-Reply-To:`` becomes an unholy mess
for large series and/or when the version count gets high, and ``In-Reply-To:``
is useless for anyone that doesn't have the original message, e.g. if someone
wasn't Cc'd on the bug report or if the list of recipients changes between
versions.

To link to a bug report, previous version, or anything of interest, use lore
links.  For referencing previous version(s), generally speaking do not include
a Link: in the changelog as there is no need to record the history in git, i.e.
put the link in the cover letter or in the section git ignores.  Do provide a
formal Link: for bug reports and/or discussions that led to the patch.  The
context of why a change was made is highly valuable for future readers.

Re: [PATCH v6 1/2] x86, lib: Add WBNOINVD helper functions

[Borislav Petkov]

On Tue, Feb 25, 2025 at 05:26:53PM -0800, Sean Christopherson wrote:
> /* Instruction encoding provided for binutils backwards compatibility. */
> #define ASM_WBNOINVD ".byte 0xf3,0x0f,0x09"

Yah, or simply INSN_WBNOINVD as that name basically tells you what it is. And
we have already

arch/x86/include/asm/bug.h:13:#define INSN_UD2  0x0b0f

oh and

arch/x86/include/asm/bug.h:12:#define ASM_UD2           ".byte 0x0f, 0x0b"

Pff.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette