[PATCH v4 0/5] Add SEV-SNP CipherTextHiding feature support

[PATCH v4 0/5] 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).

The SEV ASID space is basically split into legacy SEV and SEV-ES+.
CipherTextHiding further partitions the SEV-ES+ ASID space into SEV-ES
and SEV-SNP.

Add new module parameter to the KVM module to enable CipherTextHiding
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.

v4:
- Fix buffer allocation for SNP_FEATURE_INFO command to correctly
handle page boundary check requirements.
- Return correct length for SNP_FEATURE_INFO command from
sev_cmd_buffer_len().
- Switch to using SNP platform status instead of SEV platform status if
SNP is enabled and cache SNP platform status and feature information.
Modify sev_get_api_version() accordingly.
- Fix commit logs.
- Expand the comments on why both the feature info and the platform
status fields have to be checked for CipherTextHiding feature 
detection and enablement.
- Add new preperation patch for CipherTextHiding feature which
introduces new {min,max}_{sev_es,snp}_asid variables along with
existing {min,max}_sev_asid variable to simplify partitioning of the
SEV and SEV-ES+ ASID space.
- Switch to single KVM module parameter to enable CipherTextHiding
feature and the maximum SNP ASID usable for SNP guests when 
CipherTextHiding feature is enabled.

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 (5):
  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: SEV: Introduce new min,max sev_es and sev_snp asid variables
  KVM: SEV: Add SEV-SNP CipherTextHiding support

 .../admin-guide/kernel-parameters.txt         |  10 ++
 arch/x86/kvm/svm/sev.c                        |  68 +++++++++--
 drivers/crypto/ccp/sev-dev.c                  | 111 +++++++++++++++++-
 drivers/crypto/ccp/sev-dev.h                  |   3 +
 include/linux/psp-sev.h                       |  44 ++++++-
 include/uapi/linux/psp-sev.h                  |  10 +-
 6 files changed, 231 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH v4 1/5] 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 v4 2/5] 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.

The hypervisor may 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.

Switch to using SNP platform status instead of SEV platform status if
SNP is enabled and cache SNP platform status and feature information
from CPUID 0x8000_0024, sub-function 0, in the sev_device structure.

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

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 3451bada884e..b642f1183b8b 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -233,6 +233,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 sev_data_snp_feature_info);
 	default:				return 0;
 	}
 
@@ -1073,6 +1074,69 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrq(MSR_VM_HSAVE_PA, 0);
 }
 
+static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
+{
+	struct sev_data_snp_feature_info snp_feat_info;
+	struct sev_device *sev = psp_master->sev_data;
+	struct snp_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	struct page *page;
+	int rc;
+
+	/*
+	 * The output buffer must be firmware page if SEV-SNP is
+	 * initialized.
+	 */
+	if (sev->snp_initialized)
+		return -EINVAL;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
+
+	if (rc) {
+		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
+			rc, *error);
+		return rc;
+	}
+
+	status->api_major = sev->snp_plat_status.api_major;
+	status->api_minor = sev->snp_plat_status.api_minor;
+	status->build = sev->snp_plat_status.build_id;
+	status->state = sev->snp_plat_status.state;
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (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.
+		 */
+		page = alloc_page(GFP_KERNEL);
+		if (!page)
+			return -ENOMEM;
+		feat_info = page_address(page);
+		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;
+		else
+			dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
+				rc, *error);
+
+		__free_page(page);
+	}
+
+	return rc;
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;
@@ -1597,6 +1661,23 @@ static int sev_get_api_version(void)
 	struct sev_user_data_status status;
 	int error = 0, ret;
 
+	/*
+	 * Use SNP platform status if SNP is enabled and cache
+	 * SNP platform status and SNP feature information.
+	 */
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
+		ret = snp_get_platform_data(&status, &error);
+		if (ret) {
+			dev_err(sev->dev,
+				"SEV-SNP: failed to get status. Error: %#x\n", error);
+			return 1;
+		}
+	}
+
+	/*
+	 * Fallback to SEV platform status if SNP is not enabled
+	 * or SNP platform status fails.
+	 */
 	ret = sev_platform_status(&status, &error);
 	if (ret) {
 		dev_err(sev->dev,
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 v4 3/5] 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 new argument 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 by SEV firmware and additionally if
CipherTextHiding feature is enabled in the Platform BIOS.

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

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index b642f1183b8b..185668477182 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1074,6 +1074,24 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrq(MSR_VM_HSAVE_PA, 0);
 }
 
+bool sev_is_snp_ciphertext_hiding_supported(void)
+{
+	struct psp_device *psp = psp_master;
+	struct sev_device *sev;
+
+	sev = psp->sev_data;
+
+	/*
+	 * Feature information indicates if CipherTextHiding feature is
+	 * supported by the SEV firmware and additionally platform status
+	 * indicates if CipherTextHiding feature is enabled in the
+	 * Platform BIOS.
+	 */
+	return ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
+	    sev->snp_plat_status.ciphertext_hiding_cap);
+}
+EXPORT_SYMBOL_GPL(sev_is_snp_ciphertext_hiding_supported);
+
 static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
 {
 	struct sev_data_snp_feature_info snp_feat_info;
@@ -1167,7 +1185,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, unsigned int snp_max_snp_asid)
 {
 	struct psp_device *psp = psp_master;
 	struct sev_data_snp_init_ex data;
@@ -1228,6 +1246,12 @@ static int __sev_snp_init_locked(int *error)
 		}
 
 		memset(&data, 0, sizeof(data));
+
+		if (snp_max_snp_asid) {
+			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);
@@ -1412,7 +1436,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->snp_max_snp_asid);
 	if (rc && rc != -ENODEV)
 		return rc;
 
@@ -1495,7 +1519,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, 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..66fecd0c0f88 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,13 @@ 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
+ *  @snp_max_snp_asid: maximum ASID usable for SEV-SNP guest if
+ *  CipherTextHiding feature is to be enabled
  */
 struct sev_platform_init_args {
 	int error;
 	bool probe;
+	unsigned int snp_max_snp_asid;
 };
 
 /**
@@ -841,6 +847,8 @@ struct snp_feature_info {
 	u32 edx;
 } __packed;
 
+#define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**
@@ -984,6 +992,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 sev_is_snp_ciphertext_hiding_supported(void);
 
 #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
 
@@ -1020,6 +1029,8 @@ static inline void snp_free_firmware_page(void *addr) { }
 
 static inline void sev_platform_shutdown(void) { }
 
+static inline bool sev_is_snp_ciphertext_hiding_supported(void) { return false; }
+
 #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
 
 #endif	/* __PSP_SEV_H__ */
-- 
2.34.1

[PATCH v4 4/5] KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables

[Ashish Kalra]

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

Introduce new min, max sev_es_asid and sev_snp_asid variables.

The new {min,max}_{sev_es,snp}_asid variables along with existing
{min,max}_sev_asid variable simplifies partitioning of the
SEV and SEV-ES+ ASID space.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/kvm/svm/sev.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index dea9480b9ff6..383db1da8699 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -85,6 +85,10 @@ static DECLARE_RWSEM(sev_deactivate_lock);
 static DEFINE_MUTEX(sev_bitmap_lock);
 unsigned int max_sev_asid;
 static unsigned int min_sev_asid;
+static unsigned int max_sev_es_asid;
+static unsigned int min_sev_es_asid;
+static unsigned int max_snp_asid;
+static unsigned int min_snp_asid;
 static unsigned long sev_me_mask;
 static unsigned int nr_asids;
 static unsigned long *sev_asid_bitmap;
@@ -172,20 +176,32 @@ 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.
 	 * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
-	 * Note: min ASID can end up larger than the max if basic SEV support is
-	 * effectively disabled by disallowing use of ASIDs for SEV guests.
 	 */
-	unsigned int min_asid = sev->es_active ? 1 : min_sev_asid;
-	unsigned int max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid;
-	unsigned int asid;
+	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.
+	 */
+
 	if (min_asid > max_asid)
 		return -ENOTTY;
 
@@ -439,7 +455,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;
 
@@ -3029,6 +3045,9 @@ void __init sev_hardware_setup(void)
 		goto out;
 	}
 
+	min_sev_es_asid = min_snp_asid = 1;
+	max_sev_es_asid = max_snp_asid = min_sev_asid - 1;
+
 	/* Has the system been allocated ASIDs for SEV-ES? */
 	if (min_sev_asid == 1)
 		goto out;
@@ -3048,11 +3067,11 @@ void __init sev_hardware_setup(void)
 	if (boot_cpu_has(X86_FEATURE_SEV_ES))
 		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_es_asid, max_sev_es_asid);
 	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_snp_asid, max_snp_asid);
 
 	sev_enabled = sev_supported;
 	sev_es_enabled = sev_es_supported;
-- 
2.34.1

[PATCH v4 5/5] KVM: SEV: 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).

The SEV ASID space is basically split into legacy SEV and SEV-ES+.
CipherTextHiding further partitions the SEV-ES+ ASID space into SEV-ES
and SEV-SNP.

Add new module parameter to the KVM module to enable CipherTextHiding
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=
+			[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.
+
 	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.
+	 */
+	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;
+		}
+
+		min_sev_es_asid = ciphertext_hiding_nr_asids == -1 ? (min_sev_asid - 1) / 2 :
+				  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_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);
 }
 
-- 
2.34.1

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

[Kim Phillips]

[-- Attachment #1: Type: text/plain, Size: 10651 bytes --]

Hi Ashish,

On 5/19/25 6:56 PM, 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
> will see constant default values (0xff).
If I apply this on top of next-20250522, I get the following stacktrace,
i.e., this assertion failure:

static int sev_write_init_ex_file_if_required(int cmd_id)
{
         lockdep_assert_held(&sev_cmd_mutex);

Config attached.

Thanks,

Kim

[   34.653536] ------------[ cut here ]------------
[   34.653545] WARNING: CPU: 92 PID: 4581 at 
drivers/crypto/ccp/sev-dev.c:349 __sev_do_cmd_locked+0x7eb/0xb90 [ccp]
[   34.653570] Modules linked in: binfmt_misc rapl wmi_bmof kvm ast 
drm_client_lib drm_shmem_helper drm_kms_helper ccp(+) i2c_algo_bit 
i2c_piix4 k10temp i2c_smbus acpi_ipmi ipmi_si(+) ipmi_devintf 
ipmi_msghandler mac_hid sch_fq_codel dm_multipath drm efi_pstore 
nfnetlink dmi_sysfs ip_tables x_tables autofs4 btrfs blake2b_generic 
raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor 
async_tx xor raid6_pq raid1 raid0 linear dm_mirror dm_region_hash dm_log 
ghash_clmulni_intel nvme sha512_ssse3 ahci sha1_ssse3 libahci nvme_core 
wmi aesni_intel
[   34.653645] CPU: 92 UID: 0 PID: 4581 Comm: (udev-worker) Not tainted 
6.15.0-rc7-next-20250522+ #4 PREEMPT(voluntary) 
849304994a065362c1f65db9527c0b4292d5aea6
[   34.653651] Hardware name: AMD Corporation VOLCANO/VOLCANO, BIOS 
RVOT1005B 04/08/2025
[   34.653653] RIP: 0010:__sev_do_cmd_locked+0x7eb/0xb90 [ccp]
[   34.653661] Code: fa ff ff be ff ff ff ff 48 c7 c7 50 cd b1 c0 44 89 
85 70 ff ff ff e8 c4 fe f3 f3 44 8b 85 70 ff ff ff 85 c0 0f 85 e2 fd ff 
ff <0f> 0b e9 db fd ff ff 48 8b 05 57 aa 12 00 8b 0d 95 82 0c f5 48 c7
[   34.653664] RSP: 0018:ff51f9b5d9f37890 EFLAGS: 00010246
[   34.653668] RAX: 0000000000000000 RBX: 0000000000000083 RCX: 
0000000000000001
[   34.653671] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 
0000000000000246
[   34.653672] RBP: ff51f9b5d9f37940 R08: 0000000000000000 R09: 
0000000000000000
[   34.653674] R10: 0000000000000001 R11: 0000000000000001 R12: 
ff51f9b5d9f37954
[   34.653676] R13: ff3121dada778000 R14: 0000000000000000 R15: 
ff3121dadb5c5028
[   34.653677] FS:  00007f0ed64488c0(0000) GS:ff3121e9b1a00000(0000) 
knlGS:0000000000000000
[   34.653679] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   34.653681] CR2: 00005599a0790fc8 CR3: 0000000108cd8001 CR4: 
0000000000771ef0
[   34.653684] PKRU: 55555554
[   34.653686] Call Trace:
[   34.653687]  <TASK>
[   34.653701]  sev_get_api_version+0xb2/0x2b0 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.653714]  ? __pfx_sp_mod_init+0x10/0x10 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.653727]  sev_pci_init+0x4a/0x320 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.653733]  ? preempt_count_sub+0x50/0x80
[   34.653741]  ? _raw_write_unlock_irqrestore+0x53/0x90
[   34.653748]  ? __pfx_sp_mod_init+0x10/0x10 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.653756]  psp_pci_init+0x2f/0x50 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.653763]  sp_mod_init+0x32/0xff0 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.653770]  do_one_initcall+0x5f/0x3c0
[   34.653774]  ? __kmalloc_cache_noprof+0x331/0x430
[   34.653784]  do_init_module+0x68/0x260
[   34.653789]  load_module+0x22ea/0x2410
[   34.653803]  ? kernel_read_file+0x2a4/0x320
[   34.653811]  init_module_from_file+0x96/0xd0
[   34.653815]  ? init_module_from_file+0x96/0xd0
[   34.653825]  idempotent_init_module+0x117/0x330
[   34.653836]  __x64_sys_finit_module+0x6f/0xe0
[   34.653841]  x64_sys_call+0x1f9e/0x20c0
[   34.653844]  do_syscall_64+0x8d/0x2d0
[   34.653849]  ? local_clock_noinstr+0x12/0xc0
[   34.653855]  ? rcu_read_unlock+0x1b/0x70
[   34.653860]  ? sched_clock_noinstr+0xd/0x20
[   34.653864]  ? local_clock_noinstr+0x12/0xc0
[   34.653869]  ? exc_page_fault+0x95/0x230
[   34.653876]  ? irqentry_exit_to_user_mode+0xb1/0x1e0
[   34.653880]  ? irqentry_exit+0x6f/0xa0
[   34.653882]  ? exc_page_fault+0xb4/0x230
[   34.653886]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   34.653888] RIP: 0033:0x7f0ed632725d
[   34.653892] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 
48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48
[   34.653894] RSP: 002b:00007ffe599733b8 EFLAGS: 00000246 ORIG_RAX: 
0000000000000139
[   34.653897] RAX: ffffffffffffffda RBX: 00005599a07b4370 RCX: 
00007f0ed632725d
[   34.653899] RDX: 0000000000000000 RSI: 00007f0ed662507d RDI: 
0000000000000022
[   34.653901] RBP: 00007ffe59973470 R08: 0000000000000040 R09: 
00007ffe59973420
[   34.653902] R10: 00007f0ed6403b20 R11: 0000000000000246 R12: 
00007f0ed662507d
[   34.653903] R13: 0000000000020000 R14: 00005599a07b6020 R15: 
00005599a07b9230
[   34.653913]  </TASK>
[   34.653914] irq event stamp: 211387
[   34.653916] hardirqs last  enabled at (211393): [<ffffffffb37a6786>] 
__up_console_sem+0x86/0x90
[   34.653922] hardirqs last disabled at (211398): [<ffffffffb37a676b>] 
__up_console_sem+0x6b/0x90
[   34.653923] softirqs last  enabled at (209856): [<ffffffffb36e364f>] 
handle_softirqs+0x32f/0x410
[   34.653928] softirqs last disabled at (209833): [<ffffffffb36e3800>] 
__irq_exit_rcu+0xc0/0xf0
[   34.653932] ---[ end trace 0000000000000000 ]---
[   34.654388] ------------[ cut here ]------------
[   34.654391] WARNING: CPU: 92 PID: 4581 at 
drivers/crypto/ccp/sev-dev.c:349 __sev_do_cmd_locked+0x7eb/0xb90 [ccp]
[   34.654396] Modules linked in: binfmt_misc rapl wmi_bmof kvm ast 
drm_client_lib drm_shmem_helper drm_kms_helper ccp(+) i2c_algo_bit 
i2c_piix4 k10temp i2c_smbus acpi_ipmi ipmi_si(+) ipmi_devintf 
ipmi_msghandler mac_hid sch_fq_codel dm_multipath drm efi_pstore 
nfnetlink dmi_sysfs ip_tables x_tables autofs4 btrfs blake2b_generic 
raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor 
async_tx xor raid6_pq raid1 raid0 linear dm_mirror dm_region_hash dm_log 
ghash_clmulni_intel nvme sha512_ssse3 ahci sha1_ssse3 libahci nvme_core 
wmi aesni_intel
[   34.654430] CPU: 92 UID: 0 PID: 4581 Comm: (udev-worker) Tainted: 
G        W           6.15.0-rc7-next-20250522+ #4 PREEMPT(voluntary)  
849304994a065362c1f65db9527c0b4292d5aea6
[   34.654433] Tainted: [W]=WARN
[   34.654435] RIP: 0010:__sev_do_cmd_locked+0x7eb/0xb90 [ccp]
[   34.654439] Code: fa ff ff be ff ff ff ff 48 c7 c7 50 cd b1 c0 44 89 
85 70 ff ff ff e8 c4 fe f3 f3 44 8b 85 70 ff ff ff 85 c0 0f 85 e2 fd ff 
ff <0f> 0b e9 db fd ff ff 48 8b 05 57 aa 12 00 8b 0d 95 82 0c f5 48 c7
[   34.654440] RSP: 0018:ff51f9b5d9f37890 EFLAGS: 00010246
[   34.654442] RAX: 0000000000000000 RBX: 00000000000000ce RCX: 
0000000000000001
[   34.654443] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 
0000000000000246
[   34.654443] RBP: ff51f9b5d9f37940 R08: 0000000000000000 R09: 
0000000000000000
[   34.654444] R10: 0000000000000001 R11: 0000000000000001 R12: 
ff51f9b5d9f37968
[   34.654445] R13: ff3121dada778000 R14: 0000000000000000 R15: 
ff3121dadb5c5028
[   34.654446] FS:  00007f0ed64488c0(0000) GS:ff3121e9b1a00000(0000) 
knlGS:0000000000000000
[   34.654447] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   34.654448] CR2: 00005599a0790fc8 CR3: 0000000108cd8001 CR4: 
0000000000771ef0
[   34.654449] PKRU: 55555554
[   34.654450] Call Trace:
[   34.654451]  <TASK>
[   34.654457]  sev_get_api_version+0x1e6/0x2b0 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.654463]  ? __pfx_sp_mod_init+0x10/0x10 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.654469]  sev_pci_init+0x4a/0x320 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.654473]  ? preempt_count_sub+0x50/0x80
[   34.654475]  ? _raw_write_unlock_irqrestore+0x53/0x90
[   34.654477]  ? __pfx_sp_mod_init+0x10/0x10 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.654482]  psp_pci_init+0x2f/0x50 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.654487]  sp_mod_init+0x32/0xff0 [ccp 
3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
[   34.654491]  do_one_initcall+0x5f/0x3c0
[   34.654493]  ? __kmalloc_cache_noprof+0x331/0x430
[   34.654498]  do_init_module+0x68/0x260
[   34.654500]  load_module+0x22ea/0x2410
[   34.654509]  ? kernel_read_file+0x2a4/0x320
[   34.654513]  init_module_from_file+0x96/0xd0
[   34.654515]  ? init_module_from_file+0x96/0xd0
[   34.654522]  idempotent_init_module+0x117/0x330
[   34.654530]  __x64_sys_finit_module+0x6f/0xe0
[   34.654532]  x64_sys_call+0x1f9e/0x20c0
[   34.654534]  do_syscall_64+0x8d/0x2d0
[   34.654536]  ? local_clock_noinstr+0x12/0xc0
[   34.654539]  ? rcu_read_unlock+0x1b/0x70
[   34.654541]  ? sched_clock_noinstr+0xd/0x20
[   34.654544]  ? local_clock_noinstr+0x12/0xc0
[   34.654547]  ? exc_page_fault+0x95/0x230
[   34.654551]  ? irqentry_exit_to_user_mode+0xb1/0x1e0
[   34.654553]  ? irqentry_exit+0x6f/0xa0
[   34.654555]  ? exc_page_fault+0xb4/0x230
[   34.654558]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   34.654559] RIP: 0033:0x7f0ed632725d
[   34.654560] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 
48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48
[   34.654561] RSP: 002b:00007ffe599733b8 EFLAGS: 00000246 ORIG_RAX: 
0000000000000139
[   34.654563] RAX: ffffffffffffffda RBX: 00005599a07b4370 RCX: 
00007f0ed632725d
[   34.654564] RDX: 0000000000000000 RSI: 00007f0ed662507d RDI: 
0000000000000022
[   34.654565] RBP: 00007ffe59973470 R08: 0000000000000040 R09: 
00007ffe59973420
[   34.654566] R10: 00007f0ed6403b20 R11: 0000000000000246 R12: 
00007f0ed662507d
[   34.654566] R13: 0000000000020000 R14: 00005599a07b6020 R15: 
00005599a07b9230
[   34.654572]  </TASK>
[   34.654573] irq event stamp: 212111
[   34.654574] hardirqs last  enabled at (212117): [<ffffffffb37a6786>] 
__up_console_sem+0x86/0x90
[   34.654576] hardirqs last disabled at (212122): [<ffffffffb37a676b>] 
__up_console_sem+0x6b/0x90
[   34.654577] softirqs last  enabled at (209856): [<ffffffffb36e364f>] 
handle_softirqs+0x32f/0x410
[   34.654579] softirqs last disabled at (209833): [<ffffffffb36e3800>] 
__irq_exit_rcu+0xc0/0xf0
[   34.654581] ---[ end trace 0000000000000000 ]---

[-- Attachment #2: forashish.config.gz --]
[-- Type: application/gzip, Size: 71145 bytes --]

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

[Dave Hansen]

On 5/19/25 17:02, Ashish Kalra wrote:
> +	kvm-amd.ciphertext_hiding_nr_asids=
> +			[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.

This help text isn't great.

It doesn't come out and say what the connection between CipherTextHiding
and SEV-ES+. It's also impossible to choose a good number without
knowing how large the ASID space is in the first place.

Why use "-1"? Why not just take "auto" as a parameter?

It also needs to say what CipherTextHiding is in the first place and be
more clear about what the tradeoffs are from enabling this.

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

[Kalra, Ashish]

Hello Kim,

On 5/22/2025 9:56 AM, Kim Phillips wrote:
> Hi Ashish,
> 
> On 5/19/25 6:56 PM, 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
>> will see constant default values (0xff).
> If I apply this on top of next-20250522, I get the following stacktrace,
> i.e., this assertion failure:
> 
> static int sev_write_init_ex_file_if_required(int cmd_id)
> {
>         lockdep_assert_held(&sev_cmd_mutex);
> 
> Config attached.
> 
> Thanks,
> 
> Kim

The lockdep assertion is triggered as snp_get_platform_data() issues SNP_PLATFORM_STATUS and SNP_FEATURE_INFO commands without acquiring sev_cmd_mutex and then sev_cmd_mutex being held check (lockdep_assert_held()) getting triggered as part of __sev_do_cmd_locked().

I will fix snp_get_platform_data() to issue SNP_PLATFORM_STATUS and SNP_FEATURE_INFO commands using sev_do_cmd() instead of using __sev_do_cmd_locked() for the next version of this patch-series.

Thanks,
Ashish

> 
> [   34.653536] ------------[ cut here ]------------
> [   34.653545] WARNING: CPU: 92 PID: 4581 at drivers/crypto/ccp/sev-dev.c:349 __sev_do_cmd_locked+0x7eb/0xb90 [ccp]
> [   34.653570] Modules linked in: binfmt_misc rapl wmi_bmof kvm ast drm_client_lib drm_shmem_helper drm_kms_helper ccp(+) i2c_algo_bit i2c_piix4 k10temp i2c_smbus acpi_ipmi ipmi_si(+) ipmi_devintf ipmi_msghandler mac_hid sch_fq_codel dm_multipath drm efi_pstore nfnetlink dmi_sysfs ip_tables x_tables autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq raid1 raid0 linear dm_mirror dm_region_hash dm_log ghash_clmulni_intel nvme sha512_ssse3 ahci sha1_ssse3 libahci nvme_core wmi aesni_intel
> [   34.653645] CPU: 92 UID: 0 PID: 4581 Comm: (udev-worker) Not tainted 6.15.0-rc7-next-20250522+ #4 PREEMPT(voluntary) 849304994a065362c1f65db9527c0b4292d5aea6
> [   34.653651] Hardware name: AMD Corporation VOLCANO/VOLCANO, BIOS RVOT1005B 04/08/2025
> [   34.653653] RIP: 0010:__sev_do_cmd_locked+0x7eb/0xb90 [ccp]
> [   34.653661] Code: fa ff ff be ff ff ff ff 48 c7 c7 50 cd b1 c0 44 89 85 70 ff ff ff e8 c4 fe f3 f3 44 8b 85 70 ff ff ff 85 c0 0f 85 e2 fd ff ff <0f> 0b e9 db fd ff ff 48 8b 05 57 aa 12 00 8b 0d 95 82 0c f5 48 c7
> [   34.653664] RSP: 0018:ff51f9b5d9f37890 EFLAGS: 00010246
> [   34.653668] RAX: 0000000000000000 RBX: 0000000000000083 RCX: 0000000000000001
> [   34.653671] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000246
> [   34.653672] RBP: ff51f9b5d9f37940 R08: 0000000000000000 R09: 0000000000000000
> [   34.653674] R10: 0000000000000001 R11: 0000000000000001 R12: ff51f9b5d9f37954
> [   34.653676] R13: ff3121dada778000 R14: 0000000000000000 R15: ff3121dadb5c5028
> [   34.653677] FS:  00007f0ed64488c0(0000) GS:ff3121e9b1a00000(0000) knlGS:0000000000000000
> [   34.653679] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   34.653681] CR2: 00005599a0790fc8 CR3: 0000000108cd8001 CR4: 0000000000771ef0
> [   34.653684] PKRU: 55555554
> [   34.653686] Call Trace:
> [   34.653687]  <TASK>
> [   34.653701]  sev_get_api_version+0xb2/0x2b0 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.653714]  ? __pfx_sp_mod_init+0x10/0x10 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.653727]  sev_pci_init+0x4a/0x320 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.653733]  ? preempt_count_sub+0x50/0x80
> [   34.653741]  ? _raw_write_unlock_irqrestore+0x53/0x90
> [   34.653748]  ? __pfx_sp_mod_init+0x10/0x10 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.653756]  psp_pci_init+0x2f/0x50 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.653763]  sp_mod_init+0x32/0xff0 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.653770]  do_one_initcall+0x5f/0x3c0
> [   34.653774]  ? __kmalloc_cache_noprof+0x331/0x430
> [   34.653784]  do_init_module+0x68/0x260
> [   34.653789]  load_module+0x22ea/0x2410
> [   34.653803]  ? kernel_read_file+0x2a4/0x320
> [   34.653811]  init_module_from_file+0x96/0xd0
> [   34.653815]  ? init_module_from_file+0x96/0xd0
> [   34.653825]  idempotent_init_module+0x117/0x330
> [   34.653836]  __x64_sys_finit_module+0x6f/0xe0
> [   34.653841]  x64_sys_call+0x1f9e/0x20c0
> [   34.653844]  do_syscall_64+0x8d/0x2d0
> [   34.653849]  ? local_clock_noinstr+0x12/0xc0
> [   34.653855]  ? rcu_read_unlock+0x1b/0x70
> [   34.653860]  ? sched_clock_noinstr+0xd/0x20
> [   34.653864]  ? local_clock_noinstr+0x12/0xc0
> [   34.653869]  ? exc_page_fault+0x95/0x230
> [   34.653876]  ? irqentry_exit_to_user_mode+0xb1/0x1e0
> [   34.653880]  ? irqentry_exit+0x6f/0xa0
> [   34.653882]  ? exc_page_fault+0xb4/0x230
> [   34.653886]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [   34.653888] RIP: 0033:0x7f0ed632725d
> [   34.653892] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48
> [   34.653894] RSP: 002b:00007ffe599733b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [   34.653897] RAX: ffffffffffffffda RBX: 00005599a07b4370 RCX: 00007f0ed632725d
> [   34.653899] RDX: 0000000000000000 RSI: 00007f0ed662507d RDI: 0000000000000022
> [   34.653901] RBP: 00007ffe59973470 R08: 0000000000000040 R09: 00007ffe59973420
> [   34.653902] R10: 00007f0ed6403b20 R11: 0000000000000246 R12: 00007f0ed662507d
> [   34.653903] R13: 0000000000020000 R14: 00005599a07b6020 R15: 00005599a07b9230
> [   34.653913]  </TASK>
> [   34.653914] irq event stamp: 211387
> [   34.653916] hardirqs last  enabled at (211393): [<ffffffffb37a6786>] __up_console_sem+0x86/0x90
> [   34.653922] hardirqs last disabled at (211398): [<ffffffffb37a676b>] __up_console_sem+0x6b/0x90
> [   34.653923] softirqs last  enabled at (209856): [<ffffffffb36e364f>] handle_softirqs+0x32f/0x410
> [   34.653928] softirqs last disabled at (209833): [<ffffffffb36e3800>] __irq_exit_rcu+0xc0/0xf0
> [   34.653932] ---[ end trace 0000000000000000 ]---
> [   34.654388] ------------[ cut here ]------------
> [   34.654391] WARNING: CPU: 92 PID: 4581 at drivers/crypto/ccp/sev-dev.c:349 __sev_do_cmd_locked+0x7eb/0xb90 [ccp]
> [   34.654396] Modules linked in: binfmt_misc rapl wmi_bmof kvm ast drm_client_lib drm_shmem_helper drm_kms_helper ccp(+) i2c_algo_bit i2c_piix4 k10temp i2c_smbus acpi_ipmi ipmi_si(+) ipmi_devintf ipmi_msghandler mac_hid sch_fq_codel dm_multipath drm efi_pstore nfnetlink dmi_sysfs ip_tables x_tables autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq raid1 raid0 linear dm_mirror dm_region_hash dm_log ghash_clmulni_intel nvme sha512_ssse3 ahci sha1_ssse3 libahci nvme_core wmi aesni_intel
> [   34.654430] CPU: 92 UID: 0 PID: 4581 Comm: (udev-worker) Tainted: G        W           6.15.0-rc7-next-20250522+ #4 PREEMPT(voluntary)  849304994a065362c1f65db9527c0b4292d5aea6
> [   34.654433] Tainted: [W]=WARN
> [   34.654435] RIP: 0010:__sev_do_cmd_locked+0x7eb/0xb90 [ccp]
> [   34.654439] Code: fa ff ff be ff ff ff ff 48 c7 c7 50 cd b1 c0 44 89 85 70 ff ff ff e8 c4 fe f3 f3 44 8b 85 70 ff ff ff 85 c0 0f 85 e2 fd ff ff <0f> 0b e9 db fd ff ff 48 8b 05 57 aa 12 00 8b 0d 95 82 0c f5 48 c7
> [   34.654440] RSP: 0018:ff51f9b5d9f37890 EFLAGS: 00010246
> [   34.654442] RAX: 0000000000000000 RBX: 00000000000000ce RCX: 0000000000000001
> [   34.654443] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000246
> [   34.654443] RBP: ff51f9b5d9f37940 R08: 0000000000000000 R09: 0000000000000000
> [   34.654444] R10: 0000000000000001 R11: 0000000000000001 R12: ff51f9b5d9f37968
> [   34.654445] R13: ff3121dada778000 R14: 0000000000000000 R15: ff3121dadb5c5028
> [   34.654446] FS:  00007f0ed64488c0(0000) GS:ff3121e9b1a00000(0000) knlGS:0000000000000000
> [   34.654447] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   34.654448] CR2: 00005599a0790fc8 CR3: 0000000108cd8001 CR4: 0000000000771ef0
> [   34.654449] PKRU: 55555554
> [   34.654450] Call Trace:
> [   34.654451]  <TASK>
> [   34.654457]  sev_get_api_version+0x1e6/0x2b0 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.654463]  ? __pfx_sp_mod_init+0x10/0x10 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.654469]  sev_pci_init+0x4a/0x320 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.654473]  ? preempt_count_sub+0x50/0x80
> [   34.654475]  ? _raw_write_unlock_irqrestore+0x53/0x90
> [   34.654477]  ? __pfx_sp_mod_init+0x10/0x10 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.654482]  psp_pci_init+0x2f/0x50 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.654487]  sp_mod_init+0x32/0xff0 [ccp 3cf3cbacf97e77e53be58eab8d4f5347a13f205d]
> [   34.654491]  do_one_initcall+0x5f/0x3c0
> [   34.654493]  ? __kmalloc_cache_noprof+0x331/0x430
> [   34.654498]  do_init_module+0x68/0x260
> [   34.654500]  load_module+0x22ea/0x2410
> [   34.654509]  ? kernel_read_file+0x2a4/0x320
> [   34.654513]  init_module_from_file+0x96/0xd0
> [   34.654515]  ? init_module_from_file+0x96/0xd0
> [   34.654522]  idempotent_init_module+0x117/0x330
> [   34.654530]  __x64_sys_finit_module+0x6f/0xe0
> [   34.654532]  x64_sys_call+0x1f9e/0x20c0
> [   34.654534]  do_syscall_64+0x8d/0x2d0
> [   34.654536]  ? local_clock_noinstr+0x12/0xc0
> [   34.654539]  ? rcu_read_unlock+0x1b/0x70
> [   34.654541]  ? sched_clock_noinstr+0xd/0x20
> [   34.654544]  ? local_clock_noinstr+0x12/0xc0
> [   34.654547]  ? exc_page_fault+0x95/0x230
> [   34.654551]  ? irqentry_exit_to_user_mode+0xb1/0x1e0
> [   34.654553]  ? irqentry_exit+0x6f/0xa0
> [   34.654555]  ? exc_page_fault+0xb4/0x230
> [   34.654558]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [   34.654559] RIP: 0033:0x7f0ed632725d
> [   34.654560] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48
> [   34.654561] RSP: 002b:00007ffe599733b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [   34.654563] RAX: ffffffffffffffda RBX: 00005599a07b4370 RCX: 00007f0ed632725d
> [   34.654564] RDX: 0000000000000000 RSI: 00007f0ed662507d RDI: 0000000000000022
> [   34.654565] RBP: 00007ffe59973470 R08: 0000000000000040 R09: 00007ffe59973420
> [   34.654566] R10: 00007f0ed6403b20 R11: 0000000000000246 R12: 00007f0ed662507d
> [   34.654566] R13: 0000000000020000 R14: 00005599a07b6020 R15: 00005599a07b9230
> [   34.654572]  </TASK>
> [   34.654573] irq event stamp: 212111
> [   34.654574] hardirqs last  enabled at (212117): [<ffffffffb37a6786>] __up_console_sem+0x86/0x90
> [   34.654576] hardirqs last disabled at (212122): [<ffffffffb37a676b>] __up_console_sem+0x6b/0x90
> [   34.654577] softirqs last  enabled at (209856): [<ffffffffb36e364f>] handle_softirqs+0x32f/0x410
> [   34.654579] softirqs last disabled at (209833): [<ffffffffb36e3800>] __irq_exit_rcu+0xc0/0xf0
> [   34.654581] ---[ end trace 0000000000000000 ]---

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

[Kalra, Ashish]

On 5/23/2025 12:29 PM, Dave Hansen wrote:
> On 5/19/25 17:02, Ashish Kalra wrote:
>> +	kvm-amd.ciphertext_hiding_nr_asids=
>> +			[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.
> 
> This help text isn't great.
> 
> It doesn't come out and say what the connection between CipherTextHiding
> and SEV-ES+. It's also impossible to choose a good number without
> knowing how large the ASID space is in the first place.
> 
> Why use "-1"? Why not just take "auto" as a parameter?
> 
> It also needs to say what CipherTextHiding is in the first place and be
> more clear about what the tradeoffs are from enabling this.
> 
> 

Sure, i will work on improving the documentation.

Sean, looking forward to your review and feedback on these patch-series, especially the KVM side of patches.

Thanks,
Ashish

Re: [PATCH v4 1/5] crypto: ccp: New bit-field definitions for SNP_PLATFORM_STATUS command

[Tom Lendacky]

On 5/19/25 18:56, Ashish Kalra wrote:
> 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>

Reviewed-by: Tom Lendacky <thomas.lendacky@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 */

Re: [PATCH v4 2/5] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Tom Lendacky]

On 5/19/25 18:56, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> The FEATURE_INFO command provides host and guests a programmatic means

s/provides host and guests/provides hypervisors/

> 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.
> 
> The hypervisor may 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.

This paragraph has nothing to do with this patch, please remove it.

> 
> Switch to using SNP platform status instead of SEV platform status if
> SNP is enabled and cache SNP platform status and feature information
> from CPUID 0x8000_0024, sub-function 0, in the sev_device structure.

Since the SEV platform status and SNP platform status differ, I think this
patch should be split into two separate patches.

The first first patch would cache the current SEV platform status return
structure and eliminate the separate state field (as state is unique
between SEV and SNP). The api_major/api_minor/build can probably remain,
since the same value *should* be reported for both SNP and SEV platform
status command.

The second patch would cache the SNP platform status and feature info
data, with this status data being used for the api_major/api_minor/build.

> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 81 ++++++++++++++++++++++++++++++++++++
>  drivers/crypto/ccp/sev-dev.h |  3 ++
>  include/linux/psp-sev.h      | 29 +++++++++++++
>  3 files changed, 113 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 3451bada884e..b642f1183b8b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -233,6 +233,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 sev_data_snp_feature_info);
>  	default:				return 0;
>  	}
>  
> @@ -1073,6 +1074,69 @@ static void snp_set_hsave_pa(void *arg)
>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>  }
>  
> +static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
> +{
> +	struct sev_data_snp_feature_info snp_feat_info;
> +	struct sev_device *sev = psp_master->sev_data;
> +	struct snp_feature_info *feat_info;
> +	struct sev_data_snp_addr buf;
> +	struct page *page;
> +	int rc;
> +
> +	/*
> +	 * The output buffer must be firmware page if SEV-SNP is
> +	 * initialized.
> +	 */

This comment should be expanded and say that this function is intended to
be called when SNP is not initialized or you make this work for both
situations.

> +	if (sev->snp_initialized)
> +		return -EINVAL;
> +
> +	buf.address = __psp_pa(&sev->snp_plat_status);
> +	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
> +

Remove blank line.

> +	if (rc) {
> +		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
> +			rc, *error);
> +		return rc;
> +	}
> +
> +	status->api_major = sev->snp_plat_status.api_major;
> +	status->api_minor = sev->snp_plat_status.api_minor;
> +	status->build = sev->snp_plat_status.build_id;
> +	status->state = sev->snp_plat_status.state;

This may need to be moved based on how the patches lay out.

> +
> +	/*
> +	 * Do feature discovery of the currently loaded firmware,
> +	 * and cache feature information from CPUID 0x8000_0024,
> +	 * sub-function 0.
> +	 */
> +	if (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.
> +		 */
> +		page = alloc_page(GFP_KERNEL);
> +		if (!page)
> +			return -ENOMEM;

Add a blank line.

> +		feat_info = page_address(page);
> +		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);
> +

Remove blank line.

> +		if (!rc)
> +			sev->feat_info = *feat_info;
> +		else
> +			dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
> +				rc, *error);
> +
> +		__free_page(page);
> +	}
> +
> +	return rc;
> +}
> +
>  static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>  {
>  	struct sev_data_range_list *range_list = arg;
> @@ -1597,6 +1661,23 @@ static int sev_get_api_version(void)
>  	struct sev_user_data_status status;
>  	int error = 0, ret;
>  
> +	/*
> +	 * Use SNP platform status if SNP is enabled and cache
> +	 * SNP platform status and SNP feature information.
> +	 */
> +	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
> +		ret = snp_get_platform_data(&status, &error);
> +		if (ret) {
> +			dev_err(sev->dev,
> +				"SEV-SNP: failed to get status. Error: %#x\n", error);
> +			return 1;
> +		}
> +	}
> +
> +	/*
> +	 * Fallback to SEV platform status if SNP is not enabled
> +	 * or SNP platform status fails.
> +	 */

I think this comment is incorrect, aren't you calling this on success of
snp_get_platform_data() and returning on error?

You want both platform status outputs cached. So the above behavior is
correct, I believe, that we error out on SNP platform status failure.

Thanks,
Tom

>  	ret = sev_platform_status(&status, &error);
>  	if (ret) {
>  		dev_err(sev->dev,
> 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 v4 3/5] crypto: ccp: Add support to enable CipherTextHiding on SNP_INIT_EX

[Tom Lendacky]

On 5/19/25 18:57, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Ciphertext hiding needs to be enabled on SNP_INIT_EX.
> 
> Add new argument 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 by SEV firmware and additionally if
> CipherTextHiding feature is enabled in the Platform BIOS.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 30 +++++++++++++++++++++++++++---
>  include/linux/psp-sev.h      | 15 +++++++++++++--
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index b642f1183b8b..185668477182 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1074,6 +1074,24 @@ static void snp_set_hsave_pa(void *arg)
>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>  }
>  
> +bool sev_is_snp_ciphertext_hiding_supported(void)
> +{
> +	struct psp_device *psp = psp_master;
> +	struct sev_device *sev;
> +
> +	sev = psp->sev_data;

This needs a check for !psp and !psp->sev_data before de-referencing them.

> +
> +	/*
> +	 * Feature information indicates if CipherTextHiding feature is
> +	 * supported by the SEV firmware and additionally platform status
> +	 * indicates if CipherTextHiding feature is enabled in the
> +	 * Platform BIOS.
> +	 */
> +	return ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
> +	    sev->snp_plat_status.ciphertext_hiding_cap);

Alignment.

> +}
> +EXPORT_SYMBOL_GPL(sev_is_snp_ciphertext_hiding_supported);
> +
>  static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
>  {
>  	struct sev_data_snp_feature_info snp_feat_info;
> @@ -1167,7 +1185,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, unsigned int snp_max_snp_asid)

s/snp_max_snp_asid/max_snp_asid/

>  {
>  	struct psp_device *psp = psp_master;
>  	struct sev_data_snp_init_ex data;
> @@ -1228,6 +1246,12 @@ static int __sev_snp_init_locked(int *error)
>  		}
>  
>  		memset(&data, 0, sizeof(data));
> +
> +		if (snp_max_snp_asid) {
> +			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);
> @@ -1412,7 +1436,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->snp_max_snp_asid);
>  	if (rc && rc != -ENODEV)
>  		return rc;
>  
> @@ -1495,7 +1519,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, 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..66fecd0c0f88 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,13 @@ 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
> + *  @snp_max_snp_asid: maximum ASID usable for SEV-SNP guest if

Only a single space between the "*" and the "@"

s/snp_max_snp_asid/max_snp_asid/

> + *  CipherTextHiding feature is to be enabled
>   */
>  struct sev_platform_init_args {
>  	int error;
>  	bool probe;
> +	unsigned int snp_max_snp_asid;

s/snp_max_snp_asid/max_snp_asid/

Thanks,
Tom

>  };
>  
>  /**
> @@ -841,6 +847,8 @@ struct snp_feature_info {
>  	u32 edx;
>  } __packed;
>  
> +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)
> +
>  #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>  
>  /**
> @@ -984,6 +992,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 sev_is_snp_ciphertext_hiding_supported(void);
>  
>  #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
>  
> @@ -1020,6 +1029,8 @@ static inline void snp_free_firmware_page(void *addr) { }
>  
>  static inline void sev_platform_shutdown(void) { }
>  
> +static inline bool sev_is_snp_ciphertext_hiding_supported(void) { return false; }
> +
>  #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
>  
>  #endif	/* __PSP_SEV_H__ */

Re: [PATCH v4 4/5] KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables

[Tom Lendacky]

On 5/19/25 18:57, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Introduce new min, max sev_es_asid and sev_snp_asid variables.
> 
> The new {min,max}_{sev_es,snp}_asid variables along with existing
> {min,max}_sev_asid variable simplifies partitioning of the
> SEV and SEV-ES+ ASID space.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  arch/x86/kvm/svm/sev.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index dea9480b9ff6..383db1da8699 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -85,6 +85,10 @@ static DECLARE_RWSEM(sev_deactivate_lock);
>  static DEFINE_MUTEX(sev_bitmap_lock);
>  unsigned int max_sev_asid;
>  static unsigned int min_sev_asid;
> +static unsigned int max_sev_es_asid;
> +static unsigned int min_sev_es_asid;
> +static unsigned int max_snp_asid;
> +static unsigned int min_snp_asid;
>  static unsigned long sev_me_mask;
>  static unsigned int nr_asids;
>  static unsigned long *sev_asid_bitmap;
> @@ -172,20 +176,32 @@ 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.
>  	 * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
> -	 * Note: min ASID can end up larger than the max if basic SEV support is
> -	 * effectively disabled by disallowing use of ASIDs for SEV guests.
>  	 */
> -	unsigned int min_asid = sev->es_active ? 1 : min_sev_asid;
> -	unsigned int max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid;
> -	unsigned int asid;
> +	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.
> +	 */
> +

Remove blank line.

>  	if (min_asid > max_asid)
>  		return -ENOTTY;
>  
> @@ -439,7 +455,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;
>  
> @@ -3029,6 +3045,9 @@ void __init sev_hardware_setup(void)
>  		goto out;
>  	}
>  
> +	min_sev_es_asid = min_snp_asid = 1;
> +	max_sev_es_asid = max_snp_asid = min_sev_asid - 1;

Should these be moved to after the min_sev_asid == 1 check ...

> +
>  	/* Has the system been allocated ASIDs for SEV-ES? */
>  	if (min_sev_asid == 1)
>  		goto out;
> @@ -3048,11 +3067,11 @@ void __init sev_hardware_setup(void)
>  	if (boot_cpu_has(X86_FEATURE_SEV_ES))
>  		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_es_asid, max_sev_es_asid);

... so that this becomes 0 and 0 if min_sev_asid == 1 ? (like before)

>  	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_snp_asid, max_snp_asid);

Ditto

Thanks,
Tom

>  >  	sev_enabled = sev_supported;
>  	sev_es_enabled = sev_es_supported;

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

[Tom Lendacky]

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);
>  }
>  

Re: [PATCH v4 2/5] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Kalra, Ashish]

Hello Tom,

On 6/3/2025 10:21 AM, Tom Lendacky wrote:
> On 5/19/25 18:56, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> The FEATURE_INFO command provides host and guests a programmatic means
> 
> s/provides host and guests/provides hypervisors/
> 
>> 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.
>>
>> The hypervisor may 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.
> 
> This paragraph has nothing to do with this patch, please remove it.

Ok.

> 
>>
>> Switch to using SNP platform status instead of SEV platform status if
>> SNP is enabled and cache SNP platform status and feature information
>> from CPUID 0x8000_0024, sub-function 0, in the sev_device structure.
> 
> Since the SEV platform status and SNP platform status differ, I think this
> patch should be split into two separate patches.
> 
> The first first patch would cache the current SEV platform status return
> structure and eliminate the separate state field (as state is unique
> between SEV and SNP).

Eliminate the state field ? 

But isn't the sev_device->state field used also as driver's internal state information
and not directly mapped to platform status, except the initial state is the platform state 
on module load, so why to remove the field altogether ?

> The api_major/api_minor/build can probably remain,
> since the same value *should* be reported for both SNP and SEV platform
> status command.
> 

Ok. 

> The second patch would cache the SNP platform status and feature info
> data, with this status data being used for the api_major/api_minor/build.
> 

Ok. 

>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>  drivers/crypto/ccp/sev-dev.c | 81 ++++++++++++++++++++++++++++++++++++
>>  drivers/crypto/ccp/sev-dev.h |  3 ++
>>  include/linux/psp-sev.h      | 29 +++++++++++++
>>  3 files changed, 113 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 3451bada884e..b642f1183b8b 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -233,6 +233,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 sev_data_snp_feature_info);
>>  	default:				return 0;
>>  	}
>>  
>> @@ -1073,6 +1074,69 @@ static void snp_set_hsave_pa(void *arg)
>>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>>  }
>>  
>> +static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
>> +{
>> +	struct sev_data_snp_feature_info snp_feat_info;
>> +	struct sev_device *sev = psp_master->sev_data;
>> +	struct snp_feature_info *feat_info;
>> +	struct sev_data_snp_addr buf;
>> +	struct page *page;
>> +	int rc;
>> +
>> +	/*
>> +	 * The output buffer must be firmware page if SEV-SNP is
>> +	 * initialized.
>> +	 */
> 
> This comment should be expanded and say that this function is intended to
> be called when SNP is not initialized or you make this work for both
> situations.
>

Ok, i believe i will simply make it work for the case when SNP is not initialized, as that
is the way it is being called. 
 
>> +	if (sev->snp_initialized)
>> +		return -EINVAL;
>> +
>> +	buf.address = __psp_pa(&sev->snp_plat_status);
>> +	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
>> +
> 
> Remove blank line.
> 
>> +	if (rc) {
>> +		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
>> +			rc, *error);
>> +		return rc;
>> +	}
>> +
>> +	status->api_major = sev->snp_plat_status.api_major;
>> +	status->api_minor = sev->snp_plat_status.api_minor;
>> +	status->build = sev->snp_plat_status.build_id;
>> +	status->state = sev->snp_plat_status.state;
> 
> This may need to be moved based on how the patches lay out.
> 
>> +
>> +	/*
>> +	 * Do feature discovery of the currently loaded firmware,
>> +	 * and cache feature information from CPUID 0x8000_0024,
>> +	 * sub-function 0.
>> +	 */
>> +	if (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.
>> +		 */
>> +		page = alloc_page(GFP_KERNEL);
>> +		if (!page)
>> +			return -ENOMEM;
> 
> Add a blank line.
> 
>> +		feat_info = page_address(page);
>> +		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);
>> +
> 
> Remove blank line.
> 
>> +		if (!rc)
>> +			sev->feat_info = *feat_info;
>> +		else
>> +			dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
>> +				rc, *error);
>> +
>> +		__free_page(page);
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>>  static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>>  {
>>  	struct sev_data_range_list *range_list = arg;
>> @@ -1597,6 +1661,23 @@ static int sev_get_api_version(void)
>>  	struct sev_user_data_status status;
>>  	int error = 0, ret;
>>  
>> +	/*
>> +	 * Use SNP platform status if SNP is enabled and cache
>> +	 * SNP platform status and SNP feature information.
>> +	 */
>> +	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
>> +		ret = snp_get_platform_data(&status, &error);
>> +		if (ret) {
>> +			dev_err(sev->dev,
>> +				"SEV-SNP: failed to get status. Error: %#x\n", error);
>> +			return 1;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Fallback to SEV platform status if SNP is not enabled
>> +	 * or SNP platform status fails.
>> +	 */
> 
> I think this comment is incorrect, aren't you calling this on success of
> snp_get_platform_data() and returning on error?

Yes. 

> 
> You want both platform status outputs cached. So the above behavior is
> correct, I believe, that we error out on SNP platform status failure.
> 

Why do we want to cache *both* SEV and SNP platform status, as of now only api_major & minor, build and state
fields are used from SEV platform status --- is this just for future use cases ?

Thanks,
Ashish

> Thanks,
> Tom
> 
>>  	ret = sev_platform_status(&status, &error);
>>  	if (ret) {
>>  		dev_err(sev->dev,
>> 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 v4 3/5] crypto: ccp: Add support to enable CipherTextHiding on SNP_INIT_EX

[Kalra, Ashish]

On 6/3/2025 10:41 AM, Tom Lendacky wrote:
> On 5/19/25 18:57, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> Ciphertext hiding needs to be enabled on SNP_INIT_EX.
>>
>> Add new argument 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 by SEV firmware and additionally if
>> CipherTextHiding feature is enabled in the Platform BIOS.
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>  drivers/crypto/ccp/sev-dev.c | 30 +++++++++++++++++++++++++++---
>>  include/linux/psp-sev.h      | 15 +++++++++++++--
>>  2 files changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index b642f1183b8b..185668477182 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1074,6 +1074,24 @@ static void snp_set_hsave_pa(void *arg)
>>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>>  }
>>  
>> +bool sev_is_snp_ciphertext_hiding_supported(void)
>> +{
>> +	struct psp_device *psp = psp_master;
>> +	struct sev_device *sev;
>> +
>> +	sev = psp->sev_data;
> 
> This needs a check for !psp and !psp->sev_data before de-referencing them.
>

Yes.
 
>> +
>> +	/*
>> +	 * Feature information indicates if CipherTextHiding feature is
>> +	 * supported by the SEV firmware and additionally platform status
>> +	 * indicates if CipherTextHiding feature is enabled in the
>> +	 * Platform BIOS.
>> +	 */
>> +	return ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
>> +	    sev->snp_plat_status.ciphertext_hiding_cap);
> 
> Alignment.
> 

Ok.

>> +}
>> +EXPORT_SYMBOL_GPL(sev_is_snp_ciphertext_hiding_supported);
>> +
>>  static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
>>  {
>>  	struct sev_data_snp_feature_info snp_feat_info;
>> @@ -1167,7 +1185,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, unsigned int snp_max_snp_asid)
> 
> s/snp_max_snp_asid/max_snp_asid/
>

Ok.


 
>>  {
>>  	struct psp_device *psp = psp_master;
>>  	struct sev_data_snp_init_ex data;
>> @@ -1228,6 +1246,12 @@ static int __sev_snp_init_locked(int *error)
>>  		}
>>  
>>  		memset(&data, 0, sizeof(data));
>> +
>> +		if (snp_max_snp_asid) {
>> +			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);
>> @@ -1412,7 +1436,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->snp_max_snp_asid);
>>  	if (rc && rc != -ENODEV)
>>  		return rc;
>>  
>> @@ -1495,7 +1519,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, 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..66fecd0c0f88 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,13 @@ 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
>> + *  @snp_max_snp_asid: maximum ASID usable for SEV-SNP guest if
> 
> Only a single space between the "*" and the "@"
> 
> s/snp_max_snp_asid/max_snp_asid/
>

Ok.

Thanks,
Ashish

 
>> + *  CipherTextHiding feature is to be enabled
>>   */
>>  struct sev_platform_init_args {
>>  	int error;
>>  	bool probe;
>> +	unsigned int snp_max_snp_asid;
> 
> s/snp_max_snp_asid/max_snp_asid/
> 
> Thanks,
> Tom
> 
>>  };
>>  
>>  /**
>> @@ -841,6 +847,8 @@ struct snp_feature_info {
>>  	u32 edx;
>>  } __packed;
>>  
>> +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)
>> +
>>  #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>  
>>  /**
>> @@ -984,6 +992,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 sev_is_snp_ciphertext_hiding_supported(void);
>>  
>>  #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
>>  
>> @@ -1020,6 +1029,8 @@ static inline void snp_free_firmware_page(void *addr) { }
>>  
>>  static inline void sev_platform_shutdown(void) { }
>>  
>> +static inline bool sev_is_snp_ciphertext_hiding_supported(void) { return false; }
>> +
>>  #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
>>  
>>  #endif	/* __PSP_SEV_H__ */

Re: [PATCH v4 4/5] KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables

[Kalra, Ashish]

On 6/3/2025 10:52 AM, Tom Lendacky wrote:
> On 5/19/25 18:57, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> Introduce new min, max sev_es_asid and sev_snp_asid variables.
>>
>> The new {min,max}_{sev_es,snp}_asid variables along with existing
>> {min,max}_sev_asid variable simplifies partitioning of the
>> SEV and SEV-ES+ ASID space.
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>  arch/x86/kvm/svm/sev.c | 37 ++++++++++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index dea9480b9ff6..383db1da8699 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -85,6 +85,10 @@ static DECLARE_RWSEM(sev_deactivate_lock);
>>  static DEFINE_MUTEX(sev_bitmap_lock);
>>  unsigned int max_sev_asid;
>>  static unsigned int min_sev_asid;
>> +static unsigned int max_sev_es_asid;
>> +static unsigned int min_sev_es_asid;
>> +static unsigned int max_snp_asid;
>> +static unsigned int min_snp_asid;
>>  static unsigned long sev_me_mask;
>>  static unsigned int nr_asids;
>>  static unsigned long *sev_asid_bitmap;
>> @@ -172,20 +176,32 @@ 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.
>>  	 * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
>> -	 * Note: min ASID can end up larger than the max if basic SEV support is
>> -	 * effectively disabled by disallowing use of ASIDs for SEV guests.
>>  	 */
>> -	unsigned int min_asid = sev->es_active ? 1 : min_sev_asid;
>> -	unsigned int max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid;
>> -	unsigned int asid;
>> +	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.
>> +	 */
>> +
> 
> Remove blank line.
> 
>>  	if (min_asid > max_asid)
>>  		return -ENOTTY;
>>  
>> @@ -439,7 +455,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;
>>  
>> @@ -3029,6 +3045,9 @@ void __init sev_hardware_setup(void)
>>  		goto out;
>>  	}
>>  
>> +	min_sev_es_asid = min_snp_asid = 1;
>> +	max_sev_es_asid = max_snp_asid = min_sev_asid - 1;
> 
> Should these be moved to after the min_sev_asid == 1 check ...
> 
>> +
>>  	/* Has the system been allocated ASIDs for SEV-ES? */
>>  	if (min_sev_asid == 1)
>>  		goto out;
>> @@ -3048,11 +3067,11 @@ void __init sev_hardware_setup(void)
>>  	if (boot_cpu_has(X86_FEATURE_SEV_ES))
>>  		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_es_asid, max_sev_es_asid);
> 
> ... so that this becomes 0 and 0 if min_sev_asid == 1 ? (like before)
> 
>>  	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_snp_asid, max_snp_asid);
> 
> Ditto
>

Yes that makes sense.

Thanks,
Ashish

 
> Thanks,
> Tom
> 
>>  >  	sev_enabled = sev_supported;
>>  	sev_es_enabled = sev_es_supported;

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

[Kalra, Ashish]

Hello Tom,

On 6/3/2025 11:26 AM, Tom Lendacky wrote:
> 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.

Ok.

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

Ok.
 
>> +			[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.
>

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

Yes that sounds better.

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

Well, the user specified a non-zero ciphertext_hiding_nr_asids, so the intent is to enable ciphertext hiding and therefore 
sanitize the user specified parameter and enable ciphertext hiding.
 
> 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.
> 

Ok.

>> +		}
>> +
>> +		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?

Yes.

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

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

Considering the above example again, this will still be incorrect as with above change:

Taking, min_sev_asid == 3, max_snp_asid = 2 and min_sev_asid = 3.

So i believe it should be :

max_snp_asid = ciphertext_hiding_asids != -1 ? : (min_sev_asid - 1) / 2;
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.
> 

Ok.

Thanks,
Ashish

> 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);
>>  }
>>  

Re: [PATCH v4 2/5] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Alexey Kardashevskiy]

On 20/5/25 09:56, 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.
> 
> The hypervisor may 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.
> 
> Switch to using SNP platform status instead of SEV platform status if
> SNP is enabled and cache SNP platform status and feature information
> from CPUID 0x8000_0024, sub-function 0, in the sev_device structure.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>   drivers/crypto/ccp/sev-dev.c | 81 ++++++++++++++++++++++++++++++++++++
>   drivers/crypto/ccp/sev-dev.h |  3 ++
>   include/linux/psp-sev.h      | 29 +++++++++++++
>   3 files changed, 113 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 3451bada884e..b642f1183b8b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -233,6 +233,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 sev_data_snp_feature_info);
>   	default:				return 0;
>   	}
>   
> @@ -1073,6 +1074,69 @@ static void snp_set_hsave_pa(void *arg)
>   	wrmsrq(MSR_VM_HSAVE_PA, 0);
>   }
>   
> +static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
> +{
> +	struct sev_data_snp_feature_info snp_feat_info;
> +	struct sev_device *sev = psp_master->sev_data;
> +	struct snp_feature_info *feat_info;
> +	struct sev_data_snp_addr buf;
> +	struct page *page;
> +	int rc;
> +
> +	/*
> +	 * The output buffer must be firmware page if SEV-SNP is
> +	 * initialized.
> +	 */
> +	if (sev->snp_initialized)
> +		return -EINVAL;
> +
> +	buf.address = __psp_pa(&sev->snp_plat_status);
> +	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
> +
> +	if (rc) {
> +		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
> +			rc, *error);
> +		return rc;
> +	}
> +
> +	status->api_major = sev->snp_plat_status.api_major;
> +	status->api_minor = sev->snp_plat_status.api_minor;
> +	status->build = sev->snp_plat_status.build_id;
> +	status->state = sev->snp_plat_status.state;

These 4 lines should be ....

> +
> +	/*
> +	 * Do feature discovery of the currently loaded firmware,
> +	 * and cache feature information from CPUID 0x8000_0024,
> +	 * sub-function 0.
> +	 */
> +	if (sev->snp_plat_status.feature_info) {

Could do:

if (!sev->snp_plat_status.feature_info)
	return 0;

and reduce indentation below.

> +		/*
> +		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
> +		 * command to handle any alignment and page boundary check
> +		 * requirements.
> +		 */
> +		page = alloc_page(GFP_KERNEL);
> +		if (!page)
> +			return -ENOMEM;
> +		feat_info = page_address(page);
> +		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;
> +		else
> +			dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
> +				rc, *error);
> +
> +		__free_page(page);
> +	}
> +
> +	return rc;
> +}
> +
>   static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>   {
>   	struct sev_data_range_list *range_list = arg;
> @@ -1597,6 +1661,23 @@ static int sev_get_api_version(void)
>   	struct sev_user_data_status status;
>   	int error = 0, ret;
>   
> +	/*
> +	 * Use SNP platform status if SNP is enabled and cache
> +	 * SNP platform status and SNP feature information.
> +	 */
> +	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
> +		ret = snp_get_platform_data(&status, &error);
> +		if (ret) {
> +			dev_err(sev->dev,
> +				"SEV-SNP: failed to get status. Error: %#x\n", error);

Drop this as snp_get_platform_data() does that too (and even better as it prints @ret too).

> +			return 1;
> +		}

.... here, and drop the @status parameter. Which in fact is pointless anyway as ....

Let snp_get_platform_data() do just SNP stuff.


> +	}
> +
> +	/*
> +	 * Fallback to SEV platform status if SNP is not enabled> +	 * or SNP platform status fails.
> +	 */
>   	ret = sev_platform_status(&status, &error);

... this sev_platform_status() is called anyway (not a fallback as the comment above says) and will override whatever snp_get_platform_data() wrote to @status.


>   	if (ret) {
>   		dev_err(sev->dev,
> 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;


"snp_feat_info_0" as 1) it is for SNP like some other fields 2) there is subfunction=1 already defined so need some distinction.


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

(not insisting) You could even define this as "struct snp_feature_info_0" with all the bits from "Table 5. Contents of Each Subfunction of Fn8000_0024". Thanks,


> +
>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>   
>   /**

-- 
Alexey

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

[Alexey Kardashevskiy]

On 20/5/25 10: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
> and SEV-SNP.
> 
> Add new module parameter to the KVM module to enable CipherTextHiding
> 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=
> +			[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.


Why in halves? 0 or max would make sense and I'd think the user wants all SEV-ES+ VMs be hidden by default so I'd name the parameter as no_hiding_nr_asids and make the default value of zero mean "every SEV-ES+ is hidden".

Or there is a downside of hiding all VMs?


> +
>   	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.
> +	 */
> +	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;
> +		}
> +
> +		min_sev_es_asid = ciphertext_hiding_nr_asids == -1 ? (min_sev_asid - 1) / 2 :
> +				  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");


Can do "init_args.snp_max_snp_asid = max_snp_asid;" here (as max_snp_asid seems to not change between here and next hunk) and drop snp_cipher_text_hiding. Thanks,

> +	}
> +
>   	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);
>   }
>   

-- 
Alexey

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

[Alexey Kardashevskiy]

On 20/5/25 09:57, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Ciphertext hiding needs to be enabled on SNP_INIT_EX.
> 
> Add new argument 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 by SEV firmware and additionally if
> CipherTextHiding feature is enabled in the Platform BIOS.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>   drivers/crypto/ccp/sev-dev.c | 30 +++++++++++++++++++++++++++---
>   include/linux/psp-sev.h      | 15 +++++++++++++--
>   2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index b642f1183b8b..185668477182 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1074,6 +1074,24 @@ static void snp_set_hsave_pa(void *arg)
>   	wrmsrq(MSR_VM_HSAVE_PA, 0);
>   }
>   
> +bool sev_is_snp_ciphertext_hiding_supported(void)
> +{
> +	struct psp_device *psp = psp_master;
> +	struct sev_device *sev;
> +
> +	sev = psp->sev_data;
> +
> +	/*
> +	 * Feature information indicates if CipherTextHiding feature is
> +	 * supported by the SEV firmware and additionally platform status
> +	 * indicates if CipherTextHiding feature is enabled in the
> +	 * Platform BIOS.
> +	 */
> +	return ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
> +	    sev->snp_plat_status.ciphertext_hiding_cap);
> +}
> +EXPORT_SYMBOL_GPL(sev_is_snp_ciphertext_hiding_supported);
> +
>   static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
>   {
>   	struct sev_data_snp_feature_info snp_feat_info;
> @@ -1167,7 +1185,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, unsigned int snp_max_snp_asid)
>   {
>   	struct psp_device *psp = psp_master;
>   	struct sev_data_snp_init_ex data;
> @@ -1228,6 +1246,12 @@ static int __sev_snp_init_locked(int *error)
>   		}
>   
>   		memset(&data, 0, sizeof(data));
> +
> +		if (snp_max_snp_asid) {
> +			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);
> @@ -1412,7 +1436,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->snp_max_snp_asid);
>   	if (rc && rc != -ENODEV)
>   		return rc;
>   
> @@ -1495,7 +1519,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, 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..66fecd0c0f88 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,13 @@ 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
> + *  @snp_max_snp_asid: maximum ASID usable for SEV-SNP guest if
> + *  CipherTextHiding feature is to be enabled
>    */
>   struct sev_platform_init_args {
>   	int error;
>   	bool probe;
> +	unsigned int snp_max_snp_asid;
>   };
>   
>   /**
> @@ -841,6 +847,8 @@ struct snp_feature_info {
>   	u32 edx;
>   } __packed;
>   
> +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)

SNP_FEATURE_CIPHER_TEXT_HIDING ?
or X86_FEATURE_CIPHER_TEXT_HIDING ?

In other words, mimic X86_FEATURE_SEV which is a real CPUID bit and FEATURE_INFO mimics CPUID. Thanks,


> +
>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>   
>   /**
> @@ -984,6 +992,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 sev_is_snp_ciphertext_hiding_supported(void);
>   
>   #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
>   
> @@ -1020,6 +1029,8 @@ static inline void snp_free_firmware_page(void *addr) { }
>   
>   static inline void sev_platform_shutdown(void) { }
>   
> +static inline bool sev_is_snp_ciphertext_hiding_supported(void) { return false; }
> +
>   #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
>   
>   #endif	/* __PSP_SEV_H__ */

-- 
Alexey

Re: [PATCH v4 2/5] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Tom Lendacky]

On 6/4/25 17:52, Kalra, Ashish wrote:
> Hello Tom,
> 
> On 6/3/2025 10:21 AM, Tom Lendacky wrote:
>> On 5/19/25 18:56, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>
>>>
>>> Switch to using SNP platform status instead of SEV platform status if
>>> SNP is enabled and cache SNP platform status and feature information
>>> from CPUID 0x8000_0024, sub-function 0, in the sev_device structure.
>>
>> Since the SEV platform status and SNP platform status differ, I think this
>> patch should be split into two separate patches.
>>
>> The first first patch would cache the current SEV platform status return
>> structure and eliminate the separate state field (as state is unique
>> between SEV and SNP).
> 
> Eliminate the state field ? 
> 
> But isn't the sev_device->state field used also as driver's internal state information
> and not directly mapped to platform status, except the initial state is the platform state 
> on module load, so why to remove the field altogether ?

Well the state of SEV and SNP are different, so they need to be tracked
separately (or at least better clarified in the code that the current
state field is tracking the SEV state). Since the state field is part of
the platform status, you can just update that field. Not sure what it will
look like.

The current state is only SEV related because SNP only has UNINIT and INIT
and so the snp_initialized flag is used instead.

Thanks,
Tom

> 

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

[Tom Lendacky]

On 6/4/25 19:17, Kalra, Ashish wrote:
> Hello Tom,
> 
> On 6/3/2025 11:26 AM, Tom Lendacky wrote:
>> On 5/19/25 19:02, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>

>>
>> 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.
>>
> 
> Yes that sounds better.

Just fix my spelling errors :)

> 
>>> +	 */
>>> +	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?
>>
> 
> Well, the user specified a non-zero ciphertext_hiding_nr_asids, so the intent is to enable ciphertext hiding and therefore 
> sanitize the user specified parameter and enable ciphertext hiding.

Should you support something like ciphertext_hiding_asids=max (similar to
the 'auto' comment that Dave had) and report an error if a value is
specified that exceeds min_sev_asid and not enable ciphertext hiding?

Thanks,
Tom

>  

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

[Kalra, Ashish]

On 6/5/2025 1:32 AM, Alexey Kardashevskiy wrote:
> On 20/5/25 09:57, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> Ciphertext hiding needs to be enabled on SNP_INIT_EX.
>>
>> Add new argument 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 by SEV firmware and additionally if
>> CipherTextHiding feature is enabled in the Platform BIOS.
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>   drivers/crypto/ccp/sev-dev.c | 30 +++++++++++++++++++++++++++---
>>   include/linux/psp-sev.h      | 15 +++++++++++++--
>>   2 files changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index b642f1183b8b..185668477182 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1074,6 +1074,24 @@ static void snp_set_hsave_pa(void *arg)
>>       wrmsrq(MSR_VM_HSAVE_PA, 0);
>>   }
>>   +bool sev_is_snp_ciphertext_hiding_supported(void)
>> +{
>> +    struct psp_device *psp = psp_master;
>> +    struct sev_device *sev;
>> +
>> +    sev = psp->sev_data;
>> +
>> +    /*
>> +     * Feature information indicates if CipherTextHiding feature is
>> +     * supported by the SEV firmware and additionally platform status
>> +     * indicates if CipherTextHiding feature is enabled in the
>> +     * Platform BIOS.
>> +     */
>> +    return ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
>> +        sev->snp_plat_status.ciphertext_hiding_cap);
>> +}
>> +EXPORT_SYMBOL_GPL(sev_is_snp_ciphertext_hiding_supported);
>> +
>>   static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
>>   {
>>       struct sev_data_snp_feature_info snp_feat_info;
>> @@ -1167,7 +1185,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, unsigned int snp_max_snp_asid)
>>   {
>>       struct psp_device *psp = psp_master;
>>       struct sev_data_snp_init_ex data;
>> @@ -1228,6 +1246,12 @@ static int __sev_snp_init_locked(int *error)
>>           }
>>             memset(&data, 0, sizeof(data));
>> +
>> +        if (snp_max_snp_asid) {
>> +            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);
>> @@ -1412,7 +1436,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->snp_max_snp_asid);
>>       if (rc && rc != -ENODEV)
>>           return rc;
>>   @@ -1495,7 +1519,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, 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..66fecd0c0f88 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,13 @@ 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
>> + *  @snp_max_snp_asid: maximum ASID usable for SEV-SNP guest if
>> + *  CipherTextHiding feature is to be enabled
>>    */
>>   struct sev_platform_init_args {
>>       int error;
>>       bool probe;
>> +    unsigned int snp_max_snp_asid;
>>   };
>>     /**
>> @@ -841,6 +847,8 @@ struct snp_feature_info {
>>       u32 edx;
>>   } __packed;
>>   +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED    BIT(3)
> 
> SNP_FEATURE_CIPHER_TEXT_HIDING ?
> or X86_FEATURE_CIPHER_TEXT_HIDING ?
> 
> In other words, mimic X86_FEATURE_SEV which is a real CPUID bit and FEATURE_INFO mimics CPUID. Thanks,
>

I will still prefer to name it SNP_FEATURE_CIPHER_TEXT_HIDING, as the SNP FW ABI specs names it as such
and it is a SNP specific feature, prefixing X86 is confusing.

Thanks,
Ashish
 
> 
>> +
>>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>     /**
>> @@ -984,6 +992,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 sev_is_snp_ciphertext_hiding_supported(void);
>>     #else    /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>   @@ -1020,6 +1029,8 @@ static inline void snp_free_firmware_page(void *addr) { }
>>     static inline void sev_platform_shutdown(void) { }
>>   +static inline bool sev_is_snp_ciphertext_hiding_supported(void) { return false; }
>> +
>>   #endif    /* CONFIG_CRYPTO_DEV_SP_PSP */
>>     #endif    /* __PSP_SEV_H__ */
> 

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

[Kalra, Ashish]

On 6/5/2025 1:32 AM, Alexey Kardashevskiy wrote:
> On 20/5/25 10: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
>> and SEV-SNP.
>>
>> Add new module parameter to the KVM module to enable CipherTextHiding
>> 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=
>> +            [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.
> 
> 
> Why in halves? 0 or max would make sense and I'd think the user wants all SEV-ES+ VMs be hidden by default so I'd name the parameter as no_hiding_nr_asids and make the default value of zero mean "every SEV-ES+ is hidden".
>

Actually 'max' sounds nice, but we still want to name it ciphertext_hiding_asids or something and keep the value relative to it, so probably it can be 
a number or 'max'.

Thanks,
Ashish
 
> Or there is a downside of hiding all VMs?
> 
> 
>> +
>>       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.
>> +     */
>> +    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;
>> +        }
>> +
>> +        min_sev_es_asid = ciphertext_hiding_nr_asids == -1 ? (min_sev_asid - 1) / 2 :
>> +                  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");
> 
> 
> Can do "init_args.snp_max_snp_asid = max_snp_asid;" here (as max_snp_asid seems to not change between here and next hunk) and drop snp_cipher_text_hiding. Thanks,
> 
>> +    }
>> +
>>       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);
>>   }
>>   
> 

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

[Kalra, Ashish]

On 6/5/2025 11:23 AM, Tom Lendacky wrote:
> On 6/4/25 19:17, Kalra, Ashish wrote:
>> Hello Tom,
>>
>> On 6/3/2025 11:26 AM, Tom Lendacky wrote:
>>> On 5/19/25 19:02, Ashish Kalra wrote:
>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>>
> 
>>>
>>> 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.
>>>
>>
>> Yes that sounds better.
> 
> Just fix my spelling errors :)
>

Sure.
 
>>
>>>> +	 */
>>>> +	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?
>>>
>>
>> Well, the user specified a non-zero ciphertext_hiding_nr_asids, so the intent is to enable ciphertext hiding and therefore 
>> sanitize the user specified parameter and enable ciphertext hiding.
> 
> Should you support something like ciphertext_hiding_asids=max (similar to
> the 'auto' comment that Dave had) and report an error if a value is
> specified that exceeds min_sev_asid and not enable ciphertext hiding?
>

Ok, that makes sense.

Thanks,
Ashish
 
> Thanks,
> Tom
> 
>>  
> 

Re: [PATCH v4 2/5] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Kalra, Ashish]

On 6/4/2025 11:49 PM, Alexey Kardashevskiy wrote:
> On 20/5/25 09:56, 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.
>>
>> The hypervisor may 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.
>>
>> Switch to using SNP platform status instead of SEV platform status if
>> SNP is enabled and cache SNP platform status and feature information
>> from CPUID 0x8000_0024, sub-function 0, in the sev_device structure.
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>   drivers/crypto/ccp/sev-dev.c | 81 ++++++++++++++++++++++++++++++++++++
>>   drivers/crypto/ccp/sev-dev.h |  3 ++
>>   include/linux/psp-sev.h      | 29 +++++++++++++
>>   3 files changed, 113 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 3451bada884e..b642f1183b8b 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -233,6 +233,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 sev_data_snp_feature_info);
>>       default:                return 0;
>>       }
>>   @@ -1073,6 +1074,69 @@ static void snp_set_hsave_pa(void *arg)
>>       wrmsrq(MSR_VM_HSAVE_PA, 0);
>>   }
>>   +static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
>> +{
>> +    struct sev_data_snp_feature_info snp_feat_info;
>> +    struct sev_device *sev = psp_master->sev_data;
>> +    struct snp_feature_info *feat_info;
>> +    struct sev_data_snp_addr buf;
>> +    struct page *page;
>> +    int rc;
>> +
>> +    /*
>> +     * The output buffer must be firmware page if SEV-SNP is
>> +     * initialized.
>> +     */
>> +    if (sev->snp_initialized)
>> +        return -EINVAL;
>> +
>> +    buf.address = __psp_pa(&sev->snp_plat_status);
>> +    rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
>> +
>> +    if (rc) {
>> +        dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
>> +            rc, *error);
>> +        return rc;
>> +    }
>> +
>> +    status->api_major = sev->snp_plat_status.api_major;
>> +    status->api_minor = sev->snp_plat_status.api_minor;
>> +    status->build = sev->snp_plat_status.build_id;
>> +    status->state = sev->snp_plat_status.state;
> 
> These 4 lines should be ....
> 
>> +
>> +    /*
>> +     * Do feature discovery of the currently loaded firmware,
>> +     * and cache feature information from CPUID 0x8000_0024,
>> +     * sub-function 0.
>> +     */
>> +    if (sev->snp_plat_status.feature_info) {
> 
> Could do:
> 
> if (!sev->snp_plat_status.feature_info)
>     return 0;
> 
> and reduce indentation below.
> 
>> +        /*
>> +         * Use dynamically allocated structure for the SNP_FEATURE_INFO
>> +         * command to handle any alignment and page boundary check
>> +         * requirements.
>> +         */
>> +        page = alloc_page(GFP_KERNEL);
>> +        if (!page)
>> +            return -ENOMEM;
>> +        feat_info = page_address(page);
>> +        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;
>> +        else
>> +            dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
>> +                rc, *error);
>> +
>> +        __free_page(page);
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>>   static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>>   {
>>       struct sev_data_range_list *range_list = arg;
>> @@ -1597,6 +1661,23 @@ static int sev_get_api_version(void)
>>       struct sev_user_data_status status;
>>       int error = 0, ret;
>>   +    /*
>> +     * Use SNP platform status if SNP is enabled and cache
>> +     * SNP platform status and SNP feature information.
>> +     */
>> +    if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
>> +        ret = snp_get_platform_data(&status, &error);
>> +        if (ret) {
>> +            dev_err(sev->dev,
>> +                "SEV-SNP: failed to get status. Error: %#x\n", error);
> 
> Drop this as snp_get_platform_data() does that too (and even better as it prints @ret too).
> 
>> +            return 1;
>> +        }
> 
> .... here, and drop the @status parameter. Which in fact is pointless anyway as ....
> 
> Let snp_get_platform_data() do just SNP stuff.
> 

Yes. 

> 
>> +    }
>> +
>> +    /*
>> +     * Fallback to SEV platform status if SNP is not enabled> +     * or SNP platform status fails.
>> +     */
>>       ret = sev_platform_status(&status, &error);
> 
> ... this sev_platform_status() is called anyway (not a fallback as the comment above says) and will override whatever snp_get_platform_data() wrote to @status.
> 

Yes, as Tom suggested will be moving to caching both SEV and SNP platform status information.  

> 
>>       if (ret) {
>>           dev_err(sev->dev,
>> 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;
> 
> 
> "snp_feat_info_0" as 1) it is for SNP like some other fields 2) there is subfunction=1 already defined so need some distinction.
>

Ok.

Thanks,
Ashish
 
> 
>>   };
>>     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;
> 
> (not insisting) You could even define this as "struct snp_feature_info_0" with all the bits from "Table 5. Contents of Each Subfunction of Fn8000_0024". Thanks,
> 
> 
>> +
>>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>     /**
>