Re: [PATCH 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

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

On 8/12/24 14:42, 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).
> 
> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
> ASIDs. 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 patch-set adds a new module parameter to the CCP driver defined as
> psp_max_snp_asid which is 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.

Add something here to talk about how cipher text hiding needs to be
enabled on SNP_INIT_EX and so the module parameters are part of the CCP
driver.

Not sure if having KVM invoke the CCP, at KVM module load time, to
perform the SNP_INIT_EX would make sense or not. That would allow all
the cipher text hiding support parameters to be in KVM. Lets see what
others think.

> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  arch/x86/kvm/svm/sev.c       | 24 ++++++++++++++---
>  drivers/crypto/ccp/sev-dev.c | 50 ++++++++++++++++++++++++++++++++++++
>  include/linux/psp-sev.h      | 10 ++++++--
>  3 files changed, 79 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 532df12b43c5..954ef99a1aa8 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -173,6 +173,9 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
>  
>  static int sev_asid_new(struct kvm_sev_info *sev)
>  {
> +	struct kvm_svm *svm = container_of(sev, struct kvm_svm, sev_info);
> +	struct kvm *kvm = &svm->kvm;

Why not just add the vm-type to the input parameters?

> +
>  	/*
>  	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
>  	 * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
> @@ -199,6 +202,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
> +	 * SNP_INIT_EX command and all the SEV-ES guests must have
> +	 * an ASID greater than MAX_SNP_ASID.
> +	 */
> +	if (snp_cipher_text_hiding_en && sev->es_active) {
> +		if (kvm->arch.vm_type == KVM_X86_SNP_VM)
> +			max_asid = max_snp_asid;
> +		else
> +			min_asid = max_snp_asid + 1;
> +	}
>  again:
>  	asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid);
>  	if (asid > max_asid) {
> @@ -3058,14 +3073,17 @@ 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 (max_snp_asid >= (min_sev_asid - 1))
> +			sev_es_supported = false;
>  		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
>  			sev_es_supported ? "enabled" : "disabled",
> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
> +			min_sev_asid > 1 ? max_snp_asid ? max_snp_asid + 1 : 1 : 0, min_sev_asid - 1);

Maybe put the last parameter on a separate line to make this a little
easier to read?

> +	}
>  	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
>  		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
>  			sev_snp_supported ? "enabled" : "disabled",
> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
> +			min_sev_asid > 1 ? 1 : 0, max_snp_asid ? : min_sev_asid - 1);
>  
>  	sev_enabled = sev_supported;
>  	sev_es_enabled = sev_es_supported;
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index eefb481db5af..9ee81a6defc5 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
>  module_param(psp_init_on_probe, bool, 0444);
>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>  
> +static bool psp_cth_enabled = true;

This is a static, so how about just calling this cipher_text_hiding.

> +module_param(psp_cth_enabled, bool, 0444);
> +MODULE_PARM_DESC(psp_cth_enabled, "  if true, the PSP will enable Cipher Text Hiding");
> +
> +static int psp_max_snp_asid;

Also a static, so just call this max_snp_asid.

> +module_param(psp_max_snp_asid, int, 0444);
> +MODULE_PARM_DESC(psp_max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");
> +
>  MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
>  MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
>  MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
>  MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */
>  
> +/* Cipher Text Hiding Enabled */
> +bool snp_cipher_text_hiding_en;
> +EXPORT_SYMBOL(snp_cipher_text_hiding_en);

I think you drop the "_en" and just have snp_cipher_text_hiding.

> +
> +/* MAX_SNP_ASID */
> +unsigned int max_snp_asid;
> +EXPORT_SYMBOL(max_snp_asid);

This should have "snp_" to provide better namespace isolation.

> +
>  static bool psp_dead;
>  static int psp_timeout;
>  
> @@ -1053,6 +1069,36 @@ static void snp_set_hsave_pa(void *arg)
>  	wrmsrl(MSR_VM_HSAVE_PA, 0);
>  }
>  
> +static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error)
> +{
> +	struct psp_device *psp = psp_master;
> +	struct sev_device *sev;
> +	unsigned int edx;
> +
> +	sev = psp->sev_data;
> +
> +	/*
> +	 * Check if CipherTextHiding feature is supported and enabled
> +	 * in the Platform/BIOS.
> +	 */
> +	if (sev->feat_info.ecx & FEAT_CIPHERTEXTHIDING_SUPPORTED &&
> +	    sev->snp_plat_status.ciphertext_hiding_cap) {

I'm not sure you need both checks. Either the platform status or the
feature info check should be enough. Can you check on that?

> +		/* Retrieve SEV CPUID information */
> +		edx = cpuid_edx(0x8000001f);
> +		/* Do sanity checks on user-defined MAX_SNP_ASID */
> +		if (psp_max_snp_asid > (edx - 1)) {

How about:
		if (psp_max_snp_asid >= edx) {

> +			dev_info(sev->dev, "user-defined MAX_SNP_ASID invalid, limiting to %d\n",

max_snp_asid module parameter is not valid, limiting to %d\n

> +				 edx - 1);
> +			psp_max_snp_asid = edx - 1;
> +		}
> +		max_snp_asid = psp_max_snp_asid ? : (edx - 1) / 2;

Add a blank line here

> +		snp_cipher_text_hiding_en = 1;
> +		data->ciphertext_hiding_en = 1;
> +		data->max_snp_asid = max_snp_asid;

Ditto

Thanks,
Tom

> +		dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n");
> +	}
> +}
> +
>  static void sev_cache_snp_platform_status_and_discover_features(void)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> @@ -1181,6 +1227,10 @@ static int __sev_snp_init_locked(int *error)
>  		}
>  
>  		memset(&data, 0, sizeof(data));
> +
> +		if (psp_cth_enabled)
> +			sev_snp_enable_ciphertext_hiding(&data, error);
> +
>  		data.init_rmp = 1;
>  		data.list_paddr_en = 1;
>  		data.list_paddr = __psp_pa(snp_range_list);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index d46d73911a76..a26b43c2eab9 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -27,6 +27,9 @@ enum sev_state {
>  	SEV_STATE_MAX
>  };
>  
> +extern bool snp_cipher_text_hiding_en;
> +extern unsigned int max_snp_asid;
> +
>  /**
>   * SEV platform and guest management commands
>   */
> @@ -746,10 +749,13 @@ struct sev_data_snp_guest_request {
>  struct sev_data_snp_init_ex {
>  	u32 init_rmp:1;
>  	u32 list_paddr_en:1;
> -	u32 rsvd:30;
> +	u32 rapl_dis:1;
> +	u32 ciphertext_hiding_en:1;
> +	u32 rsvd:28;
>  	u32 rsvd1;
>  	u64 list_paddr;
> -	u8  rsvd2[48];
> +	u16 max_snp_asid;
> +	u8  rsvd2[46];
>  } __packed;
>  
>  /**