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

[Sean Christopherson <seanjc@google.com>]

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.