Message-ID: <aAlYV-4q6ndhJAVe@google.com>
Date: Wed, 23 Apr 2025 14:15:03 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Ashish Kalra <Ashish.Kalra@....com>
Cc: pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, hpa@...or.com, herbert@...dor.apana.org.au, 
	x86@...nel.org, john.allen@....com, davem@...emloft.net, 
	thomas.lendacky@....com, michael.roth@....com, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org
Subject: Re: [PATCH v3 4/4] KVM: SVM: Add SEV-SNP CipherTextHiding support

On Tue, Apr 22, 2025, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@....com>
> 
> Ciphertext hiding prevents host accesses from reading the ciphertext of
> SNP guest private memory. Instead of reading ciphertext, the host reads
> will see constant default values (0xff).
> 
> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
> ASIDs.

Uh, no.  The only "host" ASID is '0'.

> All SNP active guests must have an ASID less than or equal to MAX_SNP_ASID
> provided to the SNP_INIT_EX command. All SEV-legacy guests (SEV and SEV-ES)
> must be greater than MAX_SNP_ASID.

This is misleading, arguably wrong.  The ASID space is already split into legacy+SEV and
SEV-ES+.  CTH further splits the SEV-ES+ space into SEV-ES and SEV-SNP+.
> 
> This patch-set adds two new module parameters to the KVM module, first

No "This patch".

> to enable CipherTextHiding support and a user configurable MAX_SNP_ASID
> to define the system-wide maximum SNP ASID value. If this value is not set,
> then the ASID space is equally divided between SEV-SNP and SEV-ES guests.

This quite, and I suspect completely useless for every production use case.  I
also *really* dislike max_snp_asid.  More below.

> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
> ---
>  arch/x86/kvm/svm/sev.c | 50 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7a156ba07d1f..a905f755312a 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -58,6 +58,14 @@ static bool sev_es_debug_swap_enabled = true;
>  module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>  static u64 sev_supported_vmsa_features;
>  
> +static bool cipher_text_hiding;
> +module_param(cipher_text_hiding, bool, 0444);
> +MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
> +
> +static int max_snp_asid;
> +module_param(max_snp_asid, int, 0444);
> +MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");

I'd much, much prefer proper document in Documentation/admin-guide/kernel-parameters.txt.
The basic gist of the params is self-explanatory, but how all of this works is not.

And max_snp_asid is extremely misleading.  Pretty much any reader is going to expect
it to do what it says: set the max SNP ASID.  But unless cipher_text_hiding is
enabled, which it's not by default, the param does absolutely nothing.

To address both problems, can we somehow figure out a way to use a single param?
The hardest part is probably coming up with a name.  E.g.

  static int ciphertext_hiding_nr_asids;
  module_param(ciphertext_hiding_nr_asids, int, 0444);

Then a non-zero value means "enable CipherTexthiding", and effects the ASID carve-out.
If we wanted to support the 50/50 split, we would use '-1' as an "auto" flag,
i.e. enable CipherTexthiding and split the SEV-ES+ ASIDs.  Though to be honest,
I'd prefer to avoid that unless it's actually useful.

Ha!  And I'm doubling down on that suggestion, because this code is wrong:

	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
		if (snp_max_snp_asid >= (min_sev_asid - 1))
			sev_es_supported = false;
		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
			str_enabled_disabled(sev_es_supported),
			min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
							      0, min_sev_asid - 1);
	}

A non-zero snp_max_snp_asid shouldn't break SEV-ES if CipherTextHiding isn't supported.

>  #define AP_RESET_HOLD_NONE		0
>  #define AP_RESET_HOLD_NAE_EVENT		1
>  #define AP_RESET_HOLD_MSR_PROTO		2
> @@ -85,6 +93,8 @@ static DEFINE_MUTEX(sev_bitmap_lock);
>  unsigned int max_sev_asid;
>  static unsigned int min_sev_asid;
>  static unsigned long sev_me_mask;
> +static unsigned int snp_max_snp_asid;
> +static bool snp_cipher_text_hiding;
>  static unsigned int nr_asids;
>  static unsigned long *sev_asid_bitmap;
>  static unsigned long *sev_reclaim_asid_bitmap;
> @@ -171,7 +181,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
>  	misc_cg_uncharge(type, sev->misc_cg, 1);
>  }
>  
> -static int sev_asid_new(struct kvm_sev_info *sev)
> +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
>  {
>  	/*
>  	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
> @@ -199,6 +209,18 @@ static int sev_asid_new(struct kvm_sev_info *sev)
>  
>  	mutex_lock(&sev_bitmap_lock);
>  
> +	/*
> +	 * When CipherTextHiding is enabled, all SNP guests must have an
> +	 * ASID less than or equal to MAX_SNP_ASID provided on the

Wrap at ~80, not

> +	 * SNP_INIT_EX command and all the SEV-ES guests must have
> +	 * an ASID greater than MAX_SNP_ASID.

Please don't referense MAX_SNP_ASID.  The reader doesn't need to know what the
PSP calls its parameter.  What matters is the concept, and to a lesser extent
KVM's param.

> +	 */
> +	if (snp_cipher_text_hiding && sev->es_active) {
> +		if (vm_type == KVM_X86_SNP_VM)
> +			max_asid = snp_max_snp_asid;
> +		else
> +			min_asid = snp_max_snp_asid + 1;
> +	}

Irrespective of the module params, I would much prefer to have a max_snp_asid
param that is kept up-to-date regardless of whether or not CipherTextHiding is
enabled.   Then you don't need a comment here, only a big fat comment in the code
that configures the min/max ASIDs, which is going to be a gnarly comment no matter
what we do.  Oh, and this should be done before the

	if (min_asid > max_asid)
		return -ENOTTY;

sanity check.

And then drop the mix of ternary operators and if statements, and just do:

	unsigned int min_asid, max_asid, asid;
	bool retry = true;
	int ret;

	if (vm_type == KVM_X86_SNP_VM) {
		min_asid = min_snp_asid;
		max_asid = max_snp_asid;
	} else if (sev->es_active) {
		min_asid = min_sev_es_asid;
		max_asid = max_sev_es_asid;
	} else {
		min_asid = min_sev_asid;
		max_asid = max_sev_asid;
	}

	/*
	 * The min ASID can end up larger than the max if basic SEV support is
	 * effectively disabled by disallowing use of ASIDs for SEV guests.
	 * Ditto for SEV-ES guests when CipherTextHiding is enabled.
	 */
	if (min_asid > max_asid)
		return -ENOTTY;

> @@ -3040,14 +3074,18 @@ void __init sev_hardware_setup(void)
>  								       "unusable" :
>  								       "disabled",
>  			min_sev_asid, max_sev_asid);
> -	if (boot_cpu_has(X86_FEATURE_SEV_ES))
> +	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
> +		if (snp_max_snp_asid >= (min_sev_asid - 1))
> +			sev_es_supported = false;
>  		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
>  			str_enabled_disabled(sev_es_supported),
> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
> +			min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
> +							      0, min_sev_asid - 1);
> +	}
>  	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
>  		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
>  			str_enabled_disabled(sev_snp_supported),
> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
> +			min_sev_asid > 1 ? 1 : 0, snp_max_snp_asid ? : min_sev_asid - 1);

Mixing in snp_max_snp_asid pretty much makes this is unreadable.  Please rework
this code to generate {min,max}_{sev,sev_es,snp,}_asid (add prep patches if
necessary).  I don't care terribly if ternary operators are used, but please
don't chain them.

>  
>  	sev_enabled = sev_supported;
>  	sev_es_enabled = sev_es_supported;
> @@ -3068,6 +3106,8 @@ void __init sev_hardware_setup(void)
>  	 * Do both SNP and SEV initialization at KVM module load.
>  	 */
>  	init_args.probe = true;
> +	init_args.cipher_text_hiding_en = snp_cipher_text_hiding;
> +	init_args.snp_max_snp_asid = snp_max_snp_asid;
>  	sev_platform_init(&init_args);
>  }
>  
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists