[PATCH v3 0/4] Add SEV-SNP CipherTextHiding feature support

[PATCH v3 0/4] Add SEV-SNP CipherTextHiding feature support

[Ashish Kalra]

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
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 must be greater than MAX_SNP_ASID.

This patch-set adds two new module parameters to the KVM module
to enable SNP CipherTextHiding support and 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.

v3:
- rebase to linux-next.
- rebase on top of support to move SEV-SNP initialization to
KVM module from CCP driver.
- Split CipherTextHiding support between CCP driver and KVM module
with KVM module calling into CCP driver to initialize SNP with
CipherTextHiding enabled and MAX ASID usable for SNP guest if
KVM is enabling CipherTextHiding feature.
- Move module parameters to enable CipherTextHiding feature and
MAX ASID usable for SNP guests from CCP driver to KVM module
which allows KVM to be responsible for enabling CipherTextHiding
feature if end-user requests it.

v2:
- Fix and add more description to commit logs.
- Rename sev_cache_snp_platform_status_and_discover_features() to 
snp_get_platform_data().
- Add check in snp_get_platform_data to guard against being called
after SNP_INIT_EX.
- Fix comments for new structure field definitions being added.
- Fix naming for new structure being added.
- Add new vm-type parameter to sev_asid_new().
- Fix identation.
- Rename CCP module parameters psp_cth_enabled to cipher_text_hiding and 
psp_max_snp_asid to max_snp_asid.
- Rename max_snp_asid to snp_max_snp_asid. 

Ashish Kalra (4):
  crypto: ccp: New bit-field definitions for SNP_PLATFORM_STATUS command
  crypto: ccp: Add support for SNP_FEATURE_INFO command
  crypto: ccp: Add support to enable CipherTextHiding on SNP_INIT_EX
  KVM: SVM: Add SEV-SNP CipherTextHiding support

 arch/x86/kvm/svm/sev.c       | 50 ++++++++++++++++++++---
 drivers/crypto/ccp/sev-dev.c | 78 ++++++++++++++++++++++++++++++++++--
 drivers/crypto/ccp/sev-dev.h |  3 ++
 include/linux/psp-sev.h      | 47 +++++++++++++++++++++-
 include/uapi/linux/psp-sev.h | 10 ++++-
 5 files changed, 177 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] crypto: ccp: New bit-field definitions for SNP_PLATFORM_STATUS command

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Define new bit-field definitions returned by SNP_PLATFORM_STATUS command
such as new capabilities like SNP_FEATURE_INFO command availability,
ciphertext hiding enabled and capability.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/uapi/linux/psp-sev.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
index eeb20dfb1fda..c2fd324623c4 100644
--- a/include/uapi/linux/psp-sev.h
+++ b/include/uapi/linux/psp-sev.h
@@ -185,6 +185,10 @@ struct sev_user_data_get_id2 {
  * @mask_chip_id: whether chip id is present in attestation reports or not
  * @mask_chip_key: whether attestation reports are signed or not
  * @vlek_en: VLEK (Version Loaded Endorsement Key) hashstick is loaded
+ * @feature_info: whether SNP_FEATURE_INFO command is available
+ * @rapl_dis: whether RAPL is disabled
+ * @ciphertext_hiding_cap: whether platform has ciphertext hiding capability
+ * @ciphertext_hiding_en: whether ciphertext hiding is enabled
  * @rsvd1: reserved
  * @guest_count: the number of guest currently managed by the firmware
  * @current_tcb_version: current TCB version
@@ -200,7 +204,11 @@ struct sev_user_data_snp_status {
 	__u32 mask_chip_id:1;		/* Out */
 	__u32 mask_chip_key:1;		/* Out */
 	__u32 vlek_en:1;		/* Out */
-	__u32 rsvd1:29;
+	__u32 feature_info:1;		/* Out */
+	__u32 rapl_dis:1;		/* Out */
+	__u32 ciphertext_hiding_cap:1;	/* Out */
+	__u32 ciphertext_hiding_en:1;	/* Out */
+	__u32 rsvd1:25;
 	__u32 guest_count;		/* Out */
 	__u64 current_tcb_version;	/* Out */
 	__u64 reported_tcb_version;	/* Out */
-- 
2.34.1

[PATCH v3 2/4] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

The FEATURE_INFO command provides host and guests a programmatic means
to learn about the supported features of the currently loaded firmware.
FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
Instead of using the CPUID instruction to retrieve Fn8000_0024,
software can use FEATURE_INFO.

Host/Hypervisor would use the FEATURE_INFO command, while guests would
actually issue the CPUID instruction.

The hypervisor can provide Fn8000_0024 values to the guest via the CPUID
page in SNP_LAUNCH_UPDATE. As with all CPUID output recorded in that page,
the hypervisor can filter Fn8000_0024. The firmware will examine
Fn8000_0024 and apply its CPUID policy.

During CCP module initialization, after firmware update, the SNP
platform status and feature information from CPUID 0x8000_0024,
sub-function 0, are cached in the sev_device structure.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 47 ++++++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sev-dev.h |  3 +++
 include/linux/psp-sev.h      | 29 ++++++++++++++++++++++
 3 files changed, 79 insertions(+)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index b08db412f752..f4f8a8905115 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -232,6 +232,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct snp_feature_info);
 	default:				return 0;
 	}
 
@@ -1072,6 +1073,50 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrq(MSR_VM_HSAVE_PA, 0);
 }
 
+static void snp_get_platform_data(void)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct sev_data_snp_feature_info snp_feat_info;
+	struct snp_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	int error = 0, rc;
+
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return;
+
+	/*
+	 * The output buffer must be firmware page if SEV-SNP is
+	 * initialized.
+	 */
+	if (sev->snp_initialized)
+		return;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!rc && sev->snp_plat_status.feature_info) {
+		/*
+		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+		 * command to handle any alignment and page boundary check
+		 * requirements.
+		 */
+		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
+		snp_feat_info.length = sizeof(snp_feat_info);
+		snp_feat_info.ecx_in = 0;
+		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
+		if (!rc)
+			sev->feat_info = *feat_info;
+		kfree(feat_info);
+	}
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;
@@ -2543,6 +2588,8 @@ void sev_pci_init(void)
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);
 
+	snp_get_platform_data();
+
 	return;
 
 err:
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index 3e4e5574e88a..1c1a51e52d2b 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -57,6 +57,9 @@ struct sev_device {
 	bool cmd_buf_backup_active;
 
 	bool snp_initialized;
+
+	struct sev_user_data_snp_status snp_plat_status;
+	struct snp_feature_info feat_info;
 };
 
 int sev_dev_init(struct psp_device *psp);
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 0b3a36bdaa90..0149d4a6aceb 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -107,6 +107,7 @@ enum sev_cmd {
 	SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA,
 	SEV_CMD_SNP_COMMIT		= 0x0CB,
 	SEV_CMD_SNP_VLEK_LOAD		= 0x0CD,
+	SEV_CMD_SNP_FEATURE_INFO	= 0x0CE,
 
 	SEV_CMD_MAX,
 };
@@ -812,6 +813,34 @@ struct sev_data_snp_commit {
 	u32 len;
 } __packed;
 
+/**
+ * struct sev_data_snp_feature_info - SEV_SNP_FEATURE_INFO structure
+ *
+ * @length: len of the command buffer read by the PSP
+ * @ecx_in: subfunction index
+ * @feature_info_paddr : SPA of the FEATURE_INFO structure
+ */
+struct sev_data_snp_feature_info {
+	u32 length;
+	u32 ecx_in;
+	u64 feature_info_paddr;
+} __packed;
+
+/**
+ * struct feature_info - FEATURE_INFO structure
+ *
+ * @eax: output of SNP_FEATURE_INFO command
+ * @ebx: output of SNP_FEATURE_INFO command
+ * @ecx: output of SNP_FEATURE_INFO command
+ * #edx: output of SNP_FEATURE_INFO command
+ */
+struct snp_feature_info {
+	u32 eax;
+	u32 ebx;
+	u32 ecx;
+	u32 edx;
+} __packed;
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**
-- 
2.34.1

[PATCH v3 3/4] crypto: ccp: Add support to enable CipherTextHiding on SNP_INIT_EX

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Ciphertext hiding needs to be enabled on SNP_INIT_EX.

Add two new arguments to sev_platform_init_args to allow KVM
module to specify during SNP initialization if CipherTextHiding
feature is to be enabled and the maximum ASID usable for an
SEV-SNP guest when CipherTextHiding feature is enabled.

Add new API interface to indicate if SEV-SNP CipherTextHiding
feature is supported and enabled in the Platform/BIOS.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 31 ++++++++++++++++++++++++++++---
 include/linux/psp-sev.h      | 18 ++++++++++++++++--
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index f4f8a8905115..ca4b156598de 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1073,6 +1073,25 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrq(MSR_VM_HSAVE_PA, 0);
 }
 
+bool is_sev_snp_ciphertext_hiding_supported(void)
+{
+	struct psp_device *psp = psp_master;
+	struct sev_device *sev;
+
+	sev = psp->sev_data;
+
+	/*
+	 * Check if CipherTextHiding feature is supported and enabled
+	 * in the Platform/BIOS.
+	 */
+	if ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
+	    sev->snp_plat_status.ciphertext_hiding_cap)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(is_sev_snp_ciphertext_hiding_supported);
+
 static void snp_get_platform_data(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
@@ -1147,7 +1166,7 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 	return 0;
 }
 
-static int __sev_snp_init_locked(int *error)
+static int __sev_snp_init_locked(int *error, bool cipher_text_hiding_en, unsigned int snp_max_snp_asid)
 {
 	struct psp_device *psp = psp_master;
 	struct sev_data_snp_init_ex data;
@@ -1208,6 +1227,12 @@ static int __sev_snp_init_locked(int *error)
 		}
 
 		memset(&data, 0, sizeof(data));
+
+		if (cipher_text_hiding_en) {
+			data.ciphertext_hiding_en = 1;
+			data.max_snp_asid = snp_max_snp_asid;
+		}
+
 		data.init_rmp = 1;
 		data.list_paddr_en = 1;
 		data.list_paddr = __psp_pa(snp_range_list);
@@ -1392,7 +1417,7 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 	if (sev->state == SEV_STATE_INIT)
 		return 0;
 
-	rc = __sev_snp_init_locked(&args->error);
+	rc = __sev_snp_init_locked(&args->error, args->cipher_text_hiding_en, args->snp_max_snp_asid);
 	if (rc && rc != -ENODEV)
 		return rc;
 
@@ -1475,7 +1500,7 @@ static int snp_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_req
 {
 	int error, rc;
 
-	rc = __sev_snp_init_locked(&error);
+	rc = __sev_snp_init_locked(&error, false, 0);
 	if (rc) {
 		argp->error = SEV_RET_INVALID_PLATFORM_STATE;
 		return rc;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 0149d4a6aceb..af45e3e372f5 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -746,10 +746,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;
 
 /**
@@ -798,10 +801,16 @@ struct sev_data_snp_shutdown_ex {
  * @probe: True if this is being called as part of CCP module probe, which
  *  will defer SEV_INIT/SEV_INIT_EX firmware initialization until needed
  *  unless psp_init_on_probe module param is set
+ *  @cipher_text_hiding_en: True if SEV-SNP CipherTextHiding support is
+ *  enabled
+ *  @snp_max_snp_asid: maximum ASID usable for SEV-SNP guest if
+ *  CipherTextHiding is enabled
  */
 struct sev_platform_init_args {
 	int error;
 	bool probe;
+	bool cipher_text_hiding_en;
+	unsigned int snp_max_snp_asid;
 };
 
 /**
@@ -841,6 +850,8 @@ struct snp_feature_info {
 	u32 edx;
 } __packed;
 
+#define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**
@@ -984,6 +995,7 @@ void *psp_copy_user_blob(u64 uaddr, u32 len);
 void *snp_alloc_firmware_page(gfp_t mask);
 void snp_free_firmware_page(void *addr);
 void sev_platform_shutdown(void);
+bool is_sev_snp_ciphertext_hiding_supported(void);
 
 #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
 
@@ -1020,6 +1032,8 @@ static inline void snp_free_firmware_page(void *addr) { }
 
 static inline void sev_platform_shutdown(void) { }
 
+static inline bool is_sev_snp_ciphertext_hiding_supported(void) { return FALSE; }
+
 #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
 
 #endif	/* __PSP_SEV_H__ */
-- 
2.34.1

[PATCH v3 4/4] KVM: SVM: Add SEV-SNP CipherTextHiding support

[Ashish Kalra]

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 two new module parameters to the KVM module, first
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.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.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");
+
 #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
+	 * SNP_INIT_EX command and all the SEV-ES guests must have
+	 * an ASID greater than MAX_SNP_ASID.
+	 */
+	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;
+	}
 again:
 	asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid);
 	if (asid > max_asid) {
@@ -438,7 +460,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (vm_type == KVM_X86_SNP_VM)
 		sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
 
-	ret = sev_asid_new(sev);
+	ret = sev_asid_new(sev, vm_type);
 	if (ret)
 		goto e_no_asid;
 
@@ -3005,6 +3027,18 @@ void __init sev_hardware_setup(void)
 	if (!sev_es_enabled)
 		goto out;
 
+	if (cipher_text_hiding && is_sev_snp_ciphertext_hiding_supported()) {
+		/* Do sanity checks on user-defined MAX_SNP_ASID */
+		if (max_snp_asid >= edx) {
+			pr_info("max_snp_asid module parameter is not valid, limiting to %d\n",
+				 edx - 1);
+			max_snp_asid = edx - 1;
+		}
+		snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2;
+		snp_cipher_text_hiding = true;
+		pr_info("SEV-SNP CipherTextHiding feature support enabled\n");
+	}
+
 	/*
 	 * SEV-ES requires MMIO caching as KVM doesn't have access to the guest
 	 * instruction stream, i.e. can't emulate in response to a #NPF and
@@ -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);
 
 	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

Re: [PATCH v3 4/4] KVM: SVM: Add SEV-SNP CipherTextHiding support

[Sean Christopherson]

On Tue, Apr 22, 2025, 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.

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@amd.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
> 

Re: [PATCH v3 2/4] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Tom Lendacky]

On 4/21/25 19:24, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> The FEATURE_INFO command provides host and guests a programmatic means
> to learn about the supported features of the currently loaded firmware.
> FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
> Instead of using the CPUID instruction to retrieve Fn8000_0024,
> software can use FEATURE_INFO.
> 
> Host/Hypervisor would use the FEATURE_INFO command, while guests would
> actually issue the CPUID instruction.

You probably want to word this better in combination with the next
paragraph. The actual CPUID leaf doesn't exist. The hypervisor must
populate the entry in the CPUID page during LAUNCH_UPDATE in order for
the CPUID instruction in the guest to return a value.

> 
> The hypervisor can provide Fn8000_0024 values to the guest via the CPUID
> page in SNP_LAUNCH_UPDATE. As with all CPUID output recorded in that page,
> the hypervisor can filter Fn8000_0024. The firmware will examine
> Fn8000_0024 and apply its CPUID policy.
> 
> During CCP module initialization, after firmware update, the SNP
> platform status and feature information from CPUID 0x8000_0024,
> sub-function 0, are cached in the sev_device structure.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 47 ++++++++++++++++++++++++++++++++++++
>  drivers/crypto/ccp/sev-dev.h |  3 +++
>  include/linux/psp-sev.h      | 29 ++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index b08db412f752..f4f8a8905115 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -232,6 +232,7 @@ static int sev_cmd_buffer_len(int cmd)
>  	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
>  	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
>  	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
> +	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct snp_feature_info);
>  	default:				return 0;
>  	}
>  
> @@ -1072,6 +1073,50 @@ static void snp_set_hsave_pa(void *arg)
>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>  }
>  
> +static void snp_get_platform_data(void)
> +{
> +	struct sev_device *sev = psp_master->sev_data;
> +	struct sev_data_snp_feature_info snp_feat_info;
> +	struct snp_feature_info *feat_info;
> +	struct sev_data_snp_addr buf;
> +	int error = 0, rc;
> +
> +	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> +		return;
> +
> +	/*
> +	 * The output buffer must be firmware page if SEV-SNP is
> +	 * initialized.
> +	 */
> +	if (sev->snp_initialized)
> +		return;
> +
> +	buf.address = __psp_pa(&sev->snp_plat_status);
> +	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);

See comment below...

> +
> +	/*
> +	 * Do feature discovery of the currently loaded firmware,
> +	 * and cache feature information from CPUID 0x8000_0024,
> +	 * sub-function 0.
> +	 */
> +	if (!rc && sev->snp_plat_status.feature_info) {
> +		/*
> +		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
> +		 * command to handle any alignment and page boundary check
> +		 * requirements.
> +		 */
> +		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);

Need to check for NULL.

> +		snp_feat_info.length = sizeof(snp_feat_info);
> +		snp_feat_info.ecx_in = 0;
> +		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
> +
> +		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
> +		if (!rc)
> +			sev->feat_info = *feat_info;

Should probably issue a message if the command fails.

> +		kfree(feat_info);
> +	}
> +}
> +
>  static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>  {
>  	struct sev_data_range_list *range_list = arg;
> @@ -2543,6 +2588,8 @@ void sev_pci_init(void)
>  			 api_major, api_minor, build,
>  			 sev->api_major, sev->api_minor, sev->build);
>  
> +	snp_get_platform_data();

We should switch from using SEV platform status to SNP platform status
(when SNP is available) at the beginning of sev_pci_init() and cache the
results. Then you won't have to issue another platform status command in
snp_get_platform_data().

Thanks,
Tom

> +
>  	return;
>  
>  err:
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 3e4e5574e88a..1c1a51e52d2b 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -57,6 +57,9 @@ struct sev_device {
>  	bool cmd_buf_backup_active;
>  
>  	bool snp_initialized;
> +
> +	struct sev_user_data_snp_status snp_plat_status;
> +	struct snp_feature_info feat_info;
>  };
>  
>  int sev_dev_init(struct psp_device *psp);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 0b3a36bdaa90..0149d4a6aceb 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -107,6 +107,7 @@ enum sev_cmd {
>  	SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA,
>  	SEV_CMD_SNP_COMMIT		= 0x0CB,
>  	SEV_CMD_SNP_VLEK_LOAD		= 0x0CD,
> +	SEV_CMD_SNP_FEATURE_INFO	= 0x0CE,
>  
>  	SEV_CMD_MAX,
>  };
> @@ -812,6 +813,34 @@ struct sev_data_snp_commit {
>  	u32 len;
>  } __packed;
>  
> +/**
> + * struct sev_data_snp_feature_info - SEV_SNP_FEATURE_INFO structure
> + *
> + * @length: len of the command buffer read by the PSP
> + * @ecx_in: subfunction index
> + * @feature_info_paddr : SPA of the FEATURE_INFO structure
> + */
> +struct sev_data_snp_feature_info {
> +	u32 length;
> +	u32 ecx_in;
> +	u64 feature_info_paddr;
> +} __packed;
> +
> +/**
> + * struct feature_info - FEATURE_INFO structure
> + *
> + * @eax: output of SNP_FEATURE_INFO command
> + * @ebx: output of SNP_FEATURE_INFO command
> + * @ecx: output of SNP_FEATURE_INFO command
> + * #edx: output of SNP_FEATURE_INFO command
> + */
> +struct snp_feature_info {
> +	u32 eax;
> +	u32 ebx;
> +	u32 ecx;
> +	u32 edx;
> +} __packed;
> +
>  #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>  
>  /**

Re: [PATCH v3 3/4] crypto: ccp: Add support to enable CipherTextHiding on SNP_INIT_EX

[Tom Lendacky]

On 4/21/25 19:25, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Ciphertext hiding needs to be enabled on SNP_INIT_EX.
> 
> Add two new arguments to sev_platform_init_args to allow KVM
> module to specify during SNP initialization if CipherTextHiding
> feature is to be enabled and the maximum ASID usable for an
> SEV-SNP guest when CipherTextHiding feature is enabled.
> 
> Add new API interface to indicate if SEV-SNP CipherTextHiding
> feature is supported and enabled in the Platform/BIOS.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 31 ++++++++++++++++++++++++++++---
>  include/linux/psp-sev.h      | 18 ++++++++++++++++--
>  2 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index f4f8a8905115..ca4b156598de 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1073,6 +1073,25 @@ static void snp_set_hsave_pa(void *arg)
>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>  }
>  
> +bool is_sev_snp_ciphertext_hiding_supported(void)

sev_is_snp_ciphertext_hiding_supported

> +{
> +	struct psp_device *psp = psp_master;
> +	struct sev_device *sev;
> +
> +	sev = psp->sev_data;
> +
> +	/*
> +	 * Check if CipherTextHiding feature is supported and enabled
> +	 * in the Platform/BIOS.

I think this should be expanded a bit to indicate why both the feature
info and the platform status fields have to be checked.

> +	 */
> +	if ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
> +	    sev->snp_plat_status.ciphertext_hiding_cap)
> +		return true;
> +
> +	return false;

And then just make this:

  return (sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
	 sev->snp_plat_status.ciphertext_hiding_cap);

> +}
> +EXPORT_SYMBOL_GPL(is_sev_snp_ciphertext_hiding_supported);
> +
>  static void snp_get_platform_data(void)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> @@ -1147,7 +1166,7 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>  	return 0;
>  }
>  
> -static int __sev_snp_init_locked(int *error)
> +static int __sev_snp_init_locked(int *error, bool cipher_text_hiding_en, unsigned int snp_max_snp_asid)
>  {
>  	struct psp_device *psp = psp_master;
>  	struct sev_data_snp_init_ex data;
> @@ -1208,6 +1227,12 @@ static int __sev_snp_init_locked(int *error)
>  		}
>  
>  		memset(&data, 0, sizeof(data));
> +
> +		if (cipher_text_hiding_en) {
> +			data.ciphertext_hiding_en = 1;
> +			data.max_snp_asid = snp_max_snp_asid;
> +		}
> +
>  		data.init_rmp = 1;
>  		data.list_paddr_en = 1;
>  		data.list_paddr = __psp_pa(snp_range_list);
> @@ -1392,7 +1417,7 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
>  	if (sev->state == SEV_STATE_INIT)
>  		return 0;
>  
> -	rc = __sev_snp_init_locked(&args->error);
> +	rc = __sev_snp_init_locked(&args->error, args->cipher_text_hiding_en, args->snp_max_snp_asid);
>  	if (rc && rc != -ENODEV)
>  		return rc;
>  
> @@ -1475,7 +1500,7 @@ static int snp_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_req
>  {
>  	int error, rc;
>  
> -	rc = __sev_snp_init_locked(&error);
> +	rc = __sev_snp_init_locked(&error, false, 0);
>  	if (rc) {
>  		argp->error = SEV_RET_INVALID_PLATFORM_STATE;
>  		return rc;
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 0149d4a6aceb..af45e3e372f5 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -746,10 +746,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;
>  
>  /**
> @@ -798,10 +801,16 @@ struct sev_data_snp_shutdown_ex {
>   * @probe: True if this is being called as part of CCP module probe, which
>   *  will defer SEV_INIT/SEV_INIT_EX firmware initialization until needed
>   *  unless psp_init_on_probe module param is set
> + *  @cipher_text_hiding_en: True if SEV-SNP CipherTextHiding support is

s/is/is to be/

Thanks,
Tom

> + *  enabled
> + *  @snp_max_snp_asid: maximum ASID usable for SEV-SNP guest if
> + *  CipherTextHiding is enabled
>   */
>  struct sev_platform_init_args {
>  	int error;
>  	bool probe;
> +	bool cipher_text_hiding_en;
> +	unsigned int snp_max_snp_asid;
>  };
>  
>  /**
> @@ -841,6 +850,8 @@ struct snp_feature_info {
>  	u32 edx;
>  } __packed;
>  
> +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)
> +
>  #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>  
>  /**
> @@ -984,6 +995,7 @@ void *psp_copy_user_blob(u64 uaddr, u32 len);
>  void *snp_alloc_firmware_page(gfp_t mask);
>  void snp_free_firmware_page(void *addr);
>  void sev_platform_shutdown(void);
> +bool is_sev_snp_ciphertext_hiding_supported(void);
>  
>  #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
>  
> @@ -1020,6 +1032,8 @@ static inline void snp_free_firmware_page(void *addr) { }
>  
>  static inline void sev_platform_shutdown(void) { }
>  
> +static inline bool is_sev_snp_ciphertext_hiding_supported(void) { return FALSE; }
> +
>  #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
>  
>  #endif	/* __PSP_SEV_H__ */

Re: [PATCH v3 2/4] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Francesco Lavra]

On 2025-04-22 at 0:24, Ashish Kalra wrote:
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-
> dev.c
> index b08db412f752..f4f8a8905115 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -232,6 +232,7 @@ static int sev_cmd_buffer_len(int cmd)
>  	case SEV_CMD_SNP_GUEST_REQUEST:		return
> sizeof(struct sev_data_snp_guest_request);
>  	case SEV_CMD_SNP_CONFIG:		return sizeof(struct
> sev_user_data_snp_config);
>  	case SEV_CMD_SNP_COMMIT:		return sizeof(struct
> sev_data_snp_commit);
> +	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct
> snp_feature_info);

This function is supposed to return the size of the command buffer, so
for this command it should return sizeof(struct
sev_data_snp_feature_info).

>  	default:				return 0;
>  	}
>  
> @@ -1072,6 +1073,50 @@ static void snp_set_hsave_pa(void *arg)
>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>  }
>  
> +static void snp_get_platform_data(void)
> +{
> +	struct sev_device *sev = psp_master->sev_data;
> +	struct sev_data_snp_feature_info snp_feat_info;
> +	struct snp_feature_info *feat_info;
> +	struct sev_data_snp_addr buf;
> +	int error = 0, rc;
> +
> +	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> +		return;
> +
> +	/*
> +	 * The output buffer must be firmware page if SEV-SNP is
> +	 * initialized.
> +	 */
> +	if (sev->snp_initialized)
> +		return;
> +
> +	buf.address = __psp_pa(&sev->snp_plat_status);
> +	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf,
> &error);
> +
> +	/*
> +	 * Do feature discovery of the currently loaded firmware,
> +	 * and cache feature information from CPUID 0x8000_0024,
> +	 * sub-function 0.
> +	 */
> +	if (!rc && sev->snp_plat_status.feature_info) {
> +		/*
> +		 * Use dynamically allocated structure for the
> SNP_FEATURE_INFO
> +		 * command to handle any alignment and page boundary
> check
> +		 * requirements.
> +		 */
> +		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);

The SEV firmware requires the supplied memory range to not cross a page
boundary, but kzalloc() does not guarantee that the allocated memory
fits this requirement. You need to allocate a larger chunk of memory (2
* sizeof(*feat_info) will be enough), and possibly set
feature_info_paddr to an offset from the start of the allocated memory.

Re: [PATCH v3 4/4] KVM: SVM: Add SEV-SNP CipherTextHiding support

[Kalra, Ashish]

Hello Sean,

On 4/23/2025 4:15 PM, Sean Christopherson wrote:
> On Tue, Apr 22, 2025, 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.
> 
> 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+.
>>

But the above statement is practically correct, once CTH is enabled, 
SNP guests must have ASIDs less than or equal to MAX_SNP_ASID and SEV and SEV-ES
have to use ASIDs greater than MAX_SNP_ASID. 

And yes, CTH basically splits the SEV-ES ASID space further 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.
> 

What i really mean is that if CTH support is enabled and this MAX_SNP_ASID
is not defined by the user then the ASID space is equally divided between SNP and SEV-ES.

> 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@amd.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.

Yes, that's what i said above. 

But i do agree it is confusing and misleading.

> 
> 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.

Ok, that makes sense.

Right, split the SEV-ES+ ASID space between SNP and SEV-ES.  

> 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:

Where ?

> 
> 	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
> 		if (snp_max_snp_asid >= (min_sev_asid - 1))
> 			sev_es_supported = false;

SEV-ES is disabled if SNP is using all ASIDs upto min_sev_asid - 1.

> 		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.

I don't see above where SEV-ES is broken if snp_max_snp_asid is non-zero and CTH is enabled ?

If snp_max_snp_asid == min_sev_asid-1, then SEV-ES is going to be disabled, right ?

> 
>>  #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.
>

Ok.

>> +	 */
>> +	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. 

param ?

From what i see with your suggestions below, you are suggesting adding new
{min,max}snp/sev_es/sev_asid to track min and max ASIDs for SNP, SEV-ES
and SEV separately. 

> 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.
> 	 */

Ok, makes sense.

> 	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.
> 

Ok.

Thanks,
Ashish

>>  
>>  	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
>>

Re: [PATCH v3 4/4] KVM: SVM: Add SEV-SNP CipherTextHiding support

[Sean Christopherson]

On Fri, Apr 25, 2025, Ashish Kalra wrote:
> On 4/23/2025 4:15 PM, Sean Christopherson wrote:
> 
> > 
> > 	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
> > 		if (snp_max_snp_asid >= (min_sev_asid - 1))
> > 			sev_es_supported = false;
> 
> SEV-ES is disabled if SNP is using all ASIDs upto min_sev_asid - 1.
> 
> > 		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.
> 
> I don't see above where SEV-ES is broken if snp_max_snp_asid is non-zero and
> CTH is enabled ?

Please read what I wrote.  I did not say it's broken if CTH is enabled.  I said
it's broken if CTH isn't supported, i.e. is disabled.

snp_max_snp_asid isn't sanitized if CTH is unsupported or disabled by userspace,
and so KVM will compute the wrong min_sev_asid if snp_max_snp_asid is non-zero,
even though snp_max_snp_asid has no bearing on reality.

> >> +	 */
> >> +	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. 
> 
> param ?

Sorry, s/param/variable.  Doesn't need to be user visible.

Re: [PATCH v3 3/4] crypto: ccp: Add support to enable CipherTextHiding on SNP_INIT_EX

[kernel test robot]

Hi Ashish,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20250417]
[cannot apply to herbert-cryptodev-2.6/master herbert-crypto-2.6/master kvm/queue kvm/next linus/master kvm/linux-next v6.15-rc3 v6.15-rc2 v6.15-rc1 v6.15-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ashish-Kalra/crypto-ccp-New-bit-field-definitions-for-SNP_PLATFORM_STATUS-command/20250422-082725
base:   next-20250417
patch link:    https://lore.kernel.org/r/94ffa7595fca67cfdcd2352354791bdb6ac00499.1745279916.git.ashish.kalra%40amd.com
patch subject: [PATCH v3 3/4] crypto: ccp: Add support to enable CipherTextHiding on SNP_INIT_EX
config: i386-buildonly-randconfig-002-20250422 (https://download.01.org/0day-ci/archive/20250507/202505071309.cJl7zfy2-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250507/202505071309.cJl7zfy2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505071309.cJl7zfy2-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/x86/kvm/svm/svm.c:24:
>> include/linux/psp-sev.h:1035:74: error: use of undeclared identifier 'FALSE'
    1035 | static inline bool is_sev_snp_ciphertext_hiding_supported(void) { return FALSE; }
         |                                                                          ^
   1 error generated.


vim +/FALSE +1035 include/linux/psp-sev.h

  1034	
> 1035	static inline bool is_sev_snp_ciphertext_hiding_supported(void) { return FALSE; }
  1036	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki