Add SEV-SNP CipherTextHiding feature support

From: Ashish Kalra <[email protected]>

Cache the SEV platform status into sev_device structure and use this cached SEV platform status for api_major/minor/build.

The platform state is unique between SEV and SNP and hence needs to be tracked independently.

Remove the state field from sev_device structure and instead track SEV state from the cached SEV platform status.

Suggested-by Tom Lendacky <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 22 ++++++++++++----------
 drivers/crypto/ccp/sev-dev.h |  3 ++-
 2 files changed, 14 insertions(+), 11 deletions(-)

drivers/crypto/ccp/sev-dev.c +12 -10

index 17edc6bf5622..5a2e1651d171 100644
@@ -1286,7 +1286,7 @@ static int __sev_platform_init_locked(int *error)
 
 	sev = psp_master->sev_data;
 
-	if (sev->state == SEV_STATE_INIT)
+	if (sev->sev_plat_status.state == SEV_STATE_INIT)
 		return 0;
 
 	__sev_platform_init_handle_tmr(sev);

@@ -1318,7 +1318,7 @@ static int __sev_platform_init_locked(int *error)
 		return rc;
 	}
 
-	sev->state = SEV_STATE_INIT;
+	sev->sev_plat_status.state = SEV_STATE_INIT;
 
 	/* Prepare for first SEV guest launch after INIT */
 	wbinvd_on_all_cpus();

@@ -1347,7 +1347,7 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 
 	sev = psp_master->sev_data;
 
-	if (sev->state == SEV_STATE_INIT)
+	if (sev->sev_plat_status.state == SEV_STATE_INIT)
 		return 0;
 
 	rc = __sev_snp_init_locked(&args->error);

@@ -1384,7 +1384,7 @@ static int __sev_platform_shutdown_locked(int *error)
 
 	sev = psp->sev_data;
 
-	if (sev->state == SEV_STATE_UNINIT)
+	if (sev->sev_plat_status.state == SEV_STATE_UNINIT)
 		return 0;
 
 	ret = __sev_do_cmd_locked(SEV_CMD_SHUTDOWN, NULL, error);

@@ -1394,7 +1394,7 @@ static int __sev_platform_shutdown_locked(int *error)
 		return ret;
 	}
 
-	sev->state = SEV_STATE_UNINIT;
+	sev->sev_plat_status.state = SEV_STATE_UNINIT;
 	dev_dbg(sev->dev, "SEV firmware shutdown\n");
 
 	return ret;

@@ -1502,7 +1502,7 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
 	if (!writable)
 		return -EPERM;
 
-	if (sev->state == SEV_STATE_UNINIT) {
+	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
 		rc = sev_move_to_init_state(argp, &shutdown_required);
 		if (rc)
 			return rc;

@@ -1551,7 +1551,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	data.len = input.length;
 
 cmd:
-	if (sev->state == SEV_STATE_UNINIT) {
+	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
 		ret = sev_move_to_init_state(argp, &shutdown_required);
 		if (ret)
 			goto e_free_blob;

@@ -1606,10 +1606,12 @@ static int sev_get_api_version(void)
 		return 1;
 	}
 
+	/* Cache SEV platform status */
+	sev->sev_plat_status = status;
+
 	sev->api_major = status.api_major;
 	sev->api_minor = status.api_minor;
 	sev->build = status.build;
-	sev->state = status.state;
 
 	return 0;
 }

@@ -1837,7 +1839,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	data.oca_cert_len = input.oca_cert_len;
 
 	/* If platform is not in INIT state then transition it to INIT */
-	if (sev->state != SEV_STATE_INIT) {
+	if (sev->sev_plat_status.state != SEV_STATE_INIT) {
 		ret = sev_move_to_init_state(argp, &shutdown_required);
 		if (ret)
 			goto e_free_oca;

@@ -2008,7 +2010,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 
 cmd:
 	/* If platform is not in INIT state then transition it to INIT. */
-	if (sev->state != SEV_STATE_INIT) {
+	if (sev->sev_plat_status.state != SEV_STATE_INIT) {
 		if (!writable) {
 			ret = -EPERM;
 			goto e_free_cert;

drivers/crypto/ccp/sev-dev.h +2 -1

index 3e4e5574e88a..24dd8ff8afaa 100644
@@ -42,7 +42,6 @@ struct sev_device {
 
 	struct sev_vdata *vdata;
 
-	int state;
 	unsigned int int_rcvd;
 	wait_queue_head_t int_queue;
 	struct sev_misc_dev *misc;

@@ -57,6 +56,8 @@ struct sev_device {
 	bool cmd_buf_backup_active;
 
 	bool snp_initialized;
+
+	struct sev_user_data_status sev_plat_status;
 };
 
 int sev_dev_init(struct psp_device *psp);

From: Ashish Kalra <[email protected]>

The FEATURE_INFO command provides hypervisors 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.

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 0x7fff_0024, sub-function 0, in the sev_device structure.

Suggested-by Tom Lendacky <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 72 ++++++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sev-dev.h |  3 ++
 include/linux/psp-sev.h      | 29 +++++++++++++++
 3 files changed, 104 insertions(+)

drivers/crypto/ccp/sev-dev.c +72 -0

index 5a2e1651d171..d1517a91a27d 100644
@@ -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,67 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrq(MSR_VM_HSAVE_PA, 0);
 }
 
+static int snp_get_platform_data(struct sev_device *sev, int *error)
+{
+	struct sev_data_snp_feature_info snp_feat_info;
+	struct snp_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	struct page *page;
+	int rc;
+
+	/*
+	 * This function is expected to be called before SNP is not
+	 * initialized.
+	 */
+	if (sev->snp_initialized)
+		return -EINVAL;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = sev_do_cmd(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;
+	}
+
+	sev->api_major = sev->snp_plat_status.api_major;
+	sev->api_minor = sev->snp_plat_status.api_minor;
+	sev->build = sev->snp_plat_status.build_id;
+
+	/*
+	 * 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)
+		return 0;
+
+	/*
+	 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+	 * command to ensure structure is 8-byte aligned, and does not
+	 * cross a page boundary.
+	 */
+	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(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
+	if (!rc)
+		sev->snp_feat_info_0 = *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;

@@ -1599,6 +1661,16 @@ static int sev_get_api_version(void)
 	struct sev_user_data_status status;
 	int error = 0, ret;
 
+	/*
+	 * Cache SNP platform status and SNP feature information
+	 * if SNP is available.
+	 */
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
+		ret = snp_get_platform_data(sev, &error);
+		if (ret)
+			return 1;
+	}
+
 	ret = sev_platform_status(&status, &error);
 	if (ret) {
 		dev_err(sev->dev,

drivers/crypto/ccp/sev-dev.h +3 -0

index 24dd8ff8afaa..5aed2595c9ae 100644
@@ -58,6 +58,9 @@ struct sev_device {
 	bool snp_initialized;
 
 	struct sev_user_data_status sev_plat_status;
+
+	struct sev_user_data_snp_status snp_plat_status;
+	struct snp_feature_info snp_feat_info_0;
 };
 
 int sev_dev_init(struct psp_device *psp);

include/linux/psp-sev.h +1 -0

index 0f5f94137f6d..935547c26985 100644
@@ -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,
 };

include/linux/psp-sev.h +22 -0

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

From: Ashish Kalra <[email protected]>

Implement a new API interface that indicates both the support for the SEV-SNP Ciphertext Hiding feature by the SEV firmware and whether this feature is enabled in the platform BIOS.

Signed-off-by Ashish Kalra <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 21 +++++++++++++++++++++
 include/linux/psp-sev.h      |  5 +++++
 2 files changed, 26 insertions(+)

drivers/crypto/ccp/sev-dev.c +21 -0

index d1517a91a27d..3f2bbba93617 100644
@@ -1074,6 +1074,27 @@ 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;
+
+	if (!psp || !psp->sev_data)
+		return false;
+
+	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->snp_feat_info_0.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_device *sev, int *error)
 {
 	struct sev_data_snp_feature_info snp_feat_info;

include/linux/psp-sev.h +5 -0

index 935547c26985..ca19fddfcd4d 100644
@@ -843,6 +843,8 @@ struct snp_feature_info {
 	u32 edx;
 } __packed;
 
+#define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**

@@ -986,6 +988,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 */
 

@@ -1022,6 +1025,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__ */

From: Ashish Kalra <[email protected]>

Ciphertext hiding needs to be enabled on SNP_INIT_EX.

Enhance sev_platform_init_args by adding a new argument that enables the KVM module to specify whether the Ciphertext Hiding feature should be activated during SNP initialization. Additionally, this argument will allow for the definition of the maximum ASID that can be used for an SEV-SNP guest when Ciphertext Hiding is enabled.

Signed-off-by Ashish Kalra <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 12 +++++++++---
 include/linux/psp-sev.h      | 10 ++++++++--
 2 files changed, 17 insertions(+), 5 deletions(-)

drivers/crypto/ccp/sev-dev.c +9 -3

index 3f2bbba93617..c883ccf8c3ff 100644
@@ -1186,7 +1186,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 max_snp_asid)
 {
 	struct psp_device *psp = psp_master;
 	struct sev_data_snp_init_ex data;

@@ -1247,6 +1247,12 @@ static int __sev_snp_init_locked(int *error)
 		}
 
 		memset(&data, 0, sizeof(data));
+
+		if (max_snp_asid) {
+			data.ciphertext_hiding_en = 1;
+			data.max_snp_asid = max_snp_asid;
+		}
+
 		data.init_rmp = 1;
 		data.list_paddr_en = 1;
 		data.list_paddr = __psp_pa(snp_range_list);

@@ -1433,7 +1439,7 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 	if (sev->sev_plat_status.state == SEV_STATE_INIT)
 		return 0;
 
-	rc = __sev_snp_init_locked(&args->error);
+	rc = __sev_snp_init_locked(&args->error, args->max_snp_asid);
 	if (rc && rc != -ENODEV)
 		return rc;
 

@@ -1516,7 +1522,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;

include/linux/psp-sev.h +5 -2

index ca19fddfcd4d..b6eda9c560ee 100644
@@ -748,10 +748,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;
 
 /**

include/linux/psp-sev.h +1 -0

  */
 struct sev_platform_init_args {
 	int error;
 	bool probe;
+	unsigned int max_snp_asid;
 };
 
 /**

From: Ashish Kalra <[email protected]>

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 <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 arch/x86/kvm/svm/sev.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

From: Ashish Kalra <[email protected]>

arch/x86/kvm/svm/sev.c +18 -0

@@ -2913,10 +2921,46 @@ static bool is_sev_snp_initialized(void)
  	return initialized;
  }
 
+static bool check_and_enable_sev_snp_ciphertext_hiding(void)
+{
+	unsigned int ciphertext_hiding_asid_nr = 0;
+
+	if (!sev_is_snp_ciphertext_hiding_supported()) {
+		pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
+		return false;
+	}
+
+	if (isdigit(ciphertext_hiding_asids[0])) {
+		if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
+			pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+				ciphertext_hiding_asids);
+			return false;
+		}
+		/* Do sanity checks on user-defined ciphertext_hiding_asids */
+		if (ciphertext_hiding_asid_nr >= min_sev_asid) {
+			pr_warn("Requested ciphertext hiding ASIDs (%u) exceeds or equals minimum SEV ASID (%u)\n",

Hi Ashish,

I can confirm that this v5 series fixes v4's __sev_do_cmd_locked assertion failure problem, thanks.  More comments inline:

Quoted 1 line · Ashish Kalra

From: Ashish Kalra <[email protected]>

Extra From: line not necessary.

Quoted 26 lines

arch/x86/kvm/svm/sev.c +22 -0

@@ -2913,10 +2921,46 @@ static bool is_sev_snp_initialized(void)
       return initialized;
   }
   +static bool check_and_enable_sev_snp_ciphertext_hiding(void)
+{
+    unsigned int ciphertext_hiding_asid_nr = 0;
+
+    if (!sev_is_snp_ciphertext_hiding_supported()) {
+        pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
+        return false;
+    }
+
+    if (isdigit(ciphertext_hiding_asids[0])) {
+        if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
+            pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+                ciphertext_hiding_asids);
+            return false;
+        }
+        /* Do sanity checks on user-defined ciphertext_hiding_asids */
+        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
+            pr_warn("Requested ciphertext hiding ASIDs (%u) exceeds or equals minimum SEV ASID (%u)\n",
+                ciphertext_hiding_asid_nr, min_sev_asid);
+            return false;
+        }
+    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
+        ciphertext_hiding_asid_nr = min_sev_asid - 1;

arch/x86/kvm/svm/sev.c static bool is_sev_snp_initialized(void) · line 2951

Quoted 5 lines

arch/x86/kvm/svm/sev.c +5 -0

+    } else {
+        pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+            ciphertext_hiding_asids);
+        return false;
+    }

This code can be made much simpler if all the invalid cases were combined to emit a single pr_warn().

arch/x86/kvm/svm/sev.c void __init sev_hardware_setup(void) · line 3098

Quoted 11 lines

arch/x86/kvm/svm/sev.c +3 -1

@@ -3036,7 +3090,9 @@ void __init sev_hardware_setup(void)
               min_sev_asid, max_sev_asid);
       if (boot_cpu_has(X86_FEATURE_SEV_ES))
           pr_info("SEV-ES %s (ASIDs %u - %u)\n",
-            str_enabled_disabled(sev_es_supported),
+            sev_es_supported ? min_sev_es_asid < min_sev_asid ? "enabled" :
+                                        "unusable" :
+                                        "disabled",
               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",

If I set ciphertext_hiding_asids=99, I get the new 'unusable':

kvm_amd: SEV-SNP ciphertext hiding enabled
...
kvm_amd: SEV enabled (ASIDs 100 - 1006)
kvm_amd: SEV-ES unusable (ASIDs 100 - 99)
kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)

Ok.

Now, if I set ciphertext_hiding_asids=0, I get:

kvm_amd: SEV-SNP ciphertext hiding enabled
...
kvm_amd: SEV enabled (ASIDs 100 - 1006)
kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
kvm_amd: SEV-SNP enabled (ASIDs 1 - 0)

..where SNP is unusable this time, yet it's not flagged as such.

If there's no difference between "unusable" and not enabled, then I think it's better to keep the not enabled messaging behaviour and just not emit the line at all:  It's confusing to see the invalid "100 - 99" and "1 - 0" ranges.

Thanks,

Kim

Hello Kim,

Quoted 36 lines · Kim Phillips

Hi Ashish,

I can confirm that this v5 series fixes v4's __sev_do_cmd_locked assertion failure problem, thanks.  More comments inline:

Quoted 1 line · Ashish Kalra

From: Ashish Kalra <[email protected]>

Extra From: line not necessary.

Quoted 26 lines

arch/x86/kvm/svm/sev.c +22 -0

@@ -2913,10 +2921,46 @@ static bool is_sev_snp_initialized(void)
       return initialized;
   }
   +static bool check_and_enable_sev_snp_ciphertext_hiding(void)
+{
+    unsigned int ciphertext_hiding_asid_nr = 0;
+
+    if (!sev_is_snp_ciphertext_hiding_supported()) {
+        pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
+        return false;
+    }
+
+    if (isdigit(ciphertext_hiding_asids[0])) {
+        if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
+            pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+                ciphertext_hiding_asids);
+            return false;
+        }
+        /* Do sanity checks on user-defined ciphertext_hiding_asids */
+        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
+            pr_warn("Requested ciphertext hiding ASIDs (%u) exceeds or equals minimum SEV ASID (%u)\n",
+                ciphertext_hiding_asid_nr, min_sev_asid);
+            return false;
+        }
+    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
+        ciphertext_hiding_asid_nr = min_sev_asid - 1;

Quoted 9 lines

arch/x86/kvm/svm/sev.c static bool is_sev_snp_initialized(void) · line 2951

Quoted 5 lines

arch/x86/kvm/svm/sev.c +5 -0

+    } else {
+        pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+            ciphertext_hiding_asids);
+        return false;
+    }

This code can be made much simpler if all the invalid cases were combined to emit a single pr_warn().

There definitely has to be a different pr_warn() for the sanity check case and invalid parameter cases and sanity check has to be done if the specified parameter is an unsigned int, so the check needs to be done separately.

I can definitely add a branch just for the invalid cases.

Quoted 20 lines

arch/x86/kvm/svm/sev.c void __init sev_hardware_setup(void) · line 3098

Quoted 11 lines

arch/x86/kvm/svm/sev.c +3 -1

@@ -3036,7 +3090,9 @@ void __init sev_hardware_setup(void)
               min_sev_asid, max_sev_asid);
       if (boot_cpu_has(X86_FEATURE_SEV_ES))
           pr_info("SEV-ES %s (ASIDs %u - %u)\n",
-            str_enabled_disabled(sev_es_supported),
+            sev_es_supported ? min_sev_es_asid < min_sev_asid ? "enabled" :
+                                        "unusable" :
+                                        "disabled",
               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",

If I set ciphertext_hiding_asids=99, I get the new 'unusable':

kvm_amd: SEV-SNP ciphertext hiding enabled
...
kvm_amd: SEV enabled (ASIDs 100 - 1006)
kvm_amd: SEV-ES unusable (ASIDs 100 - 99)
kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)

Quoted 1 line

Ok.

Which is correct.

This is similar to the SEV case where min_sev_asid can be greater than max_sev_asid and that also emits similarly :
SEV unusable (ASIDs 1007 - 1006) (this is an example of that case).

Quoted 9 lines

Now, if I set ciphertext_hiding_asids=0, I get:

kvm_amd: SEV-SNP ciphertext hiding enabled
...
kvm_amd: SEV enabled (ASIDs 100 - 1006)
kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
kvm_amd: SEV-SNP enabled (ASIDs 1 - 0)

Quoted 2 lines

..where SNP is unusable this time, yet it's not flagged as such.

Actually SNP still needs to be usable/enabled in this case, as specifying ciphertext_hiding_asids=0 is same as specifying that ciphertext hiding feature should not be enabled, so code-wise this is behaving correctly, but messaging needs to be fixed, which i will fix.

Quoted 9 lines

If there's no difference between "unusable" and not enabled, then I think it's better to keep the not enabled messaging behaviour and just not emit the line at all:  It's confusing to see the invalid "100 - 99" and "1 - 0" ranges.

Thanks,

Kim

Quoted 78 lines · Ashish

Hello Kim,

Quoted 36 lines · Kim Phillips

Hi Ashish,

I can confirm that this v5 series fixes v4's __sev_do_cmd_locked assertion failure problem, thanks.  More comments inline:

Quoted 1 line · Ashish Kalra

From: Ashish Kalra <[email protected]>

Extra From: line not necessary.

Quoted 26 lines

arch/x86/kvm/svm/sev.c +22 -0

@@ -2913,10 +2921,46 @@ static bool is_sev_snp_initialized(void)
       return initialized;
   }
   +static bool check_and_enable_sev_snp_ciphertext_hiding(void)
+{
+    unsigned int ciphertext_hiding_asid_nr = 0;
+
+    if (!sev_is_snp_ciphertext_hiding_supported()) {
+        pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
+        return false;
+    }
+
+    if (isdigit(ciphertext_hiding_asids[0])) {
+        if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
+            pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+                ciphertext_hiding_asids);
+            return false;
+        }
+        /* Do sanity checks on user-defined ciphertext_hiding_asids */
+        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
+            pr_warn("Requested ciphertext hiding ASIDs (%u) exceeds or equals minimum SEV ASID (%u)\n",
+                ciphertext_hiding_asid_nr, min_sev_asid);
+            return false;
+        }
+    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
+        ciphertext_hiding_asid_nr = min_sev_asid - 1;

Quoted 9 lines

arch/x86/kvm/svm/sev.c static bool is_sev_snp_initialized(void) · line 2951

Quoted 5 lines

arch/x86/kvm/svm/sev.c +5 -0

+    } else {
+        pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+            ciphertext_hiding_asids);
+        return false;
+    }

This code can be made much simpler if all the invalid cases were combined to emit a single pr_warn().

There definitely has to be a different pr_warn() for the sanity check case and invalid parameter cases and sanity check has to be done if the specified parameter is an unsigned int, so the check needs to be done separately.

I can definitely add a branch just for the invalid cases.

Quoted 14 lines

arch/x86/kvm/svm/sev.c void __init sev_hardware_setup(void) · line 3098

Quoted 11 lines

arch/x86/kvm/svm/sev.c +3 -1

@@ -3036,7 +3090,9 @@ void __init sev_hardware_setup(void)
               min_sev_asid, max_sev_asid);
       if (boot_cpu_has(X86_FEATURE_SEV_ES))
           pr_info("SEV-ES %s (ASIDs %u - %u)\n",
-            str_enabled_disabled(sev_es_supported),
+            sev_es_supported ? min_sev_es_asid < min_sev_asid ? "enabled" :
+                                        "unusable" :
+                                        "disabled",
               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",

If I set ciphertext_hiding_asids=99, I get the new 'unusable':

Quoted 7 lines

kvm_amd: SEV-SNP ciphertext hiding enabled
...
kvm_amd: SEV enabled (ASIDs 100 - 1006)
kvm_amd: SEV-ES unusable (ASIDs 100 - 99)
kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)

Ok.

Which is correct.

Quoted 3 lines

This is similar to the SEV case where min_sev_asid can be greater than max_sev_asid and that also emits similarly :
SEV unusable (ASIDs 1007 - 1006) (this is an example of that case).

Also do note that the message above is printing the exact values of min_sev_es_asid and max_sev_es_asid, as they have been computed.

And it adds that SEV-ES is now unusable as now min_sev_es_asid > max_sev_es_asid.

Quoted 8 lines

Quoted 8 lines

Now, if I set ciphertext_hiding_asids=0, I get:

kvm_amd: SEV-SNP ciphertext hiding enabled
...
kvm_amd: SEV enabled (ASIDs 100 - 1006)
kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
kvm_amd: SEV-SNP enabled (ASIDs 1 - 0)

Quoted 6 lines

Quoted 2 lines

..where SNP is unusable this time, yet it's not flagged as such.

Actually SNP still needs to be usable/enabled in this case, as specifying ciphertext_hiding_asids=0 is same as specifying that ciphertext hiding feature should not be enabled, so code-wise this is behaving correctly, but messaging needs to be fixed, which i will fix.

And i do need to fix this case for ciphertext_hiding_asids==0, i.e., ciphertext hiding feature is not enabled, as the above is not functioning correctly.

Thanks,
Ashish

Quoted 5 lines

Quoted 5 lines

If there's no difference between "unusable" and not enabled, then I think it's better to keep the not enabled messaging behaviour and just not emit the line at all:  It's confusing to see the invalid "100 - 99" and "1 - 0" ranges.

From: Ashish Kalra <[email protected]>

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

Ciphertext hiding further partitions the SEV-ES+ ASID space into SEV-ES and SEV-SNP.

Add new module parameter to the KVM module to enable Ciphertext hiding

support and a user configurable system-wide maximum SNP ASID value. If the module parameter value is "max" then the complete SEV-ES+ ASID

space is allocated to SEV-SNP guests.

Suggested-by Sean Christopherson <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../admin-guide/kernel-parameters.txt         | 19 ++++++
 arch/x86/kvm/svm/sev.c                        | 58 ++++++++++++++++++-
 2 files changed, 76 insertions(+), 1 deletion(-)

Documentation/admin-guide/kernel-parameters.txt +0 -0

index ee0735c6b8e2..05e50c37969e 100644

Documentation/admin-guide/kernel-parameters.txt +2 -0

+			SEV-ES.
+
 	kvm-arm.mode=
 			[KVM,ARM,EARLY] Select one of KVM/arm64's modes of
 			operation.

arch/x86/kvm/svm/sev.c +0 -0

index 89ce9e298201..16723b8e0e37 100644

arch/x86/kvm/svm/sev.c +4 -0

+
 #define AP_RESET_HOLD_NONE		0
 #define AP_RESET_HOLD_NAE_EVENT		1
 #define AP_RESET_HOLD_MSR_PROTO		2

@@ -200,6 +205,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 ciphertext hiding is enabled, effectively disabling SEV-ES
+	 * support.
 	 */
 	if (min_asid > max_asid)
 		return -ENOTTY;

arch/x86/kvm/svm/sev.c +4 -0

+				ciphertext_hiding_asid_nr, min_sev_asid);
+			return false;
+		}
+	} else if (!strcmp(ciphertext_hiding_asids, "max")) {
 
 

arch/x86/kvm/svm/sev.c +4 -0

+	return true;
+}
+
 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_ciphertext_hiding_enabled = false;
 	bool sev_snp_supported = false;

arch/x86/kvm/svm/sev.c +13 -1

 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3014,6 +3058,14 @@ void __init sev_hardware_setup(void)
 	min_sev_es_asid = min_snp_asid = 1;
 	max_sev_es_asid = max_snp_asid = min_sev_asid - 1;
 
+	/*
+	 * The ciphertext hiding feature partitions the joint SEV-ES/SEV-SNP
+	 * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
+	 * the SEV-SNP ASID starting at 1.
+	 */
+	if (ciphertext_hiding_asids[0])
+		snp_ciphertext_hiding_enabled = check_and_enable_sev_snp_ciphertext_hiding();
+
 	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;

@@ -3022,6 +3074,8 @@ void __init sev_hardware_setup(void)
 out:
 	if (sev_enabled) {
 		init_args.probe = true;
+		if (snp_ciphertext_hiding_enabled)
+			init_args.max_snp_asid = max_snp_asid;
 		if (sev_platform_init(&init_args))
 			sev_supported = sev_es_supported = sev_snp_supported = false;
 		else if (sev_snp_supported)

@@ -3036,7 +3090,9 @@ void __init sev_hardware_setup(void)
 			min_sev_asid, max_sev_asid);
 	if (boot_cpu_has(X86_FEATURE_SEV_ES))
 		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
-			str_enabled_disabled(sev_es_supported),
+			sev_es_supported ? min_sev_es_asid < min_sev_asid ? "enabled" :
+									    "unusable" :
+									    "disabled",
 			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",