Add SEV-SNP CipherTextHiding feature support

From: Ashish Kalra <[email protected]>

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

---
 include/uapi/linux/psp-sev.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

include/uapi/linux/psp-sev.h +4 -0

index 832c15d9155b..dd7298b67b37 100644
@@ -178,6 +178,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

include/uapi/linux/psp-sev.h +4 -1

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

From: Ashish Kalra <[email protected]>

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

---
 include/uapi/linux/psp-sev.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

include/uapi/linux/psp-sev.h +9 -1

index 832c15d9155b..dd7298b67b37 100644
@@ -178,6 +178,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

@@ -193,7 +197,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 */

From: Ashish Kalra <[email protected]>

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

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

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

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

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

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

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

index af018afd9cd7..564daf748293 100644
@@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct snp_feature_info);
 	default:				return 0;
 	}
 

@@ -1063,6 +1064,50 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void snp_get_platform_data(void)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct sev_data_snp_feature_info snp_feat_info;

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

+	 */
+	if (sev->snp_initialized)
+		return;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!rc && sev->snp_plat_status.feature_info) {
+		/*
+		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+		 * command to handle any alignment and page boundary check
+		 * requirements.
+		 */
+		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
+		snp_feat_info.length = sizeof(snp_feat_info);
+		snp_feat_info.ecx_in = 0;
+		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
+		if (!rc)
+			sev->feat_info = *feat_info;
+		kfree(feat_info);
+	}
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;

@@ -2415,6 +2460,8 @@ void sev_pci_init(void)
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);
 
+	snp_get_platform_data();
+
 	/* Initialize the platform */
 	args.probe = true;
 	rc = sev_platform_init(&args);

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

index 3e4e5574e88a..1c1a51e52d2b 100644
@@ -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);

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

index 903ddfea8585..6068a89839e1 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,
 };

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

Quoted 47 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

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

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

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

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

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

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

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

index af018afd9cd7..564daf748293 100644
@@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct snp_feature_info);
 	default:				return 0;
 	}
 

@@ -1063,6 +1064,50 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void snp_get_platform_data(void)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct sev_data_snp_feature_info snp_feat_info;

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1080

Quoted 10 lines

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

+	struct snp_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	int error = 0, rc;
+
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return;
+
+	/*
+	 * The output buffer must be firmware page if SEV-SNP is
+	 * initialized.

This comment is a little confusing relative to the "if" check that is performed. Add some more detail about what this check is for.

But... would this ever need to be called after SNP_INIT? Would we want to call this again after, say, a DOWNLOAD_FIRMWARE command?

Thanks,
Tom

Quoted 103 lines

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

+	 */
+	if (sev->snp_initialized)
+		return;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!rc && sev->snp_plat_status.feature_info) {
+		/*
+		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+		 * command to handle any alignment and page boundary check
+		 * requirements.
+		 */
+		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
+		snp_feat_info.length = sizeof(snp_feat_info);
+		snp_feat_info.ecx_in = 0;
+		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
+		if (!rc)
+			sev->feat_info = *feat_info;
+		kfree(feat_info);
+	}
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;

@@ -2415,6 +2460,8 @@ void sev_pci_init(void)
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);
 
+	snp_get_platform_data();
+
 	/* Initialize the platform */
 	args.probe = true;
 	rc = sev_platform_init(&args);

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

index 3e4e5574e88a..1c1a51e52d2b 100644
@@ -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);

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

index 903ddfea8585..6068a89839e1 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,
 };

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

Quoted 61 lines · Tom Lendacky

Quoted 47 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

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

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

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

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

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

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

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

index af018afd9cd7..564daf748293 100644
@@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct snp_feature_info);
 	default:				return 0;
 	}
 

@@ -1063,6 +1064,50 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void snp_get_platform_data(void)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct sev_data_snp_feature_info snp_feat_info;

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1080

Quoted 10 lines

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

+	struct snp_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	int error = 0, rc;
+
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return;
+
+	/*
+	 * The output buffer must be firmware page if SEV-SNP is
+	 * initialized.

This comment is a little confusing relative to the "if" check that is performed. Add some more detail about what this check is for.

Quoted 2 lines

But... would this ever need to be called after SNP_INIT? Would we want to call this again after, say, a DOWNLOAD_FIRMWARE command?

Although, as I hit send I realized that we only do DOWNLOAD_FIRMWARE before SNP is initialized (currently).

Thanks,
Tom

Quoted 106 lines

Thanks,
Tom

Quoted 103 lines

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

+	 */
+	if (sev->snp_initialized)
+		return;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!rc && sev->snp_plat_status.feature_info) {
+		/*
+		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+		 * command to handle any alignment and page boundary check
+		 * requirements.
+		 */
+		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
+		snp_feat_info.length = sizeof(snp_feat_info);
+		snp_feat_info.ecx_in = 0;
+		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
+		if (!rc)
+			sev->feat_info = *feat_info;
+		kfree(feat_info);
+	}
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;

@@ -2415,6 +2460,8 @@ void sev_pci_init(void)
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);
 
+	snp_get_platform_data();
+
 	/* Initialize the platform */
 	args.probe = true;
 	rc = sev_platform_init(&args);

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

index 3e4e5574e88a..1c1a51e52d2b 100644
@@ -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);

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

index 903ddfea8585..6068a89839e1 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,
 };

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

Hello Tom,

Quoted 62 lines · Tom Lendacky

Quoted 61 lines · Tom Lendacky

Quoted 47 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

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

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

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

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

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

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

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

index af018afd9cd7..564daf748293 100644
@@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct snp_feature_info);
 	default:				return 0;
 	}
 

@@ -1063,6 +1064,50 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void snp_get_platform_data(void)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct sev_data_snp_feature_info snp_feat_info;

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1080

Quoted 10 lines

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

+	struct snp_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	int error = 0, rc;
+
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return;
+
+	/*
+	 * The output buffer must be firmware page if SEV-SNP is
+	 * initialized.

This comment is a little confusing relative to the "if" check that is performed. Add some more detail about what this check is for.

Quoted 4 lines

Quoted 2 lines

But... would this ever need to be called after SNP_INIT? Would we want to call this again after, say, a DOWNLOAD_FIRMWARE command?

Although, as I hit send I realized that we only do DOWNLOAD_FIRMWARE before SNP is initialized (currently).

We do have DOWNLOAD_FIRMWARE_EX support coming up which can/will happen after SNP_INIT, but there we can still use SEV's PLATFORM_DATA command to get (updated) SEV/SNP firmware version.

Thanks, Ashish

Quoted 110 lines

Thanks,
Tom

Quoted 106 lines

Thanks,
Tom

Quoted 103 lines

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

+	 */
+	if (sev->snp_initialized)
+		return;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!rc && sev->snp_plat_status.feature_info) {
+		/*
+		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+		 * command to handle any alignment and page boundary check
+		 * requirements.
+		 */
+		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
+		snp_feat_info.length = sizeof(snp_feat_info);
+		snp_feat_info.ecx_in = 0;
+		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
+		if (!rc)
+			sev->feat_info = *feat_info;
+		kfree(feat_info);
+	}
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;

@@ -2415,6 +2460,8 @@ void sev_pci_init(void)
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);
 
+	snp_get_platform_data();
+
 	/* Initialize the platform */
 	args.probe = true;
 	rc = sev_platform_init(&args);

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

index 3e4e5574e88a..1c1a51e52d2b 100644
@@ -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);

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

index 903ddfea8585..6068a89839e1 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,
 };

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

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

Ciphertext hiding separates the ASID space into SNP guest ASIDs and host ASIDs. All SNP active guests must have an ASID less than or equal to MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests (SEV and SEV-ES) must be greater than MAX_SNP_ASID.

This patch-set adds a new module parameter to the CCP driver defined as max_snp_asid which is a user configurable MAX_SNP_ASID to define the system-wide maximum SNP ASID value. If this value is not set, then the ASID space is equally divided between SEV-SNP and SEV-ES guests.

Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this new module parameter has to added to the CCP driver.

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

---
 arch/x86/kvm/svm/sev.c       | 26 ++++++++++++++----
 drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++
 include/linux/psp-sev.h      | 12 +++++++--
 3 files changed, 83 insertions(+), 7 deletions(-)

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

index 0b851ef937f2..a345b4111ad6 100644
@@ -171,7 +171,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
        misc_cg_uncharge(type, sev->misc_cg, 1);
 }
 
-static int sev_asid_new(struct kvm_sev_info *sev)
+static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
 {
        /*
         * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.

@@ -199,6 +199,18 @@ static int sev_asid_new(struct kvm_sev_info *sev)
 
        mutex_lock(&sev_bitmap_lock);
 
+       /*
+        * When CipherTextHiding is enabled, all SNP guests must have an
+        * ASID less than or equal to MAX_SNP_ASID provided on the
+        * SNP_INIT_EX command and all the SEV-ES guests must have
+        * an ASID greater than MAX_SNP_ASID.
+        */
+       if (snp_cipher_text_hiding && sev->es_active) {
+               if (vm_type == KVM_X86_SNP_VM)
+                       max_asid = snp_max_snp_asid;
+               else
+                       min_asid = snp_max_snp_asid + 1;
+       }
 again:
        asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid);
        if (asid > max_asid) {

@@ -440,7 +452,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;
 

@@ -3059,14 +3071,18 @@ void __init sev_hardware_setup(void)
                                                                       "unusable" :
                                                                       "disabled",
                        min_sev_asid, max_sev_asid);
-       if (boot_cpu_has(X86_FEATURE_SEV_ES))
+       if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
+               if (snp_max_snp_asid >= (min_sev_asid - 1))
+                       sev_es_supported = false;
                pr_info("SEV-ES %s (ASIDs %u - %u)\n",
                        sev_es_supported ? "enabled" : "disabled",
-                       min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+                       min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
+                                                             0, min_sev_asid - 1);
+       }
        if (boot_cpu_has(X86_FEATURE_SEV_SNP))
                pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
                        sev_snp_supported ? "enabled" : "disabled",
-                       min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+                       min_sev_asid > 1 ? 1 : 0, snp_max_snp_asid ? : min_sev_asid - 1);
 
        sev_enabled = sev_supported;
        sev_es_enabled = sev_es_supported;

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

index 564daf748293..77900abb1b46 100644

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

+
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */
 
+/* Cipher Text Hiding Enabled */
+bool snp_cipher_text_hiding;
+EXPORT_SYMBOL(snp_cipher_text_hiding);
+
+/* MAX_SNP_ASID */
+unsigned int snp_max_snp_asid;
+EXPORT_SYMBOL(snp_max_snp_asid);
+
 static bool psp_dead;
 static int psp_timeout;
 

@@ -1064,6 +1080,38 @@ static void snp_set_hsave_pa(void *arg)
        wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error)
+{
+       struct psp_device *psp = psp_master;
+       struct sev_device *sev;
+       unsigned int edx;
+
+       sev = psp->sev_data;
+
+       /*
+        * Check if CipherTextHiding feature is supported and enabled
+        * in the Platform/BIOS.
+        */
+       if ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
+           sev->snp_plat_status.ciphertext_hiding_cap) {
+               /* Retrieve SEV CPUID information */
+               edx = cpuid_edx(0x8000001f);
+               /* Do sanity checks on user-defined MAX_SNP_ASID */
+               if (max_snp_asid >= edx) {
+                       dev_info(sev->dev, "max_snp_asid module parameter is not valid, limiting to %d\n",
+                                edx - 1);
+                       max_snp_asid = edx - 1;
+               }
+               snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2;
+
+               snp_cipher_text_hiding = 1;
+               data->ciphertext_hiding_en = 1;
+               data->max_snp_asid = snp_max_snp_asid;
+
+               dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n");
+       }
+}
+
 static void snp_get_platform_data(void)
 {
        struct sev_device *sev = psp_master->sev_data;

@@ -1199,6 +1247,10 @@ static int __sev_snp_init_locked(int *error)
                }
 
                memset(&data, 0, sizeof(data));
+
+               if (cipher_text_hiding)
+                       sev_snp_enable_ciphertext_hiding(&data, error);
+
                data.init_rmp = 1;
                data.list_paddr_en = 1;
                data.list_paddr = __psp_pa(snp_range_list);

include/linux/psp-sev.h +10 -3

index 6068a89839e1..2102248bd436 100644
@@ -27,6 +27,9 @@ enum sev_state {
        SEV_STATE_MAX
 };
 
+extern bool snp_cipher_text_hiding;
+extern unsigned int snp_max_snp_asid;
+
 /**
  * SEV platform and guest management commands
  */

@@ -746,10 +749,13 @@ struct sev_data_snp_guest_request {
 struct sev_data_snp_init_ex {
        u32 init_rmp:1;
        u32 list_paddr_en:1;
-       u32 rsvd:30;
+       u32 rapl_dis:1;
+       u32 ciphertext_hiding_en:1;
+       u32 rsvd:28;
        u32 rsvd1;
        u64 list_paddr;
-       u8  rsvd2[48];
+       u16 max_snp_asid;
+       u8  rsvd2[46];
 } __packed;
 
 /**

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

2.34.1

Quoted 97 lines · Ashish Kalra

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

Ciphertext hiding separates the ASID space into SNP guest ASIDs and host ASIDs. All SNP active guests must have an ASID less than or equal to MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests (SEV and SEV-ES) must be greater than MAX_SNP_ASID.

This patch-set adds a new module parameter to the CCP driver defined as max_snp_asid which is a user configurable MAX_SNP_ASID to define the system-wide maximum SNP ASID value. If this value is not set, then the ASID space is equally divided between SEV-SNP and SEV-ES guests.

Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this new module parameter has to added to the CCP driver.

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

---
 arch/x86/kvm/svm/sev.c       | 26 ++++++++++++++----
 drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++
 include/linux/psp-sev.h      | 12 +++++++--
 3 files changed, 83 insertions(+), 7 deletions(-)

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

index 0b851ef937f2..a345b4111ad6 100644
@@ -171,7 +171,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
        misc_cg_uncharge(type, sev->misc_cg, 1);
 }
 
-static int sev_asid_new(struct kvm_sev_info *sev)
+static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
 {
        /*
         * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.

@@ -199,6 +199,18 @@ static int sev_asid_new(struct kvm_sev_info *sev)
 
        mutex_lock(&sev_bitmap_lock);
 
+       /*
+        * When CipherTextHiding is enabled, all SNP guests must have an
+        * ASID less than or equal to MAX_SNP_ASID provided on the
+        * SNP_INIT_EX command and all the SEV-ES guests must have
+        * an ASID greater than MAX_SNP_ASID.
+        */
+       if (snp_cipher_text_hiding && sev->es_active) {
+               if (vm_type == KVM_X86_SNP_VM)
+                       max_asid = snp_max_snp_asid;
+               else
+                       min_asid = snp_max_snp_asid + 1;
+       }
 again:
        asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid);
        if (asid > max_asid) {

@@ -440,7 +452,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;
 

@@ -3059,14 +3071,18 @@ void __init sev_hardware_setup(void)
                                                                       "unusable" :
                                                                       "disabled",
                        min_sev_asid, max_sev_asid);
-       if (boot_cpu_has(X86_FEATURE_SEV_ES))
+       if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
+               if (snp_max_snp_asid >= (min_sev_asid - 1))
+                       sev_es_supported = false;
                pr_info("SEV-ES %s (ASIDs %u - %u)\n",
                        sev_es_supported ? "enabled" : "disabled",
-                       min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+                       min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
+                                                             0, min_sev_asid - 1);
+       }
        if (boot_cpu_has(X86_FEATURE_SEV_SNP))
                pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
                        sev_snp_supported ? "enabled" : "disabled",
-                       min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+                       min_sev_asid > 1 ? 1 : 0, snp_max_snp_asid ? : min_sev_asid - 1);
 
        sev_enabled = sev_supported;
        sev_es_enabled = sev_es_supported;

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

index 564daf748293..77900abb1b46 100644
@@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
 module_param(psp_init_on_probe, bool, 0444);
 MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
 

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 82

Quoted 7 lines

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

+static bool cipher_text_hiding = true;
+module_param(cipher_text_hiding, bool, 0444);
+MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
+
+static int max_snp_asid;
+module_param(max_snp_asid, int, 0444);
+MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");

My read of the spec is if Ciphertext hiding is not enabled there is no additional split in the ASID space. Am I understanding that correctly?

If so, I don't think we want to enable ciphertext hiding by default because it might break whatever management of ASIDs systems already have. For instance right now we have to split SEV-ES and SEV ASIDS, and SNP guests need SEV-ES ASIDS. This change would half the # of SNP enable ASIDs on a system.

Also should we move the ASID splitting code to be all in one place? Right now KVM handles it in sev_hardware_setup().

Quoted 110 lines

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

+
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */
 
+/* Cipher Text Hiding Enabled */
+bool snp_cipher_text_hiding;
+EXPORT_SYMBOL(snp_cipher_text_hiding);
+
+/* MAX_SNP_ASID */
+unsigned int snp_max_snp_asid;
+EXPORT_SYMBOL(snp_max_snp_asid);
+
 static bool psp_dead;
 static int psp_timeout;
 

@@ -1064,6 +1080,38 @@ static void snp_set_hsave_pa(void *arg)
        wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error)
+{
+       struct psp_device *psp = psp_master;
+       struct sev_device *sev;
+       unsigned int edx;
+
+       sev = psp->sev_data;
+
+       /*
+        * Check if CipherTextHiding feature is supported and enabled
+        * in the Platform/BIOS.
+        */
+       if ((sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
+           sev->snp_plat_status.ciphertext_hiding_cap) {
+               /* Retrieve SEV CPUID information */
+               edx = cpuid_edx(0x8000001f);
+               /* Do sanity checks on user-defined MAX_SNP_ASID */
+               if (max_snp_asid >= edx) {
+                       dev_info(sev->dev, "max_snp_asid module parameter is not valid, limiting to %d\n",
+                                edx - 1);
+                       max_snp_asid = edx - 1;
+               }
+               snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2;
+
+               snp_cipher_text_hiding = 1;
+               data->ciphertext_hiding_en = 1;
+               data->max_snp_asid = snp_max_snp_asid;
+
+               dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n");
+       }
+}
+
 static void snp_get_platform_data(void)
 {
        struct sev_device *sev = psp_master->sev_data;

@@ -1199,6 +1247,10 @@ static int __sev_snp_init_locked(int *error)
                }
 
                memset(&data, 0, sizeof(data));
+
+               if (cipher_text_hiding)
+                       sev_snp_enable_ciphertext_hiding(&data, error);
+
                data.init_rmp = 1;
                data.list_paddr_en = 1;
                data.list_paddr = __psp_pa(snp_range_list);

include/linux/psp-sev.h +10 -3

index 6068a89839e1..2102248bd436 100644
@@ -27,6 +27,9 @@ enum sev_state {
        SEV_STATE_MAX
 };
 
+extern bool snp_cipher_text_hiding;
+extern unsigned int snp_max_snp_asid;
+
 /**
  * SEV platform and guest management commands
  */

@@ -746,10 +749,13 @@ struct sev_data_snp_guest_request {
 struct sev_data_snp_init_ex {
        u32 init_rmp:1;
        u32 list_paddr_en:1;
-       u32 rsvd:30;
+       u32 rapl_dis:1;
+       u32 ciphertext_hiding_en:1;
+       u32 rsvd:28;
        u32 rsvd1;
        u64 list_paddr;
-       u8  rsvd2[48];
+       u16 max_snp_asid;
+       u8  rsvd2[46];
 } __packed;
 
 /**

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

2.34.1

Quoted 5 lines

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 82

Quoted 3 lines

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

+static int max_snp_asid;
+module_param(max_snp_asid, int, 0444);
+MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");

My read of the spec is if Ciphertext hiding is not enabled there is no additional split in the ASID space. Am I understanding that correctly?

Yes that is correct.

Quoted 5 lines

If so, I don't think we want to enable ciphertext hiding by default because it might break whatever management of ASIDs systems already have. For instance right now we have to split SEV-ES and SEV ASIDS, and SNP guests need SEV-ES ASIDS. This change would half the # of SNP enable ASIDs on a system.

My thought here is that we probably want to enable Ciphertext hiding by default as that should fix any security issues and concerns around SNP encryption as .Ciphertext hiding prevents host accesses from reading the ciphertext of SNP guest private memory.

This patch does add a new CCP module parameter, max_snp_asid, which can be used to dedicate all SEV-ES ASIDs to SNP guests.

Quoted 1 line

Quoted 2 lines

Also should we move the ASID splitting code to be all in one place? Right now KVM handles it in sev_hardware_setup().

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

This refactoring won't be part of the SNP ciphertext hiding support patches.

Quoted 20 lines

Quoted 5 lines

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 82

Quoted 3 lines

Quoted 1 line

+static int max_snp_asid;

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

+module_param(max_snp_asid, int, 0444);
+MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");

My read of the spec is if Ciphertext hiding is not enabled there is no additional split in the ASID space. Am I understanding that correctly?

Yes that is correct.

Quoted 5 lines

If so, I don't think we want to enable ciphertext hiding by default because it might break whatever management of ASIDs systems already have. For instance right now we have to split SEV-ES and SEV ASIDS, and SNP guests need SEV-ES ASIDS. This change would half the # of SNP enable ASIDs on a system.

My thought here is that we probably want to enable Ciphertext hiding by default as that should fix any security issues and concerns around SNP encryption as .Ciphertext hiding prevents host accesses from reading the ciphertext of SNP guest private memory.

This patch does add a new CCP module parameter, max_snp_asid, which can be used to dedicate all SEV-ES ASIDs to SNP guests.

Quoted 3 lines

Also should we move the ASID splitting code to be all in one place? Right now KVM handles it in sev_hardware_setup().

Quoted 3 lines

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

This refactoring won't be part of the SNP ciphertext hiding support patches.

Makes sense. I see Tom has asked you to split this patch into ccp and KVM.

Maybe add a line to the description so more are aware of the impending changes to asids?

I tested these patches a bit with the selftests / manually by backporting to 6.11-rc7. When you send a V3 I'll redo for a tag. BTW for some reason 6.12-rc1 and kvm/queue both fail to init SNP for me, then the kernel segfaults. Not sure whats going on there...

Hello Peter,

Quoted 9 lines · Peter Gonda

Quoted 8 lines · Ashish Kalra

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

index 564daf748293..77900abb1b46 100644
@@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
 module_param(psp_init_on_probe, bool, 0444);
 MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
 

Quoted 9 lines

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 82

Quoted 7 lines

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

+static bool cipher_text_hiding = true;
+module_param(cipher_text_hiding, bool, 0444);
+MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
+
+static int max_snp_asid;
+module_param(max_snp_asid, int, 0444);
+MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");

My read of the spec is if Ciphertext hiding is not enabled there is no additional split in the ASID space. Am I understanding that correctly?

Yes that is correct.

Quoted 5 lines

If so, I don't think we want to enable ciphertext hiding by default because it might break whatever management of ASIDs systems already have. For instance right now we have to split SEV-ES and SEV ASIDS, and SNP guests need SEV-ES ASIDS. This change would half the # of SNP enable ASIDs on a system.

My thought here is that we probably want to enable Ciphertext hiding by default as that should fix any security issues and concerns around SNP encryption as .Ciphertext hiding prevents host accesses from reading the ciphertext of SNP guest private memory.

This patch does add a new CCP module parameter, max_snp_asid, which can be used to dedicate all SEV-ES ASIDs to SNP guests.

Quoted 1 line

Quoted 2 lines

Also should we move the ASID splitting code to be all in one place? Right now KVM handles it in sev_hardware_setup().

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

This refactoring won't be part of the SNP ciphertext hiding support patches.

Quoted 43 lines · Ashish Kalra

Hello Peter,

Quoted 9 lines · Peter Gonda

Quoted 8 lines · Ashish Kalra

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

index 564daf748293..77900abb1b46 100644
@@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
 module_param(psp_init_on_probe, bool, 0444);
 MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
 

Quoted 9 lines

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 82

Quoted 7 lines

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

+static bool cipher_text_hiding = true;
+module_param(cipher_text_hiding, bool, 0444);
+MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
+
+static int max_snp_asid;
+module_param(max_snp_asid, int, 0444);
+MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");

My read of the spec is if Ciphertext hiding is not enabled there is no additional split in the ASID space. Am I understanding that correctly?

Yes that is correct.

Quoted 5 lines

If so, I don't think we want to enable ciphertext hiding by default because it might break whatever management of ASIDs systems already have. For instance right now we have to split SEV-ES and SEV ASIDS, and SNP guests need SEV-ES ASIDS. This change would half the # of SNP enable ASIDs on a system.

My thought here is that we probably want to enable Ciphertext hiding by default as that should fix any security issues and concerns around SNP encryption as .Ciphertext hiding prevents host accesses from reading the ciphertext of SNP guest private memory.

This patch does add a new CCP module parameter, max_snp_asid, which can be used to dedicate all SEV-ES ASIDs to SNP guests.

Quoted 3 lines

Also should we move the ASID splitting code to be all in one place? Right now KVM handles it in sev_hardware_setup().

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

Quoted 1 line

This refactoring won't be part of the SNP ciphertext hiding support patches.

It should, because that's not a "refactoring", that's a change of roles and responsibilities. And this series does the same; even worse, this series leaves things in a half-baked state, where the CCP and KVM have a weird shared ownership of ASID management.

I'm ok with essentially treating CipherText Hiding enablement as an extension of firmware, e.g. it's better than having to go into UEFI settings to toggle the feature on/off. But we need to have a clear, well-defined vision for how we want this all to look in the end.

Quoted 10 lines · Sean Christopherson

Quoted 4 lines · Ashish Kalra

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

This refactoring won't be part of the SNP ciphertext hiding support patches.

It should, because that's not a "refactoring", that's a change of roles and responsibilities. And this series does the same; even worse, this series leaves things in a half-baked state, where the CCP and KVM have a weird shared ownership of ASID management.

Sorry for the delayed reply to your response, the SNP DOWNLOAD_FIRMWARE_EX patches got posted in the meanwhile and that had additional considerations of moving SNP GCTX pages stuff into the PSP driver from KVM and that again got into this discussion about splitting ASID management across KVM and PSP driver and as you pointed out on those patches that there is zero reason that the PSP driver needs to care about ASIDs.

Well, CipherText Hiding (CTH) support is one reason where the PSP driver gets involved with ASIDs as CTH feature has to be enabled as part of SNP_INIT_EX and once CTH feature is enabled, the SEV-ES ASID space is split across SEV-SNP and SEV-ES VMs.

With reference to SNP GCTX pages, we are looking at some possibilities to push the requirement to update SNP GCTX pages to SNP firmware and remove that requirement from the kernel/KVM side.

Considering that, I will still like to keep ASID management in KVM, there are issues with locking, for example, sev_deactivate_lock is used to protect SNP ASID allocations (or actually for protecting ASID reuse/lazy-allocation requiring WBINVD/DF_FLUSH) and guarding this DF_FLUSH from VM destruction (DEACTIVATE). Moving ASID management stuff into PSP driver will then add complexity of adding this synchronization between different kernel modules or handling locking in two different kernel modules, to guard ASID allocation in PSP driver with VM destruction in KVM module.

There is also this sev_vmcbs[] array indexed by ASID (part of svm_cpu_data) which gets referenced during the ASID free code path in KVM. It just makes it simpler to keep ASID management stuff in KVM.

So probably we can add an API interface exported by the PSP driver something like is_sev_ciphertext_hiding_enabled() or sev_override_max_snp_asid()

Quoted 19 lines · Ashish Kalra

Quoted 10 lines · Sean Christopherson

Quoted 4 lines · Ashish Kalra

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

This refactoring won't be part of the SNP ciphertext hiding support patches.

It should, because that's not a "refactoring", that's a change of roles and responsibilities. And this series does the same; even worse, this series leaves things in a half-baked state, where the CCP and KVM have a weird shared ownership of ASID management.

Sorry for the delayed reply to your response, the SNP DOWNLOAD_FIRMWARE_EX patches got posted in the meanwhile and that had additional considerations of moving SNP GCTX pages stuff into the PSP driver from KVM and that again got into this discussion about splitting ASID management across KVM and PSP driver and as you pointed out on those patches that there is zero reason that the PSP driver needs to care about ASIDs.

Quoted 4 lines

Well, CipherText Hiding (CTH) support is one reason where the PSP driver gets involved with ASIDs as CTH feature has to be enabled as part of SNP_INIT_EX and once CTH feature is enabled, the SEV-ES ASID space is split across SEV-SNP and SEV-ES VMs.

Right, but that's just a case where KVM needs to react to the setup done by the PSP, correct? E.g. it's similar to SEV-ES being enabled/disabled in firmware, only that "firmware" happens to be a kernel driver.

Quoted 4 lines

With reference to SNP GCTX pages, we are looking at some possibilities to push the requirement to update SNP GCTX pages to SNP firmware and remove that requirement from the kernel/KVM side.

Heh, that'd work too.

Quoted 9 lines

Considering that, I will still like to keep ASID management in KVM, there are issues with locking, for example, sev_deactivate_lock is used to protect SNP ASID allocations (or actually for protecting ASID reuse/lazy-allocation requiring WBINVD/DF_FLUSH) and guarding this DF_FLUSH from VM destruction (DEACTIVATE). Moving ASID management stuff into PSP driver will then add complexity of adding this synchronization between different kernel modules or handling locking in two different kernel modules, to guard ASID allocation in PSP driver with VM destruction in KVM module.

Quoted 6 lines

There is also this sev_vmcbs[] array indexed by ASID (part of svm_cpu_data) which gets referenced during the ASID free code path in KVM. It just makes it simpler to keep ASID management stuff in KVM.

So probably we can add an API interface exported by the PSP driver something like is_sev_ciphertext_hiding_enabled() or sev_override_max_snp_asid()

What about adding a cc_attr_flags entry?

Quoted 20 lines · Sean Christopherson

Quoted 19 lines · Ashish Kalra

Quoted 10 lines · Sean Christopherson

Quoted 4 lines · Ashish Kalra

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

This refactoring won't be part of the SNP ciphertext hiding support patches.

It should, because that's not a "refactoring", that's a change of roles and responsibilities. And this series does the same; even worse, this series leaves things in a half-baked state, where the CCP and KVM have a weird shared ownership of ASID management.

Sorry for the delayed reply to your response, the SNP DOWNLOAD_FIRMWARE_EX patches got posted in the meanwhile and that had additional considerations of moving SNP GCTX pages stuff into the PSP driver from KVM and that again got into this discussion about splitting ASID management across KVM and PSP driver and as you pointed out on those patches that there is zero reason that the PSP driver needs to care about ASIDs.

Quoted 8 lines

Quoted 4 lines

Well, CipherText Hiding (CTH) support is one reason where the PSP driver gets involved with ASIDs as CTH feature has to be enabled as part of SNP_INIT_EX and once CTH feature is enabled, the SEV-ES ASID space is split across SEV-SNP and SEV-ES VMs.

Right, but that's just a case where KVM needs to react to the setup done by the PSP, correct? E.g. it's similar to SEV-ES being enabled/disabled in firmware, only that "firmware" happens to be a kernel driver.

Yes that is true.

Quoted 23 lines

Quoted 4 lines

With reference to SNP GCTX pages, we are looking at some possibilities to push the requirement to update SNP GCTX pages to SNP firmware and remove that requirement from the kernel/KVM side.

Heh, that'd work too.

Quoted 15 lines

Considering that, I will still like to keep ASID management in KVM, there are issues with locking, for example, sev_deactivate_lock is used to protect SNP ASID allocations (or actually for protecting ASID reuse/lazy-allocation requiring WBINVD/DF_FLUSH) and guarding this DF_FLUSH from VM destruction (DEACTIVATE). Moving ASID management stuff into PSP driver will then add complexity of adding this synchronization between different kernel modules or handling locking in two different kernel modules, to guard ASID allocation in PSP driver with VM destruction in KVM module.

There is also this sev_vmcbs[] array indexed by ASID (part of svm_cpu_data) which gets referenced during the ASID free code path in KVM. It just makes it simpler to keep ASID management stuff in KVM.

So probably we can add an API interface exported by the PSP driver something like is_sev_ciphertext_hiding_enabled() or sev_override_max_snp_asid()

Quoted 1 line

What about adding a cc_attr_flags entry?

Yes, that is a possibility i will look into.

But, along with an additional cc_attr_flags entry, max_snp_asid (which is a PSP driver module parameter) also needs to be propagated to KVM, that's what i was considering passing as parameter to the above API interface.

Quoted 56 lines · Ashish

Quoted 20 lines · Sean Christopherson

Quoted 19 lines · Ashish Kalra

Quoted 10 lines · Sean Christopherson

Quoted 4 lines · Ashish Kalra

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

This refactoring won't be part of the SNP ciphertext hiding support patches.

It should, because that's not a "refactoring", that's a change of roles and responsibilities. And this series does the same; even worse, this series leaves things in a half-baked state, where the CCP and KVM have a weird shared ownership of ASID management.

Sorry for the delayed reply to your response, the SNP DOWNLOAD_FIRMWARE_EX patches got posted in the meanwhile and that had additional considerations of moving SNP GCTX pages stuff into the PSP driver from KVM and that again got into this discussion about splitting ASID management across KVM and PSP driver and as you pointed out on those patches that there is zero reason that the PSP driver needs to care about ASIDs.

Quoted 8 lines

Quoted 4 lines

Well, CipherText Hiding (CTH) support is one reason where the PSP driver gets involved with ASIDs as CTH feature has to be enabled as part of SNP_INIT_EX and once CTH feature is enabled, the SEV-ES ASID space is split across SEV-SNP and SEV-ES VMs.

Right, but that's just a case where KVM needs to react to the setup done by the PSP, correct? E.g. it's similar to SEV-ES being enabled/disabled in firmware, only that "firmware" happens to be a kernel driver.

Yes that is true.

Quoted 23 lines

Quoted 4 lines

With reference to SNP GCTX pages, we are looking at some possibilities to push the requirement to update SNP GCTX pages to SNP firmware and remove that requirement from the kernel/KVM side.

Heh, that'd work too.

Quoted 15 lines

Considering that, I will still like to keep ASID management in KVM, there are issues with locking, for example, sev_deactivate_lock is used to protect SNP ASID allocations (or actually for protecting ASID reuse/lazy-allocation requiring WBINVD/DF_FLUSH) and guarding this DF_FLUSH from VM destruction (DEACTIVATE). Moving ASID management stuff into PSP driver will then add complexity of adding this synchronization between different kernel modules or handling locking in two different kernel modules, to guard ASID allocation in PSP driver with VM destruction in KVM module.

There is also this sev_vmcbs[] array indexed by ASID (part of svm_cpu_data) which gets referenced during the ASID free code path in KVM. It just makes it simpler to keep ASID management stuff in KVM.

So probably we can add an API interface exported by the PSP driver something like is_sev_ciphertext_hiding_enabled() or sev_override_max_snp_asid()

Quoted 7 lines

Quoted 1 line

What about adding a cc_attr_flags entry?

Yes, that is a possibility i will look into.

But, along with an additional cc_attr_flags entry, max_snp_asid (which is a PSP driver module parameter) also needs to be propagated to KVM, that's what i was considering passing as parameter to the above API interface.

Adding a new cc_attr_flags entry indicating CTH support is enabled.

And as discussed with Boris, using the cc_platform_set() to add a new attr like max_asid and adding a getter interface on top to return the max_snp_asid.

Quoted 57 lines · Ashish Kalra

Quoted 56 lines · Ashish

Quoted 20 lines · Sean Christopherson

Quoted 19 lines · Ashish Kalra

Quoted 10 lines · Sean Christopherson

Quoted 4 lines · Ashish Kalra

Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module.

This refactoring won't be part of the SNP ciphertext hiding support patches.

It should, because that's not a "refactoring", that's a change of roles and responsibilities. And this series does the same; even worse, this series leaves things in a half-baked state, where the CCP and KVM have a weird shared ownership of ASID management.

Sorry for the delayed reply to your response, the SNP DOWNLOAD_FIRMWARE_EX patches got posted in the meanwhile and that had additional considerations of moving SNP GCTX pages stuff into the PSP driver from KVM and that again got into this discussion about splitting ASID management across KVM and PSP driver and as you pointed out on those patches that there is zero reason that the PSP driver needs to care about ASIDs.

Quoted 8 lines

Quoted 4 lines

Well, CipherText Hiding (CTH) support is one reason where the PSP driver gets involved with ASIDs as CTH feature has to be enabled as part of SNP_INIT_EX and once CTH feature is enabled, the SEV-ES ASID space is split across SEV-SNP and SEV-ES VMs.

Right, but that's just a case where KVM needs to react to the setup done by the PSP, correct? E.g. it's similar to SEV-ES being enabled/disabled in firmware, only that "firmware" happens to be a kernel driver.

Yes that is true.

Quoted 23 lines

Quoted 4 lines

With reference to SNP GCTX pages, we are looking at some possibilities to push the requirement to update SNP GCTX pages to SNP firmware and remove that requirement from the kernel/KVM side.

Heh, that'd work too.

Quoted 15 lines

Considering that, I will still like to keep ASID management in KVM, there are issues with locking, for example, sev_deactivate_lock is used to protect SNP ASID allocations (or actually for protecting ASID reuse/lazy-allocation requiring WBINVD/DF_FLUSH) and guarding this DF_FLUSH from VM destruction (DEACTIVATE). Moving ASID management stuff into PSP driver will then add complexity of adding this synchronization between different kernel modules or handling locking in two different kernel modules, to guard ASID allocation in PSP driver with VM destruction in KVM module.

There is also this sev_vmcbs[] array indexed by ASID (part of svm_cpu_data) which gets referenced during the ASID free code path in KVM. It just makes it simpler to keep ASID management stuff in KVM.

So probably we can add an API interface exported by the PSP driver something like is_sev_ciphertext_hiding_enabled() or sev_override_max_snp_asid()

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

What about adding a cc_attr_flags entry?

Yes, that is a possibility i will look into.

But, along with an additional cc_attr_flags entry, max_snp_asid (which is a PSP driver module parameter) also needs to be propagated to KVM, that's what i was considering passing as parameter to the above API interface.

Doh, right, I managed to forget about those module params.

Quoted 5 lines

Adding a new cc_attr_flags entry indicating CTH support is enabled.

And as discussed with Boris, using the cc_platform_set() to add a new attr like max_asid and adding a getter interface on top to return the max_snp_asid.

Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of the APIs that invoke it are flawed, and make all of this way more confusing and convoluted than it needs to be.

IIUC, SNP initialization is forced during probe purely because SNP can't be initialized if VMs are running. But the only in-tree user of SEV-XXX functionality is KVM, and KVM depends on whatever this driver is called. So forcing SNP initialization because a hypervisor could be running legacy VMs make no sense. Just require KVM to initialize SEV functionality if KVM wants to use SEV+.

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */
	rc = __sev_snp_init_locked(&args->error); 

Rather than automatically init SEV+ functionality, can we instead do something like the (half-baked pseudo-patch) below? I.e. delete all paths that implicitly init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use SEV+. Then we can put the CipherText and SNP ASID params in KVM.

That would also allow (a) registering the SNP panic notifier if and only if SNP is actually initailized and (b) shutting down SEV+ in the PSP when KVM is unloaded. Arguably, the PSP should be shutdown when KVM is unloaded, irrespective of the CipherText and SNP ASID knobs. But with those knobs, it becomes even more desirable, because it would allow userspace to reload *KVM* in order to change the CipherText and SNP ASID module params. I.e. doesn't require unloading the entire CCP driver.

If dropping the implicit initialization in some of the ioctls would break existing userspace, then maybe we could add a module param (or Kconfig?) to preserve that behavior? I'm not familiar with what actually uses /dev/sev.

Side topic #1, sev_pci_init() is buggy. It should destroy SEV if getting the API version fails after a firmware update.

Side topic #2, the version check is broken, as it returns "success" when initialization quite obviously failed.

	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
		return 0;
	}

---
 drivers/crypto/ccp/sev-dev.c | 102 +++++++++--------------------------
 include/linux/psp-sev.h      |  19 ++-----
 2 files changed, 28 insertions(+), 93 deletions(-)

drivers/crypto/ccp/sev-dev.c +25 -77

index af018afd9cd7..563cc235b095 100644
@@ -69,10 +69,6 @@ static char *init_ex_path;
 module_param(init_ex_path, charp, 0444);
 MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
 
-static bool psp_init_on_probe = true;
-module_param(psp_init_on_probe, bool, 0444);
-MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
-
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */

@@ -1306,11 +1302,13 @@ static int __sev_platform_init_locked(int *error)
 	return 0;
 }
 
-static int _sev_platform_init_locked(struct sev_platform_init_args *args)
+int sev_platform_init(int *error)
 {
 	struct sev_device *sev;
 	int rc;
 
+	guard(mutex)(&sev_cmd_mutex)
+
 	if (!psp_master || !psp_master->sev_data)
 		return -ENODEV;
 

@@ -1319,36 +1317,17 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 	if (sev->state == SEV_STATE_INIT)
 		return 0;
 
-	/*
-	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
-	 * so perform SEV-SNP initialization at probe time.
-	 */
-	rc = __sev_snp_init_locked(&args->error);
+	rc = __sev_snp_init_locked(error);
 	if (rc && rc != -ENODEV) {
 		/*
 		 * Don't abort the probe if SNP INIT failed,
 		 * continue to initialize the legacy SEV firmware.
 		 */
 		dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n",
-			rc, args->error);
+			rc, *error);
 	}
 
-	/* Defer legacy SEV/SEV-ES support if allowed by caller/module. */
-	if (args->probe && !psp_init_on_probe)
-		return 0;
-
-	return __sev_platform_init_locked(&args->error);
-}
-
-int sev_platform_init(struct sev_platform_init_args *args)
-{
-	int rc;
-
-	mutex_lock(&sev_cmd_mutex);
-	rc = _sev_platform_init_locked(args);
-	mutex_unlock(&sev_cmd_mutex);
-
-	return rc;
+	return __sev_platform_init_locked(error);
 }
 EXPORT_SYMBOL_GPL(sev_platform_init);
 

@@ -1441,16 +1420,12 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	int rc;
 
 	if (!writable)
 		return -EPERM;
 
-	if (sev->state == SEV_STATE_UNINIT) {
-		rc = __sev_platform_init_locked(&argp->error);
-		if (rc)
-			return rc;
-	}
+	if (sev->state == SEV_STATE_UNINIT)
+		return -ENOTTY;
 
 	return __sev_do_cmd_locked(cmd, NULL, &argp->error);
 }

@@ -1467,6 +1442,9 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	if (!writable)
 		return -EPERM;
 
+	if (sev->state == SEV_STATE_UNINIT)
+		return -ENOTTY;
+
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 

@@ -1489,12 +1467,6 @@ 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) {
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
-			goto e_free_blob;
-	}
-
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
 
 	 /* If we query the CSR length, FW responded with expected data. */

@@ -1584,7 +1556,6 @@ static int sev_get_firmware(struct device *dev,
 	return -ENOENT;
 }
 
-/* Don't fail if SEV FW couldn't be updated. Continue with existing SEV FW */
 static int sev_update_firmware(struct device *dev)
 {
 	struct sev_data_download_firmware *data;

@@ -1732,6 +1703,9 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	if (!writable)
 		return -EPERM;
 
+	if (sev->state == SEV_STATE_UNINIT)
+		return -ENOTTY;
+
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 

@@ -1754,16 +1728,8 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	data.oca_cert_address = __psp_pa(oca_blob);
 	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) {
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
-			goto e_free_oca;
-	}
-
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
 
-e_free_oca:
 	kfree(oca_blob);
 e_free_pek:
 	kfree(pek_blob);

@@ -1882,15 +1848,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	void __user *input_pdh_cert_address;
 	int ret;
 
-	/* If platform is not in INIT state then transition it to INIT. */
-	if (sev->state != SEV_STATE_INIT) {
-		if (!writable)
-			return -EPERM;
-
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
-			return ret;
-	}
+	if (sev->state != SEV_STATE_INIT)
+		return -ENOTTY;
 
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;

@@ -2296,6 +2255,9 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
 {
 	int error;
 
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &snp_panic_notifier);
+
 	__sev_platform_shutdown_locked(NULL);
 
 	if (sev_es_tmr) {

@@ -2390,9 +2352,7 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
 void sev_pci_init(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	struct sev_platform_init_args args = {0};
 	u8 api_major, api_minor, build;
-	int rc;
 
 	if (!sev)
 		return;

@@ -2406,27 +2366,18 @@ void sev_pci_init(void)
 	api_minor = sev->api_minor;
 	build     = sev->build;
 
-	if (sev_update_firmware(sev->dev) == 0)
-		sev_get_api_version();
+	/* Don't fail if SEV FW couldn't be updated. Continue with existing SEV FW. */
+	if (sev_update_firmware(sev->dev))
+		return;
+
+	if (sev_get_api_version())
+		goto err;
 
 	if (api_major != sev->api_major || api_minor != sev->api_minor ||
 	    build != sev->build)
 		dev_info(sev->dev, "SEV firmware updated from %d.%d.%d to %d.%d.%d\n",
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);
-
-	/* Initialize the platform */
-	args.probe = true;
-	rc = sev_platform_init(&args);
-	if (rc)
-		dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
-			args.error, rc);
-
-	dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_initialized ?
-		"-SNP" : "", sev->api_major, sev->api_minor, sev->build);
-
-	atomic_notifier_chain_register(&panic_notifier_list,
-				       &snp_panic_notifier);
 	return;
 
 err:

@@ -2443,7 +2394,4 @@ void sev_pci_exit(void)
 		return;
 
 	sev_firmware_shutdown(sev);
-
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					 &snp_panic_notifier);
 }

include/linux/psp-sev.h +3 -16

index 903ddfea8585..def40f7ea01d 100644
@@ -790,19 +790,6 @@ struct sev_data_snp_shutdown_ex {
 	u32 rsvd1:31;
 } __packed;
 
-/**
- * struct sev_platform_init_args
- *
- * @error: SEV firmware error code
- * @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
- */
-struct sev_platform_init_args {
-	int error;
-	bool probe;
-};
-
 /**
  * struct sev_data_snp_commit - SNP_COMMIT structure
  *

@@ -817,7 +804,7 @@ struct sev_data_snp_commit {
 /**
  * sev_platform_init - perform SEV INIT command
  *
- * @args: struct sev_platform_init_args to pass in arguments
+ * @error: SEV firmware error code
  *
  * Returns:
  * 0 if the SEV successfully processed the command

@@ -826,7 +813,7 @@ struct sev_data_snp_commit {
  * -%ETIMEDOUT if the SEV command timed out
  * -%EIO       if the SEV returned a non-zero return code
  */
-int sev_platform_init(struct sev_platform_init_args *args);
+int sev_platform_init(int *error);
 
 /**
  * sev_platform_status - perform SEV PLATFORM_STATUS command

@@ -951,7 +938,7 @@ void snp_free_firmware_page(void *addr);
 static inline int
 sev_platform_status(struct sev_user_data_status *status, int *error) { return -ENODEV; }
 
-static inline int sev_platform_init(struct sev_platform_init_args *args) { return -ENODEV; }
+static inline int sev_platform_init(int *error) { return -ENODEV; }
 
 static inline int
 sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { return -ENODEV; }

base-commit: 43fb83c17ba2d63dfb798f0be7453ed55ca3f9c2

Quoted 5 lines · Sean Christopherson

On Thu, Nov 21, 2024, Ashish Kalra wrote:
Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of the APIs that invoke it are flawed, and make all of this way more confusing and convoluted than it needs to be.

Quoted 5 lines

IIUC, SNP initialization is forced during probe purely because SNP can't be initialized if VMs are running. But the only in-tree user of SEV-XXX functionality is KVM, and KVM depends on whatever this driver is called. So forcing SNP initialization because a hypervisor could be running legacy VMs make no sense. Just require KVM to initialize SEV functionality if KVM wants to use SEV+.

When we say legacy VMs, that also means non-SEV VMs. So you can't have any VM running within a VMRUN instruction.

Or...

Quoted 11 lines

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */
	rc = __sev_snp_init_locked(&args->error); 

Rather than automatically init SEV+ functionality, can we instead do something like the (half-baked pseudo-patch) below? I.e. delete all paths that implicitly init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use SEV+. Then we can put the CipherText and SNP ASID params in KVM.

... do you mean at module load time (based on the module parameters)? Or when the first SEV VM is run? I would think the latter, as the parameters are all true by default. If the latter, that would present a problem of having to ensure no VMs are active while performing the SNP_INIT.

Quoted 7 lines

That would also allow (a) registering the SNP panic notifier if and only if SNP is actually initailized and (b) shutting down SEV+ in the PSP when KVM is unloaded. Arguably, the PSP should be shutdown when KVM is unloaded, irrespective of the CipherText and SNP ASID knobs. But with those knobs, it becomes even more desirable, because it would allow userspace to reload *KVM* in order to change the CipherText and SNP ASID module params. I.e. doesn't require unloading the entire CCP driver.

Quoted 6 lines

If dropping the implicit initialization in some of the ioctls would break existing userspace, then maybe we could add a module param (or Kconfig?) to preserve that behavior? I'm not familiar with what actually uses /dev/sev.

Side topic #1, sev_pci_init() is buggy. It should destroy SEV if getting the API version fails after a firmware update.

True, we'll look at doing a fix for that.

Quoted 3 lines

Side topic #2, the version check is broken, as it returns "success" when initialization quite obviously failed.

That is ok because you can still initialize SEV / SEV-ES support.

Quoted 6 lines · Tom Lendacky

Quoted 5 lines · Sean Christopherson

On Thu, Nov 21, 2024, Ashish Kalra wrote:
Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of the APIs that invoke it are flawed, and make all of this way more confusing and convoluted than it needs to be.

Quoted 8 lines

Quoted 5 lines

IIUC, SNP initialization is forced during probe purely because SNP can't be initialized if VMs are running. But the only in-tree user of SEV-XXX functionality is KVM, and KVM depends on whatever this driver is called. So forcing SNP initialization because a hypervisor could be running legacy VMs make no sense. Just require KVM to initialize SEV functionality if KVM wants to use SEV+.

When we say legacy VMs, that also means non-SEV VMs. So you can't have any VM running within a VMRUN instruction.

Yeah, I know. But if KVM initializes the PSP SEV stuff when KVM is loaded, then KVM can't possibly be running VMs of any kind.

Quoted 14 lines

Or...

Quoted 11 lines

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */
	rc = __sev_snp_init_locked(&args->error); 

Rather than automatically init SEV+ functionality, can we instead do something like the (half-baked pseudo-patch) below? I.e. delete all paths that implicitly init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use SEV+. Then we can put the CipherText and SNP ASID params in KVM.

Quoted 4 lines

... do you mean at module load time (based on the module parameters)? Or when the first SEV VM is run? I would think the latter, as the parameters are all true by default. If the latter, that would present a problem of having to ensure no VMs are active while performing the SNP_INIT.

kvm-amd.ko load time.

Quoted 8 lines

Quoted 8 lines

That would also allow (a) registering the SNP panic notifier if and only if SNP is actually initailized and (b) shutting down SEV+ in the PSP when KVM is unloaded. Arguably, the PSP should be shutdown when KVM is unloaded, irrespective of the CipherText and SNP ASID knobs. But with those knobs, it becomes even more desirable, because it would allow userspace to reload *KVM* in order to change the CipherText and SNP ASID module params. I.e. doesn't require unloading the entire CCP driver.

Quoted 4 lines

Quoted 4 lines

If dropping the implicit initialization in some of the ioctls would break existing userspace, then maybe we could add a module param (or Kconfig?) to preserve that behavior? I'm not familiar with what actually uses /dev/sev.

Quoted 10 lines

Quoted 2 lines

Side topic #1, sev_pci_init() is buggy. It should destroy SEV if getting the API version fails after a firmware update.

True, we'll look at doing a fix for that.

Quoted 3 lines

Side topic #2, the version check is broken, as it returns "success" when initialization quite obviously failed.

That is ok because you can still initialize SEV / SEV-ES support.

Right, but as I've complained elsewhere, KVM shouldn't think SNP is supported when in reality firmware is effectively too old.

Quoted 33 lines · Sean Christopherson

Quoted 6 lines · Tom Lendacky

Quoted 5 lines · Sean Christopherson

On Thu, Nov 21, 2024, Ashish Kalra wrote:
Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of the APIs that invoke it are flawed, and make all of this way more confusing and convoluted than it needs to be.

Quoted 8 lines

Quoted 5 lines

IIUC, SNP initialization is forced during probe purely because SNP can't be initialized if VMs are running. But the only in-tree user of SEV-XXX functionality is KVM, and KVM depends on whatever this driver is called. So forcing SNP initialization because a hypervisor could be running legacy VMs make no sense. Just require KVM to initialize SEV functionality if KVM wants to use SEV+.

When we say legacy VMs, that also means non-SEV VMs. So you can't have any VM running within a VMRUN instruction.

Yeah, I know. But if KVM initializes the PSP SEV stuff when KVM is loaded, then KVM can't possibly be running VMs of any kind.

Quoted 14 lines

Or...

Quoted 11 lines

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */
	rc = __sev_snp_init_locked(&args->error); 

Rather than automatically init SEV+ functionality, can we instead do something like the (half-baked pseudo-patch) below? I.e. delete all paths that implicitly init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use SEV+. Then we can put the CipherText and SNP ASID params in KVM.

Quoted 6 lines

Quoted 4 lines

... do you mean at module load time (based on the module parameters)? Or when the first SEV VM is run? I would think the latter, as the parameters are all true by default. If the latter, that would present a problem of having to ensure no VMs are active while performing the SNP_INIT.

kvm-amd.ko load time.

Ok, so kvm module load will init SEV+ if indicated by it's module parameters.

But, there are additional concerns here.

SNP will still have to be initialized first, because SNP_INIT will fail if SEV INIT has been done.

Additionally, to support SEV firmware hotloading (DLFW_EX), SEV can't be initialized.

So probably, we will have to retain some PSP style SEV+ initialization here, SNP_INIT is always done first and then SEV INIT is skipped if explicitly specified by a module param. This allows SEV firmware hotloading to be supported.

But, then with SEV firmware hotload support how do we do SEV INIT without unloading and reloading KVM module ?

This can reuse the current support (in KVM) to do SEV INIT implicitly when the first SEV VM is run: sev_guest_init() -> sev_platform_init()

Quoted 51 lines · Ashish Kalra

Quoted 33 lines · Sean Christopherson

Quoted 6 lines · Tom Lendacky

Quoted 5 lines · Sean Christopherson

On Thu, Nov 21, 2024, Ashish Kalra wrote:
Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of the APIs that invoke it are flawed, and make all of this way more confusing and convoluted than it needs to be.

Quoted 8 lines

Quoted 5 lines

IIUC, SNP initialization is forced during probe purely because SNP can't be initialized if VMs are running. But the only in-tree user of SEV-XXX functionality is KVM, and KVM depends on whatever this driver is called. So forcing SNP initialization because a hypervisor could be running legacy VMs make no sense. Just require KVM to initialize SEV functionality if KVM wants to use SEV+.

When we say legacy VMs, that also means non-SEV VMs. So you can't have any VM running within a VMRUN instruction.

Yeah, I know. But if KVM initializes the PSP SEV stuff when KVM is loaded, then KVM can't possibly be running VMs of any kind.

Quoted 14 lines

Or...

Quoted 11 lines

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */
	rc = __sev_snp_init_locked(&args->error); 

Rather than automatically init SEV+ functionality, can we instead do something like the (half-baked pseudo-patch) below? I.e. delete all paths that implicitly init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use SEV+. Then we can put the CipherText and SNP ASID params in KVM.

Quoted 6 lines

Quoted 4 lines

... do you mean at module load time (based on the module parameters)? Or when the first SEV VM is run? I would think the latter, as the parameters are all true by default. If the latter, that would present a problem of having to ensure no VMs are active while performing the SNP_INIT.

kvm-amd.ko load time.

Ok, so kvm module load will init SEV+ if indicated by it's module parameters.

But, there are additional concerns here.

SNP will still have to be initialized first, because SNP_INIT will fail if SEV INIT has been done.

Additionally, to support SEV firmware hotloading (DLFW_EX), SEV can't be initialized.

Quoted 7 lines

So probably, we will have to retain some PSP style SEV+ initialization here, SNP_INIT is always done first and then SEV INIT is skipped if explicitly specified by a module param. This allows SEV firmware hotloading to be supported.

But, then with SEV firmware hotload support how do we do SEV INIT without unloading and reloading KVM module ?

So the above says:

SEV_CMD_SNP_INIT{_ES} cannot be executed if SEV_CMD_INIT{_EX} has been executed.

but the existing comment in _sev_platform_init_locked() says:

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */

Which one is correct? I don't think it matters in the end, just trying to wrap my head around everything.

And IIUC, SEV_CMD_SNP_INIT{_EX} can be executed before firmware hotload, but SEV_CMD_INIT{_EX} cannot. Is that correct? Because if firmware hotload can't be done while SEV VMs are _active_, then that's a very different situation.

Quoted 2 lines

This can reuse the current support (in KVM) to do SEV INIT implicitly when the first SEV VM is run: sev_guest_init() -> sev_platform_init()

I don't love the implicit behavior, but assuming hotloading firmware can't be done after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.

To summarize, if the above assumptions hold:

1. Initialize SNP when kvm-amd.ko is loaded.
2. Define CipherTextHiding and ASID params kvm-amd.ko.
3. Initialize SEV+ at first use.

Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is built-in, correct? I.e. doesn't requires deferring kvm-amd.ko load until after firmware hotloading.

Quoted 6 lines · Sean Christopherson

Quoted 2 lines

This can reuse the current support (in KVM) to do SEV INIT implicitly when the first SEV VM is run: sev_guest_init() -> sev_platform_init()

I don't love the implicit behavior, but assuming hotloading firmware can't be done after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.

Quoted 5 lines

To summarize, if the above assumptions hold:

1. Initialize SNP when kvm-amd.ko is loaded.
2. Define CipherTextHiding and ASID params kvm-amd.ko.
3. Initialize SEV+ at first use.

Yes, the above summary is correct except for (3).

The initial set of patches will initialize SNP and SEV both at kvm-amd.ko module load, similar to PSP module load/probe time.

For backward compatibility, the PSP module parameter psp_init_on_probe will still be supported, i believe it is used for INIT_EX support.

Quoted 3 lines

Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is built-in, correct? I.e. doesn't requires deferring kvm-amd.ko load until after firmware hotloading.

Yes, this should work, for supporting firmware hotloading, the PSP driver's psp_init_on_probe parameter will need to be set to false, which will ensure that SEV INIT is not done during SEV/SNP platform initialization at KVM module probe time and instead it will be done implicitly at first SEV/SEV-ES VM launch.

Quoted 7 lines · Ashish Kalra

Quoted 6 lines · Sean Christopherson

Quoted 2 lines

This can reuse the current support (in KVM) to do SEV INIT implicitly when the first SEV VM is run: sev_guest_init() -> sev_platform_init()

I don't love the implicit behavior, but assuming hotloading firmware can't be done after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.

Quoted 7 lines

Quoted 5 lines

To summarize, if the above assumptions hold:

1. Initialize SNP when kvm-amd.ko is loaded.
2. Define CipherTextHiding and ASID params kvm-amd.ko.
3. Initialize SEV+ at first use.

Yes, the above summary is correct except for (3).

Heh, that wasn't a statement of fast, it was a suggestion for a possible implementation.

Quoted 2 lines

The initial set of patches will initialize SNP and SEV both at kvm-amd.ko module load, similar to PSP module load/probe time.

Why? If SEV+ is initialized at kvm-amd.ko load, doesn't that prevent firmware hotloading?

Quoted 3 lines

For backward compatibility, the PSP module parameter psp_init_on_probe will still be supported, i believe it is used for INIT_EX support.

Again, why? If the only use of psp_init_on_probe is to _disable_ that behavior, and we make the code never init-on-probe, then the param is unnecessary, no?

Quoted 9 lines

Quoted 4 lines

Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is built-in, correct? I.e. doesn't requires deferring kvm-amd.ko load until after firmware hotloading.

Yes, this should work, for supporting firmware hotloading, the PSP driver's psp_init_on_probe parameter will need to be set to false, which will ensure that SEV INIT is not done during SEV/SNP platform initialization at KVM module probe time and instead it will be done implicitly at first SEV/SEV-ES VM launch.

Please no. I really, really don't want gunk like this in KVM:

	init_args.probe = false;
	ret = sev_platform_init(&init_args);

That's inscrutable without a verbose comment, and all kinds of ugly. Why can't we simply separate SNP initialization from SEV+ initialization?

Quoted 1 line · Sean Christopherson

Why can't we simply separate SNP initialization from SEV+ initialization?

Yes we can do that, by default KVM module load time will only do SNP initialization, and then we will do SEV initialization if a SEV VM is being launched.

This will remove the probe parameter from init_args above, but will need to add another parameter like VM type to specify if SNP or SEV initialization is to be performed with the sev_platform_init() call.

Quoted 6 lines · Ashish Kalra

Quoted 1 line · Sean Christopherson

Why can't we simply separate SNP initialization from SEV+ initialization?

Yes we can do that, by default KVM module load time will only do SNP initialization, and then we will do SEV initialization if a SEV VM is being launched.

Quoted 3 lines

This will remove the probe parameter from init_args above, but will need to add another parameter like VM type to specify if SNP or SEV initialization is to be performed with the sev_platform_init() call.

Any reason not to simply use separate APIs? E.g. sev_snp_platform_init() and sev_platform_init()?

And if the cc_platform_has(CC_ATTR_HOST_SEV_SNP) check is moved inside of sev_snp_platform_init() (probably needs to be there anyways), then the KVM code is quite simple and will undergo minimal churn.

E.g.

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

index 5e4581ed0ef1..7e75bc55d017 100644
@@ -404,7 +404,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
                            unsigned long vm_type)
 {
        struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-       struct sev_platform_init_args init_args = {0};
        bool es_active = vm_type != KVM_X86_SEV_VM;
        u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
        int ret;

@@ -444,8 +443,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        if (ret)
                goto e_no_asid;
 
-       init_args.probe = false;
-       ret = sev_platform_init(&init_args);
+       ret = sev_platform_init();
        if (ret)
                goto e_free;
 

@@ -3053,7 +3051,7 @@ void __init sev_hardware_setup(void)
        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;
-       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
+       sev_snp_supported = sev_snp_enabled && !sev_snp_platform_init();
 
 out:
        if (boot_cpu_has(X86_FEATURE_SEV))

Quoted 11 lines · Sean Christopherson

Quoted 9 lines · Ashish Kalra

Quoted 1 line · Sean Christopherson

Why can't we simply separate SNP initialization from SEV+ initialization?

Yes we can do that, by default KVM module load time will only do SNP initialization, and then we will do SEV initialization if a SEV VM is being launched.

This will remove the probe parameter from init_args above, but will need to add another parameter like VM type to specify if SNP or SEV initialization is to be performed with the sev_platform_init() call.

Quoted 2 lines

Any reason not to simply use separate APIs? E.g. sev_snp_platform_init() and sev_platform_init()?

One reason is the need to do SEV SHUTDOWN before SNP_SHUTDOWN if any SEV VMs are active and this is taken care with the single API interface sev_platform_shutdown(), so that's why considering using a consistent API interface for both INIT and SHUTDOWN ...
- sev_platform_init()
- sev_platform_shutdown()

We can use separate APIs, but then we probably need the same for shutdown too and KVM will need to keep track of any active SEV VMs and ensure to call sev_platform_shutdown() before sev_snp_platform_shutdown() (as part of sev_hardware_unsetup()).

Thanks,
Ashish

Quoted 38 lines

And if the cc_platform_has(CC_ATTR_HOST_SEV_SNP) check is moved inside of sev_snp_platform_init() (probably needs to be there anyways), then the KVM code is quite simple and will undergo minimal churn.

E.g.

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

index 5e4581ed0ef1..7e75bc55d017 100644
@@ -404,7 +404,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
                            unsigned long vm_type)
 {
        struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-       struct sev_platform_init_args init_args = {0};
        bool es_active = vm_type != KVM_X86_SEV_VM;
        u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
        int ret;

@@ -444,8 +443,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        if (ret)
                goto e_no_asid;
 
-       init_args.probe = false;
-       ret = sev_platform_init(&init_args);
+       ret = sev_platform_init();
        if (ret)
                goto e_free;
 

@@ -3053,7 +3051,7 @@ void __init sev_hardware_setup(void)
        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;
-       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
+       sev_snp_supported = sev_snp_enabled && !sev_snp_platform_init();
 
 out:
        if (boot_cpu_has(X86_FEATURE_SEV))

Quoted 14 lines · Ashish

Quoted 11 lines · Sean Christopherson

Quoted 9 lines · Ashish Kalra

Quoted 1 line · Sean Christopherson

Why can't we simply separate SNP initialization from SEV+ initialization?

Yes we can do that, by default KVM module load time will only do SNP initialization, and then we will do SEV initialization if a SEV VM is being launched.

This will remove the probe parameter from init_args above, but will need to add another parameter like VM type to specify if SNP or SEV initialization is to be performed with the sev_platform_init() call.

Quoted 8 lines

Quoted 2 lines

Any reason not to simply use separate APIs? E.g. sev_snp_platform_init() and sev_platform_init()?

One reason is the need to do SEV SHUTDOWN before SNP_SHUTDOWN if any SEV VMs are active and this is taken care with the single API interface sev_platform_shutdown(), so that's why considering using a consistent API interface for both INIT and SHUTDOWN ...
- sev_platform_init()
- sev_platform_shutdown()

Which also assists in using the same internal interface __sev_firmware_shutdown() to be called both with sev_platform_shutdown() and the SNP panic notifier to shutdown both SEV and SNP (in that order).

Thanks,
Ashish

Quoted 5 lines

We can use separate APIs, but then we probably need the same for shutdown too and KVM will need to keep track of any active SEV VMs and ensure to call sev_platform_shutdown() before sev_snp_platform_shutdown() (as part of sev_hardware_unsetup()).

Quoted 40 lines

Quoted 39 lines

And if the cc_platform_has(CC_ATTR_HOST_SEV_SNP) check is moved inside of sev_snp_platform_init() (probably needs to be there anyways), then the KVM code is quite simple and will undergo minimal churn.

E.g.

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

index 5e4581ed0ef1..7e75bc55d017 100644
@@ -404,7 +404,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
                            unsigned long vm_type)
 {
        struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-       struct sev_platform_init_args init_args = {0};
        bool es_active = vm_type != KVM_X86_SEV_VM;
        u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
        int ret;

@@ -444,8 +443,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        if (ret)
                goto e_no_asid;
 
-       init_args.probe = false;
-       ret = sev_platform_init(&init_args);
+       ret = sev_platform_init();
        if (ret)
                goto e_free;
 

@@ -3053,7 +3051,7 @@ void __init sev_hardware_setup(void)
        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;
-       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
+       sev_snp_supported = sev_snp_enabled && !sev_snp_platform_init();
 
 out:
        if (boot_cpu_has(X86_FEATURE_SEV))

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

Ciphertext hiding separates the ASID space into SNP guest ASIDs and host ASIDs. All SNP active guests must have an ASID less than or equal to MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests (SEV and SEV-ES) must be greater than MAX_SNP_ASID.

This patch-set adds a new module parameter to the CCP driver defined as max_snp_asid which is a user configurable MAX_SNP_ASID to define the system-wide maximum SNP ASID value. If this value is not set, then the ASID space is equally divided between SEV-SNP and SEV-ES guests.

Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this new module parameter has to added to the CCP driver.

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

---
 arch/x86/kvm/svm/sev.c       | 26 ++++++++++++++----
 drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++
 include/linux/psp-sev.h      | 12 +++++++--
 3 files changed, 83 insertions(+), 7 deletions(-)

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

index 0b851ef937f2..a345b4111ad6 100644
@@ -171,7 +171,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
 	misc_cg_uncharge(type, sev->misc_cg, 1);
 }
 
-static int sev_asid_new(struct kvm_sev_info *sev)
+static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
 {
 	/*
 	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.

@@ -199,6 +199,18 @@ static int sev_asid_new(struct kvm_sev_info *sev)
 
 	mutex_lock(&sev_bitmap_lock);
 
+	/*
+	 * When CipherTextHiding is enabled, all SNP guests must have an

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

 again:
 	asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid);
 	if (asid > max_asid) {

@@ -440,7 +452,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;
 

@@ -3059,14 +3071,18 @@ void __init sev_hardware_setup(void)
 								       "unusable" :
 								       "disabled",

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

+	}
 	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
 		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
 			sev_snp_supported ? "enabled" : "disabled",
-			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+			min_sev_asid > 1 ? 1 : 0, snp_max_snp_asid ? : min_sev_asid - 1);
 
 	sev_enabled = sev_supported;
 	sev_es_enabled = sev_es_supported;

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

index 564daf748293..77900abb1b46 100644

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

+
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */
 
+/* Cipher Text Hiding Enabled */
+bool snp_cipher_text_hiding;
+EXPORT_SYMBOL(snp_cipher_text_hiding);
+
+/* MAX_SNP_ASID */
+unsigned int snp_max_snp_asid;
+EXPORT_SYMBOL(snp_max_snp_asid);
+
 static bool psp_dead;
 static int psp_timeout;
 

@@ -1064,6 +1080,38 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error)
+{
+	struct psp_device *psp = psp_master;
+	struct sev_device *sev;

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

+	}
+}
+
 static void snp_get_platform_data(void)
 {
 	struct sev_device *sev = psp_master->sev_data;

@@ -1199,6 +1247,10 @@ static int __sev_snp_init_locked(int *error)
 		}
 
 		memset(&data, 0, sizeof(data));
+
+		if (cipher_text_hiding)
+			sev_snp_enable_ciphertext_hiding(&data, error);
+
 		data.init_rmp = 1;
 		data.list_paddr_en = 1;
 		data.list_paddr = __psp_pa(snp_range_list);

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

index 6068a89839e1..2102248bd436 100644
@@ -27,6 +27,9 @@ enum sev_state {
 	SEV_STATE_MAX
 };
 
+extern bool snp_cipher_text_hiding;
+extern unsigned int snp_max_snp_asid;
+
 /**
  * SEV platform and guest management commands
  */

@@ -746,10 +749,13 @@ struct sev_data_snp_guest_request {
 struct sev_data_snp_init_ex {
 	u32 init_rmp:1;
 	u32 list_paddr_en:1;
-	u32 rsvd:30;
+	u32 rapl_dis:1;
+	u32 ciphertext_hiding_en:1;
+	u32 rsvd:28;
 	u32 rsvd1;
 	u64 list_paddr;
-	u8  rsvd2[48];
+	u16 max_snp_asid;
+	u8  rsvd2[46];
 } __packed;
 
 /**

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

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

Ciphertext hiding separates the ASID space into SNP guest ASIDs and host ASIDs. All SNP active guests must have an ASID less than or equal to MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests (SEV and SEV-ES) must be greater than MAX_SNP_ASID.

This patch-set adds a new module parameter to the CCP driver defined as max_snp_asid which is a user configurable MAX_SNP_ASID to define the system-wide maximum SNP ASID value. If this value is not set, then the ASID space is equally divided between SEV-SNP and SEV-ES guests.

Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this new module parameter has to added to the CCP driver.

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

---
 arch/x86/kvm/svm/sev.c       | 26 ++++++++++++++----
 drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++
 include/linux/psp-sev.h      | 12 +++++++--
 3 files changed, 83 insertions(+), 7 deletions(-)

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

+module_param(max_snp_asid, int, 0444);
+MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");
+
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */

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

 static bool psp_dead;
 static int psp_timeout;
 

@@ -1064,6 +1080,38 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error)
+{
+	struct psp_device *psp = psp_master;
+	struct sev_device *sev;

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

+		data->ciphertext_hiding_en = 1;
+		data->max_snp_asid = snp_max_snp_asid;
+
+		dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n");
+	}
+}