Re: [PATCH v4 5/5] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Tom Lendacky <thomas.lendacky@amd.com>]

On 5/19/25 19:02, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.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).
> 
> The SEV ASID space is basically split into legacy SEV and SEV-ES+.
> CipherTextHiding further partitions the SEV-ES+ ASID space into SEV-ES

s/CipherTextHiding/Ciphertext hiding/

> and SEV-SNP.
> 
> Add new module parameter to the KVM module to enable CipherTextHiding

Ditto.

> support and a user configurable system-wide maximum SNP ASID value. If
> the module parameter value is -1 then the ASID space is equally
> divided between SEV-SNP and SEV-ES guests.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  .../admin-guide/kernel-parameters.txt         | 10 ++++++
>  arch/x86/kvm/svm/sev.c                        | 31 +++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1e5e76bba9da..2cddb2b5c59d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2891,6 +2891,16 @@
>  			(enabled). Disable by KVM if hardware lacks support
>  			for NPT.
>  
> +	kvm-amd.ciphertext_hiding_nr_asids=

s/ns_asids/asids/

I'm not sure that the "nr" adds anything here.

> +			[KVM,AMD] Enables SEV-SNP CipherTextHiding feature and
> +			controls show many ASIDs are available for SEV-SNP guests.
> +			The ASID space is basically split into legacy SEV and
> +			SEV-ES+. CipherTextHiding feature further splits the
> +			SEV-ES+ ASID space into SEV-ES and SEV-SNP.
> +			If the value is -1, then it is used as an auto flag
> +			and splits the ASID space equally between SEV-ES and
> +			SEV-SNP ASIDs.
> +

Ditto on Dave's comments.

>  	kvm-arm.mode=
>  			[KVM,ARM,EARLY] Select one of KVM/arm64's modes of
>  			operation.
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 383db1da8699..68dcb13d98f2 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -59,6 +59,10 @@ 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 int ciphertext_hiding_nr_asids;
> +module_param(ciphertext_hiding_nr_asids, int, 0444);
> +MODULE_PARM_DESC(max_snp_asid, "  Number of ASIDs available for SEV-SNP guests when CipherTextHiding is enabled");
> +
>  #define AP_RESET_HOLD_NONE		0
>  #define AP_RESET_HOLD_NAE_EVENT		1
>  #define AP_RESET_HOLD_MSR_PROTO		2
> @@ -200,6 +204,9 @@ static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
>  	/*
>  	 * 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.
> +	 * Similarly for SEV-ES guests the min ASID can end up larger than the
> +	 * max when CipherTextHiding is enabled, effectively disabling SEV-ES
> +	 * support.
>  	 */
>  
>  	if (min_asid > max_asid)
> @@ -2955,6 +2962,7 @@ void __init sev_hardware_setup(void)
>  {
>  	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
>  	struct sev_platform_init_args init_args = {0};
> +	bool snp_cipher_text_hiding = false;
>  	bool sev_snp_supported = false;
>  	bool sev_es_supported = false;
>  	bool sev_supported = false;
> @@ -3052,6 +3060,27 @@ void __init sev_hardware_setup(void)
>  	if (min_sev_asid == 1)
>  		goto out;
>  
> +	/*
> +	 * The ASID space is basically split into legacy SEV and SEV-ES+.
> +	 * CipherTextHiding feature further partitions the SEV-ES+ ASID space
> +	 * into ASIDs for SEV-ES and SEV-SNP guests.

I think it is already understood that the ASID space is split between SEV
and SEV-ES/SEV-SNP guests. So something like this maybe?

The ciphertext hiding feature partions the joint SEV-ES/SEV-SNP ASID range
into separate SEV-ES and SEV-SNP ASID ranges with teh SEV-SNP ASID range
starting at 1.

> +	 */
> +	if (ciphertext_hiding_nr_asids && sev_is_snp_ciphertext_hiding_supported()) {
> +		/* Do sanity checks on user-defined ciphertext_hiding_nr_asids */
> +		if (ciphertext_hiding_nr_asids != -1 &&
> +		    ciphertext_hiding_nr_asids >= min_sev_asid) {
> +			pr_info("ciphertext_hiding_nr_asids module parameter invalid, limiting SEV-SNP ASIDs to %d\n",
> +				 min_sev_asid);
> +			ciphertext_hiding_nr_asids = min_sev_asid - 1;

So specifying a number greater than min_sev_asid will result in enabling
ciphertext hiding and no SEV-ES guests allowed even though you report that
the number is invalid?

I think the message can be worded better to convey what happens.

"Requested ciphertext hiding ASIDs (%u) exceeds minimum SEV ASID (%u), setting ciphertext hiding ASID range to the maximum value (%u)\n"

Or something a little more concise.

> +		}
> +
> +		min_sev_es_asid = ciphertext_hiding_nr_asids == -1 ? (min_sev_asid - 1) / 2 :

Should this be (min_sev_asid - 1) / 2 + 1 ?

Take min_sev_asid = 3, that means min_sev_es_asid would be 2 and
max_snp_asid would be 1, right?

And if you set min_sev_es_asid before first you favor SEV-ES.

So should you do:

max_snp_asid = ciphertext_hiding_asids != -1 ? : (min_sev_asid - 1) / 2 + 1;
min_sev_es_asid = max_snp_asid + 1;


> +				  ciphertext_hiding_nr_asids + 1;
> +		max_snp_asid = min_sev_es_asid - 1;
> +		snp_cipher_text_hiding = true;
> +		pr_info("SEV-SNP CipherTextHiding feature support enabled\n");

"SEV-SNP ciphertext hiding enabled\n"

No need to use the CipherTextHiding nomenclature everywhere.

Thanks,
Tom

> +	}
> +
>  	sev_es_asid_count = min_sev_asid - 1;
>  	WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
>  	sev_es_supported = true;
> @@ -3092,6 +3121,8 @@ void __init sev_hardware_setup(void)
>  	 * Do both SNP and SEV initialization at KVM module load.
>  	 */
>  	init_args.probe = true;
> +	if (snp_cipher_text_hiding)
> +		init_args.snp_max_snp_asid = max_snp_asid;
>  	sev_platform_init(&init_args);
>  }
>