[PATCH v2 0/3] Add SEV-SNP CipherTextHiding feature support

[PATCH v2 0/3] Add SEV-SNP CipherTextHiding feature support

[Ashish Kalra]

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

Ciphertext hiding prevents host accesses from reading the ciphertext
of SNP guest private memory. Instead of reading ciphertext, the host
will see constant default values (0xff).

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

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

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

Ashish Kalra (3):
  crypto: ccp: New bit-field definitions for SNP_PLATFORM_STATUS command
  crypto: ccp: Add support for SNP_FEATURE_INFO command
  x86/sev: Add SEV-SNP CipherTextHiding support

 arch/x86/kvm/svm/sev.c       | 26 ++++++++--
 drivers/crypto/ccp/sev-dev.c | 99 ++++++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sev-dev.h |  3 ++
 include/linux/psp-sev.h      | 41 ++++++++++++++-
 include/uapi/linux/psp-sev.h | 10 +++-
 5 files changed, 171 insertions(+), 8 deletions(-)

-- 
2.34.1

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

[Ashish Kalra]

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

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

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

diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
index 832c15d9155b..dd7298b67b37 100644
--- a/include/uapi/linux/psp-sev.h
+++ b/include/uapi/linux/psp-sev.h
@@ -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 */
-- 
2.34.1

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

[Peter Gonda]

On Tue, Sep 17, 2024 at 2:16 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> Define new bit-field definitions returned by SNP_PLATFORM_STATUS command
> such as new capabilities like SNP_FEATURE_INFO command availability,
> ciphertext hiding enabled and capability.
>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  include/uapi/linux/psp-sev.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> index 832c15d9155b..dd7298b67b37 100644
> --- a/include/uapi/linux/psp-sev.h
> +++ b/include/uapi/linux/psp-sev.h
> @@ -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 */

Is this the right way for userspace to check if a command is
available? None of the other commands are detailed like this one is.

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

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

[Tom Lendacky]

On 9/17/24 15:16, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Define new bit-field definitions returned by SNP_PLATFORM_STATUS command
> such as new capabilities like SNP_FEATURE_INFO command availability,
> ciphertext hiding enabled and capability.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  include/uapi/linux/psp-sev.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> index 832c15d9155b..dd7298b67b37 100644
> --- a/include/uapi/linux/psp-sev.h
> +++ b/include/uapi/linux/psp-sev.h
> @@ -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 */

[PATCH v2 2/3] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Ashish Kalra]

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

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

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

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

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

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

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index af018afd9cd7..564daf748293 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -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;
+	struct snp_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	int error = 0, rc;
+
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return;
+
+	/*
+	 * The output buffer must be firmware page if SEV-SNP is
+	 * initialized.
+	 */
+	if (sev->snp_initialized)
+		return;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!rc && sev->snp_plat_status.feature_info) {
+		/*
+		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+		 * command to handle any alignment and page boundary check
+		 * requirements.
+		 */
+		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
+		snp_feat_info.length = sizeof(snp_feat_info);
+		snp_feat_info.ecx_in = 0;
+		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
+		if (!rc)
+			sev->feat_info = *feat_info;
+		kfree(feat_info);
+	}
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;
@@ -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);
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index 3e4e5574e88a..1c1a51e52d2b 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -57,6 +57,9 @@ struct sev_device {
 	bool cmd_buf_backup_active;
 
 	bool snp_initialized;
+
+	struct sev_user_data_snp_status snp_plat_status;
+	struct snp_feature_info feat_info;
 };
 
 int sev_dev_init(struct psp_device *psp);
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 903ddfea8585..6068a89839e1 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -107,6 +107,7 @@ enum sev_cmd {
 	SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA,
 	SEV_CMD_SNP_COMMIT		= 0x0CB,
 	SEV_CMD_SNP_VLEK_LOAD		= 0x0CD,
+	SEV_CMD_SNP_FEATURE_INFO	= 0x0CE,
 
 	SEV_CMD_MAX,
 };
@@ -812,6 +813,34 @@ struct sev_data_snp_commit {
 	u32 len;
 } __packed;
 
+/**
+ * struct sev_data_snp_feature_info - SEV_SNP_FEATURE_INFO structure
+ *
+ * @length: len of the command buffer read by the PSP
+ * @ecx_in: subfunction index
+ * @feature_info_paddr : SPA of the FEATURE_INFO structure
+ */
+struct sev_data_snp_feature_info {
+	u32 length;
+	u32 ecx_in;
+	u64 feature_info_paddr;
+} __packed;
+
+/**
+ * struct feature_info - FEATURE_INFO structure
+ *
+ * @eax: output of SNP_FEATURE_INFO command
+ * @ebx: output of SNP_FEATURE_INFO command
+ * @ecx: output of SNP_FEATURE_INFO command
+ * #edx: output of SNP_FEATURE_INFO command
+ */
+struct snp_feature_info {
+	u32 eax;
+	u32 ebx;
+	u32 ecx;
+	u32 edx;
+} __packed;
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**
-- 
2.34.1

Re: [PATCH v2 2/3] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Tom Lendacky]

On 9/17/24 15:16, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> The FEATURE_INFO command provides host and guests a programmatic means
> to learn about the supported features of the currently loaded firmware.
> FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
> Instead of using the CPUID instruction to retrieve Fn8000_0024,
> software can use FEATURE_INFO.
> 
> Host/Hypervisor would use the FEATURE_INFO command, while guests would
> actually issue the CPUID instruction.
> 
> The hypervisor can provide Fn8000_0024 values to the guest via the CPUID
> page in SNP_LAUNCH_UPDATE. As with all CPUID output recorded in that page,
> the hypervisor can filter Fn8000_0024. The firmware will examine
> Fn8000_0024 and apply its CPUID policy.
> 
> During CCP module initialization, after firmware update, the SNP
> platform status and feature information from CPUID 0x8000_0024,
> sub-function 0, are cached in the sev_device structure.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 47 ++++++++++++++++++++++++++++++++++++
>  drivers/crypto/ccp/sev-dev.h |  3 +++
>  include/linux/psp-sev.h      | 29 ++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index af018afd9cd7..564daf748293 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -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;
> +	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

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

Re: [PATCH v2 2/3] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Tom Lendacky]

On 10/2/24 16:18, Tom Lendacky wrote:
> On 9/17/24 15:16, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> The FEATURE_INFO command provides host and guests a programmatic means
>> to learn about the supported features of the currently loaded firmware.
>> FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
>> Instead of using the CPUID instruction to retrieve Fn8000_0024,
>> software can use FEATURE_INFO.
>>
>> Host/Hypervisor would use the FEATURE_INFO command, while guests would
>> actually issue the CPUID instruction.
>>
>> The hypervisor can provide Fn8000_0024 values to the guest via the CPUID
>> page in SNP_LAUNCH_UPDATE. As with all CPUID output recorded in that page,
>> the hypervisor can filter Fn8000_0024. The firmware will examine
>> Fn8000_0024 and apply its CPUID policy.
>>
>> During CCP module initialization, after firmware update, the SNP
>> platform status and feature information from CPUID 0x8000_0024,
>> sub-function 0, are cached in the sev_device structure.
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>  drivers/crypto/ccp/sev-dev.c | 47 ++++++++++++++++++++++++++++++++++++
>>  drivers/crypto/ccp/sev-dev.h |  3 +++
>>  include/linux/psp-sev.h      | 29 ++++++++++++++++++++++
>>  3 files changed, 79 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index af018afd9cd7..564daf748293 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -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;
>> +	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?

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

Thanks,
Tom

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

Re: [PATCH v2 2/3] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Kalra, Ashish]

Hello Tom,

On 10/2/2024 4:19 PM, Tom Lendacky wrote:
> On 10/2/24 16:18, Tom Lendacky wrote:
>> On 9/17/24 15:16, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>
>>> The FEATURE_INFO command provides host and guests a programmatic means
>>> to learn about the supported features of the currently loaded firmware.
>>> FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
>>> Instead of using the CPUID instruction to retrieve Fn8000_0024,
>>> software can use FEATURE_INFO.
>>>
>>> Host/Hypervisor would use the FEATURE_INFO command, while guests would
>>> actually issue the CPUID instruction.
>>>
>>> The hypervisor can provide Fn8000_0024 values to the guest via the CPUID
>>> page in SNP_LAUNCH_UPDATE. As with all CPUID output recorded in that page,
>>> the hypervisor can filter Fn8000_0024. The firmware will examine
>>> Fn8000_0024 and apply its CPUID policy.
>>>
>>> During CCP module initialization, after firmware update, the SNP
>>> platform status and feature information from CPUID 0x8000_0024,
>>> sub-function 0, are cached in the sev_device structure.
>>>
>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>>> ---
>>>  drivers/crypto/ccp/sev-dev.c | 47 ++++++++++++++++++++++++++++++++++++
>>>  drivers/crypto/ccp/sev-dev.h |  3 +++
>>>  include/linux/psp-sev.h      | 29 ++++++++++++++++++++++
>>>  3 files changed, 79 insertions(+)
>>>
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index af018afd9cd7..564daf748293 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -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;
>>> +	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?
> 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

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

Re: [PATCH v2 2/3] crypto: ccp: Add support for SNP_FEATURE_INFO command

[Tom Lendacky]

On 10/2/24 16:40, Kalra, Ashish wrote:
> Hello Tom,
> 
> On 10/2/2024 4:19 PM, Tom Lendacky wrote:
>> On 10/2/24 16:18, Tom Lendacky wrote:
>>> On 9/17/24 15:16, Ashish Kalra wrote:
>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>>
>>>> The FEATURE_INFO command provides host and guests a programmatic means
>>>> to learn about the supported features of the currently loaded firmware.
>>>> FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
>>>> Instead of using the CPUID instruction to retrieve Fn8000_0024,
>>>> software can use FEATURE_INFO.
>>>>
>>>> Host/Hypervisor would use the FEATURE_INFO command, while guests would
>>>> actually issue the CPUID instruction.
>>>>
>>>> The hypervisor can provide Fn8000_0024 values to the guest via the CPUID
>>>> page in SNP_LAUNCH_UPDATE. As with all CPUID output recorded in that page,
>>>> the hypervisor can filter Fn8000_0024. The firmware will examine
>>>> Fn8000_0024 and apply its CPUID policy.
>>>>
>>>> During CCP module initialization, after firmware update, the SNP
>>>> platform status and feature information from CPUID 0x8000_0024,
>>>> sub-function 0, are cached in the sev_device structure.
>>>>
>>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>>>> ---
>>>>  drivers/crypto/ccp/sev-dev.c | 47 ++++++++++++++++++++++++++++++++++++
>>>>  drivers/crypto/ccp/sev-dev.h |  3 +++
>>>>  include/linux/psp-sev.h      | 29 ++++++++++++++++++++++
>>>>  3 files changed, 79 insertions(+)
>>>>
>>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>>> index af018afd9cd7..564daf748293 100644
>>>> --- a/drivers/crypto/ccp/sev-dev.c
>>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>>> @@ -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;
>>>> +	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?
>> 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.

But you probably also want to get updated platform data, too. So I would
think you would want to be able to call this routine again, post SNP_INIT.

Thanks,
Tom

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

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

[Ashish Kalra]

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

Ciphertext hiding prevents host accesses from reading the ciphertext of
SNP guest private memory. Instead of reading ciphertext, the host reads
will see constant default values (0xff).

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 <ashish.kalra@amd.com>
---
 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(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 0b851ef937f2..a345b4111ad6 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -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;
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 564daf748293..77900abb1b46 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
 module_param(psp_init_on_probe, bool, 0444);
 MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
 
+static bool 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");
+
 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);
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 6068a89839e1..2102248bd436 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -27,6 +27,9 @@ enum sev_state {
 	SEV_STATE_MAX
 };
 
+extern bool snp_cipher_text_hiding;
+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

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

[Peter Gonda]

On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> Ciphertext hiding prevents host accesses from reading the ciphertext of
> SNP guest private memory. Instead of reading ciphertext, the host reads
> will see constant default values (0xff).
>
> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
> ASIDs. All SNP active guests must have an ASID less than or equal to
> MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests
> (SEV and SEV-ES) must be greater than MAX_SNP_ASID.
>
> This patch-set adds a new module parameter to the CCP driver defined as
> 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 <ashish.kalra@amd.com>
> ---
>  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(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0b851ef937f2..a345b4111ad6 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -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;
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 564daf748293..77900abb1b46 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
>  module_param(psp_init_on_probe, bool, 0444);
>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>
> +static bool 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().

> +
>  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);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 6068a89839e1..2102248bd436 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -27,6 +27,9 @@ enum sev_state {
>         SEV_STATE_MAX
>  };
>
> +extern bool snp_cipher_text_hiding;
> +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
>
>

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

[Kalra, Ashish]

Hello Peter,

On 10/2/2024 9:58 AM, Peter Gonda wrote:
> On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> Ciphertext hiding prevents host accesses from reading the ciphertext of
>> SNP guest private memory. Instead of reading ciphertext, the host reads
>> will see constant default values (0xff).
>>
>> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
>> ASIDs. All SNP active guests must have an ASID less than or equal to
>> MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests
>> (SEV and SEV-ES) must be greater than MAX_SNP_ASID.
>>
>> This patch-set adds a new module parameter to the CCP driver defined as
>> 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 <ashish.kalra@amd.com>
>> ---
>>  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(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 0b851ef937f2..a345b4111ad6 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -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;
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 564daf748293..77900abb1b46 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
>>  module_param(psp_init_on_probe, bool, 0444);
>>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>>
>> +static bool 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.
> 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.

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

Thanks, Ashish

>> +
>>  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);
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 6068a89839e1..2102248bd436 100644
>> --- a/include/linux/psp-sev.h
>> +++ b/include/linux/psp-sev.h
>> @@ -27,6 +27,9 @@ enum sev_state {
>>         SEV_STATE_MAX
>>  };
>>
>> +extern bool snp_cipher_text_hiding;
>> +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
>>
>>

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

[Peter Gonda]

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

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

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

[Ashish Kalra]

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

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

Sure, i will do that.

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

I tested with 6.12-rc1 and i don't have any issues with SNP init and running SNP 
VMs on that (with and without CipherTextHiding enabled), but i am getting a lot of 
stack dumps especially during host boot with apparmor, surely something looks
to be broken on apparmor on 6.12-rc1: 

[   33.180836] BUG: kernel NULL pointer dereference, address: 000000000000001c
[   33.180842] #PF: supervisor read access in kernel mode
[   33.180843] #PF: error_code(0x0000) - not-present page
[   33.180846] PGD 16bc1b067 P4D 0 
[   33.180849] Oops: Oops: 0000 [#4] SMP NOPTI
[   33.180853] CPU: 155 UID: 0 PID: 2521 Comm: apparmor_parser Tainted: G      D W          6.12.0-rc1-next-20241003-snp-host-f2a41ff576cc-dirty #19
[   33.632606] RIP: 0010:krealloc_noprof+0x8f/0x300
[   33.632608] Code: 8b 50 08 f6 c2 01 0f 85 14 02 00 00 0f 1f 44 00 00 80 78 33 f5 0f 85 0e 02 00 00 48 85 c0 0f 84 05 02 00 00 48 8b 48 08 66 90 <48> 63 59 1c 41 89 df 4d 39 fd 0f 87 8c 00 00 00 0f 1f 44 00 00 48
[   33.632610] RSP: 0018:ff2e31fe0ad3f848 EFLAGS: 00010202
[   33.632611] RAX: ff9e19414443ec00 RBX: 0000000000000001 RCX: 0000000000000000
[   33.632613] RDX: 0000000000000000 RSI: 0000000000002beb RDI: ff2d8c4410fb0000
[   33.632614] RBP: ff2e31fe0ad3f878 R08: 0000000000002be4 R09: 0000000000000000
[   33.632615] R10: 00000000000093cb R11: ff2d8c4410fb2beb R12: ff2d8c4410fb0000
[   33.632616] R13: 0000000000002beb R14: 0000000000000cc0 R15: ff2d8c446d000000
[   33.632618] FS:  00007ff504a05740(0000) GS:ff2d8c4b2c500000(0000) knlGS:0000000000000000
[   33.632619] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   33.632620] CR2: 000000000000001c CR3: 0000000157f2e001 CR4: 0000000000771ef0
[   33.632622] PKRU: 55555554
[   33.632623] note: apparmor_parser[2522] exited with irqs disabled
[   33.977961] Tainted: [D]=DIE, [W]=WARN
[   33.990019] Hardware name: AMD Corporation PURICO/PURICO, BIOS TPUT0090F 06/05/2024
[   34.006754] RIP: 0010:krealloc_noprof+0x8f/0x300
[   34.020151] Code: 8b 50 08 f6 c2 01 0f 85 14 02 00 00 0f 1f 44 00 00 80 78 33 f5 0f 85 0e 02 00 00 48 85 c0 0f 84 05 02 00 00 48 8b 48 08 66 90 <48> 63 59 1c 41 89 df 4d 39 fd 0f 87 8c 00 00 00 0f 1f 44 00 00 48
[   34.058484] RSP: 0018:ff2e31fe0ad57928 EFLAGS: 00010202
[   34.073095] RAX: ff9e194145b4c400 RBX: 0000000000000001 RCX: 0000000000000000
[   34.089957] RDX: 0000000000000000 RSI: 00000000000057bf RDI: ff2d8c446d310000
[   34.106827] RBP: ff2e31fe0ad57958 R08: 00000000000057b8 R09: 0000000000000000
[   34.123733] R10: 000000000000dac1 R11: ff2d8c446d3157bf R12: ff2d8c446d310000
[   34.140668] R13: 00000000000057bf R14: 0000000000000cc0 R15: ff2d8c446d400000
[   34.157572] FS:  00007ff504a05740(0000) GS:ff2d8c4b2b380000(0000) knlGS:0000000000000000
[   34.175513] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   34.190675] CR2: 000000000000001c CR3: 0000000157f2a004 CR4: 0000000000771ef0
[   34.207373] PKRU: 55555554
[   34.218980] Call Trace:
[   34.230226]  <TASK>
[   34.241043]  ? show_regs+0x6d/0x80
[   34.253389]  ? __die+0x29/0x70
[   34.265311]  ? page_fault_oops+0x15c/0x550
[   34.278341]  ? do_user_addr_fault+0x45e/0x7b0
[   34.291477]  ? ZSTD_compressEnd_public+0x2c/0x170
[   34.304780]  ? exc_page_fault+0x7c/0x170
[   34.316962]  ? asm_exc_page_fault+0x2b/0x30
[   34.329194]  ? krealloc_noprof+0x8f/0x300
[   34.341001]  ? zstd_compress_cctx+0x87/0xa0
[   34.353005]  aa_unpack+0x688/0x700
[   34.364035]  aa_replace_profiles+0x9e/0x1170
[   34.375977]  policy_update+0xd9/0x170
[   34.387225]  profile_replace+0xb0/0x130
[   34.398644]  vfs_write+0xf8/0x3e0
[   34.409463]  ? __x64_sys_openat+0x59/0xa0
[   34.420909]  ksys_write+0x6b/0xf0
[   34.431356]  __x64_sys_write+0x1d/0x30
[   34.442244]  x64_sys_call+0x1685/0x20d0
[   34.453055]  do_syscall_64+0x6f/0x110
[   34.463491]  ? ksys_read+0x6b/0xf0
[   34.473492]  ? syscall_exit_to_user_mode+0x57/0x1b0
[   34.485139]  ? do_syscall_64+0x7b/0x110
[   34.495611]  ? generic_file_read_iter+0xbf/0x110
[   34.506980]  ? apparmor_file_permission+0x6f/0x170
[   34.518530]  ? ext4_file_read_iter+0x5f/0x1e0
[   34.529610]  ? vfs_read+0x25c/0x340
[   34.539607]  ? ksys_read+0x6b/0xf0
[   34.549394]  ? syscall_exit_to_user_mode+0x57/0x1b0
[   34.560829]  ? do_syscall_64+0x7b/0x110
[   34.571009]  ? irqentry_exit_to_user_mode+0x33/0x170
[   34.582461]  ? irqentry_exit+0x21/0x40
[   34.592443]  ? exc_page_fault+0x8d/0x170
[   34.602507]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   34.613786] RIP: 0033:0x7ff504714887
[   34.623229] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
[   34.655533] RSP: 002b:00007ffcb6fbc758 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   34.669681] RAX: ffffffffffffffda RBX: 000055f36c77bdc0 RCX: 00007ff504714887
[   34.683405] RDX: 000000000000dac1 RSI: 000055f36c7a1680 RDI: 0000000000000007
[   34.697133] RBP: 000000000000dac1 R08: 0000000000000000 R09: 000055f36c7a1680
[   34.710815] R10: 0000000000000000 R11: 0000000000000246 R12: 000055f36c7a1680
[   34.724467] R13: 000000000000dac1 R14: 000055f3654bcc5b R15: 0000000000000007
[   34.738032]  </TASK>
[   34.745917] Modules linked in: nls_iso8859_1 wmi_bmof rapl input_leds joydev ccp(+) k10temp wmi acpi_ipmi ipmi_si ipmi_devintf ipmi_msghandler mac_hid sch_fq_codel dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua msr efi_pstore drm autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq raid1 raid0 crct10dif_pclmul ahci crc32_pclmul tg3 ghash_clmulni_intel libahci i2c_piix4 i2c_smbus hid_generic usbhid hid aesni_intel crypto_simd cryptd
[   34.819993] CR2: 000000000000001c
[   34.830269] ---[ end trace 0000000000000000 ]---

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

[Sean Christopherson]

On Wed, Oct 02, 2024, Ashish Kalra wrote:
> Hello Peter,
> 
> On 10/2/2024 9:58 AM, Peter Gonda wrote:
> > On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> >> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> >> index 564daf748293..77900abb1b46 100644
> >> --- a/drivers/crypto/ccp/sev-dev.c
> >> +++ b/drivers/crypto/ccp/sev-dev.c
> >> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
> >>  module_param(psp_init_on_probe, bool, 0444);
> >>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
> >>
> >> +static bool 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.
> > 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.
> 
> >
> > 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.

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. 

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

[Kalra, Ashish]

Hello Sean,

On 10/11/2024 11:04 AM, Sean Christopherson wrote:
> On Wed, Oct 02, 2024, Ashish Kalra wrote:
>> Hello Peter,
>>
>> On 10/2/2024 9:58 AM, Peter Gonda wrote:
>>> On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>>> index 564daf748293..77900abb1b46 100644
>>>> --- a/drivers/crypto/ccp/sev-dev.c
>>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>>> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
>>>>  module_param(psp_init_on_probe, bool, 0444);
>>>>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>>>>
>>>> +static bool 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.
>>> 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.
>>
>>>
>>> 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.
> 
> 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() instead of using
external variables in PSP driver, which KVM can call in sev_hardware_setup() to
retrieve MAX_SNP_ASID and also overriding max_asid (when CTH feature is enabled) in sev_asid_new(). 

Thanks,
Ashish

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

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

[Sean Christopherson]

On Tue, Nov 19, 2024, Ashish Kalra wrote:
> On 10/11/2024 11:04 AM, Sean Christopherson wrote:
> > On Wed, Oct 02, 2024, Ashish Kalra wrote:
> >> 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. 

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.

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

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

What about adding a cc_attr_flags entry?

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

[Kalra, Ashish]

On 11/20/2024 3:53 PM, Sean Christopherson wrote:
> On Tue, Nov 19, 2024, Ashish Kalra wrote:
>> On 10/11/2024 11:04 AM, Sean Christopherson wrote:
>>> On Wed, Oct 02, 2024, Ashish Kalra wrote:
>>>> 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. 
> 
> 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.

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

Thanks,
Ashish

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

[Kalra, Ashish]

On 11/20/2024 5:43 PM, Kalra, Ashish wrote:
> 
> On 11/20/2024 3:53 PM, Sean Christopherson wrote:
>> On Tue, Nov 19, 2024, Ashish Kalra wrote:
>>> On 10/11/2024 11:04 AM, Sean Christopherson wrote:
>>>> On Wed, Oct 02, 2024, Ashish Kalra wrote:
>>>>> 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. 
>>
>> 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.
> 
>>
>>> 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.
>>
>>> 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()
>>
>> 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.

Thanks,
Ashish

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

[Sean Christopherson]

On Thu, Nov 21, 2024, Ashish Kalra wrote:
> On 11/20/2024 5:43 PM, Kalra, Ashish wrote:
> > 
> > On 11/20/2024 3:53 PM, Sean Christopherson wrote:
> >> On Tue, Nov 19, 2024, Ashish Kalra wrote:
> >>> On 10/11/2024 11:04 AM, Sean Christopherson wrote:
> >>>> On Wed, Oct 02, 2024, Ashish Kalra wrote:
> >>>>> 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. 
> >>
> >> 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.
> > 
> >>
> >>> 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.
> >>
> >>> 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()
> >>
> >> 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.

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

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index af018afd9cd7..563cc235b095 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -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);
 }
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 903ddfea8585..def40f7ea01d 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -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
-- 

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

[Tom Lendacky]

On 11/21/24 10:56, Sean Christopherson wrote:
> On Thu, Nov 21, 2024, Ashish Kalra wrote:
>> On 11/20/2024 5:43 PM, Kalra, Ashish wrote:
>>>
>>> On 11/20/2024 3:53 PM, Sean Christopherson wrote:
>>>> On Tue, Nov 19, 2024, Ashish Kalra wrote:
>>>>> On 10/11/2024 11:04 AM, Sean Christopherson wrote:
>>>>>> On Wed, Oct 02, 2024, Ashish Kalra wrote:
>>>>>>> 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. 
>>>>
>>>> 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.
>>>
>>>>
>>>>> 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.
>>>>
>>>>> 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()
>>>>
>>>> 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.
> 
>> 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+.

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

Or...

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

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

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

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

Thanks,
Tom

> 
> 	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(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index af018afd9cd7..563cc235b095 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -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);
>  }
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 903ddfea8585..def40f7ea01d 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -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

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

[Sean Christopherson]

On Thu, Nov 21, 2024, Tom Lendacky wrote:
> On 11/21/24 10:56, Sean Christopherson wrote:
> > 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.
> > 
> > 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.

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

kvm-amd.ko load time.

> > 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.
> 
> True, we'll look at doing a fix for that.
> 
> > 
> > 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.

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

[Kalra, Ashish]

On 11/21/2024 11:42 AM, Sean Christopherson wrote:
> On Thu, Nov 21, 2024, Tom Lendacky wrote:
>> On 11/21/24 10:56, Sean Christopherson wrote:
>>> 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.
>>>
>>> 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.
> 
>> Or...
>>
>>>
>>> 	/*
>>> 	 * 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.
> 
> 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() 

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

Yes, i do think this can be an issue as those ioctl's may be in use by userspace tools like
sevtool, etc., and require SEV implicit initialization and we will have to preserve this 
behavior if indicated by a PSP module param.

Thanks,
Ashish

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

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

[Sean Christopherson]

On Thu, Nov 21, 2024, Ashish Kalra wrote:
> On 11/21/2024 11:42 AM, Sean Christopherson wrote:
> > On Thu, Nov 21, 2024, Tom Lendacky wrote:
> >> On 11/21/24 10:56, Sean Christopherson wrote:
> >>> 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.
> >>>
> >>> 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.
> > 
> >> Or...
> >>
> >>>
> >>> 	/*
> >>> 	 * 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.
> > 
> > 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 ?

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.

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

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

[Kalra, Ashish]

Hello Sean, 

Please see my responses below:

On 12/6/2024 4:30 PM, Sean Christopherson wrote:
> On Thu, Nov 21, 2024, Ashish Kalra wrote:
>> On 11/21/2024 11:42 AM, Sean Christopherson wrote:
>>> On Thu, Nov 21, 2024, Tom Lendacky wrote:
>>>> On 11/21/24 10:56, Sean Christopherson wrote:
>>>>> 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.
>>>>>
>>>>> 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.
>>>
>>>> Or...
>>>>
>>>>>
>>>>> 	/*
>>>>> 	 * 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.
>>>
>>> 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 ?
> 
> 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.

Here is a summary:

1). SNP_INIT(_EX) cannot be done if SEV INIT has been done, the above comment mentions the same,  
in other words, if SEV INIT has completed successfully then any subsequent SNP_INIT(_EX) will fail.

2). Also if SNP is enabled (SNPEn) system-wide, which is done during early kernel boot, then 
SNP_INIT_EX has to be done before SEV INIT, otherwise SEV INIT will fail. 

SNP is enabled system-wide if SNP support and RMP table support is enabled in BIOS and 
additionally RMP table sanity checks are successful, RMP table is mapped and SNP support is
enabled on the IOMMU.

This also means that if we only want to launch legacy VMs (SEV/SEV-ES), but SNP has been enabled
at early kernel boot then we still need to do SNP_INIT(_EX) before SEV INIT. 

Considering all these, SNP_INIT(_EX) is always attempted before SEV INIT as above.

If SNP is not enabled system-wide (CC_ATTR_HOST_SEV_SNP is not set), then SNP_INIT(_EX) will
be skipped.

If we specify KVM module parameter sev_snp=false, but if SNPEn is set system-wide, we
will still do SNP_INIT(_EX) before SEV INIT to ensure that SEV INIT is successful. 
But do note, in this configuration launching any SNP VMs will fail.

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

Yes, that is true. As per SNP Firmware API specs, SNP DOWNLOAD_FIRMWARE_EX adds support 
for provisional updates and for updates while SNP firmware is in the INIT state.

The SNP firmware may be in any state. SEV must be in the UNINIT state.

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

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.

I have the base set of patches ready which remove SEV/SNP platform initialization from
PSP module probe time and instead move it to KVM module load time (sev_hardware_setup)
and similarly for platform shutdown.

I will post this patch-set early next week.

On top of this base patch-set we will push the SNP CipherTextHiding support and SEV
firmware hotloading support. The SEV firmware hotloading support will add support
for (3) above.

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

Hopefully, this summarizes all the possible configurations for SEV/SNP platform
initialization.

Thanks,
Ashish

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

[Sean Christopherson]

On Fri, Dec 06, 2024, Ashish Kalra wrote:
> On 12/6/2024 4:30 PM, Sean Christopherson wrote:
> >> 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.
> 
> 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.

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

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

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

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

[Kalra, Ashish]

Hello Sean,

On 12/9/2024 7:30 PM, Sean Christopherson wrote:
> On Fri, Dec 06, 2024, Ashish Kalra wrote:
>> On 12/6/2024 4:30 PM, Sean Christopherson wrote:
>>>> 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.
>>
>> 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.
> 
>> 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?

Yes it does, i was thinking of fixing it as part of a series on top of these patches
to support SEV firmware hotloading.

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

Yes, it makes sense to remove this param.

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

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.

Will work on re-posting my series with SNP initialization separated from SEV
initialization.

Thanks,
Ashish

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

[Sean Christopherson]

On Tue, Dec 10, 2024, Ashish Kalra wrote:
> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
> > 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.

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.

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5e4581ed0ef1..7e75bc55d017 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -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))

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

[Kalra, Ashish]

On 12/10/2024 4:57 PM, Sean Christopherson wrote:
> On Tue, Dec 10, 2024, Ashish Kalra wrote:
>> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
>>> 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.
> 
> 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

> 
> 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.
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5e4581ed0ef1..7e75bc55d017 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -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))

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

[Kalra, Ashish]

On 12/10/2024 6:48 PM, Kalra, Ashish wrote:
> 
> 
> On 12/10/2024 4:57 PM, Sean Christopherson wrote:
>> On Tue, Dec 10, 2024, Ashish Kalra wrote:
>>> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
>>>> 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.
>>
>> 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

> 
> 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
> 
>>
>> 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.
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 5e4581ed0ef1..7e75bc55d017 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -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))
> 

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

[Kalra, Ashish]

On 12/10/2024 7:01 PM, Kalra, Ashish wrote:
> 
> 
> On 12/10/2024 6:48 PM, Kalra, Ashish wrote:
>>
>>
>> On 12/10/2024 4:57 PM, Sean Christopherson wrote:
>>> On Tue, Dec 10, 2024, Ashish Kalra wrote:
>>>> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
>>>>> 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.
>>>
>>> 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
> 
>>
>> 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()).
>>

Worked on separating SEV and SNP initialization and got it working, but it needs 
additional fix in qemu to remove the check for SEV-ES being already initialized (i.e, SEV
INIT being already done) as below to ensure that SEV INIT is done on demand when
SEV/SEV-ES guests are being launched: 

diff --git a/target/i386/sev.c b/target/i386/sev.c
index a0d271f898..bb541f9d41 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1503,15 +1503,6 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         }
     }

-    if (sev_es_enabled() && !sev_snp_enabled()) {
-        if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) {
-            error_setg(errp, "%s: guest policy requires SEV-ES, but "
-                         "host SEV-ES support unavailable",
-                         __func__);
-            return -1;
-        }
-    }
-
     trace_kvm_sev_init();
     switch (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common))) {
     case KVM_X86_DEFAULT_VM:
>>
>>>
>>> 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.
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 5e4581ed0ef1..7e75bc55d017 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -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))
>>
> 

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

[Tom Lendacky]

On 9/17/24 15:16, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Ciphertext hiding prevents host accesses from reading the ciphertext of
> SNP guest private memory. Instead of reading ciphertext, the host reads
> will see constant default values (0xff).
> 
> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
> ASIDs. All SNP active guests must have an ASID less than or equal to
> MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests
> (SEV and SEV-ES) must be greater than MAX_SNP_ASID.
> 
> This patch-set adds a new module parameter to the CCP driver defined as
> 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 <ashish.kalra@amd.com>
> ---
>  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(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0b851ef937f2..a345b4111ad6 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -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;
> +	}

Add a blank line here.

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

I think this might be easier to read if you align everything based on
the conditions, e.g.:

		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
			sev_es_supported ? "enabled" : "disabled",
			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;
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 564daf748293..77900abb1b46 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
>  module_param(psp_init_on_probe, bool, 0444);
>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>  
> +static bool 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");

I agree with Peter that this should be false by default to maintain
backward compatibility.

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

MODULE_PARM_DESC(max_snp_asid, "  the maximum SNP ASID available when Cipher Text Hiding is enabled");

> +
>  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) {

Remove the indentation here by just doing:

	if (!(sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED))
		return;

	if (!sev->snp_plat_status.ciphertext_hiding_cap)
		return;

> +		/* Retrieve SEV CPUID information */

Remove this comment and ...

> +		edx = cpuid_edx(0x8000001f);
> +		/* Do sanity checks on user-defined MAX_SNP_ASID */

... move this comment above the cpuid_edx() call.

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

s/1/true/

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

"SEV-SNP cipher text hiding enabled"

Thanks,
Tom

> +	}
> +}
> +
>  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);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 6068a89839e1..2102248bd436 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -27,6 +27,9 @@ enum sev_state {
>  	SEV_STATE_MAX
>  };
>  
> +extern bool snp_cipher_text_hiding;
> +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
>  
>  /**

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

[Tom Lendacky]

On 9/17/24 15:16, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Ciphertext hiding prevents host accesses from reading the ciphertext of
> SNP guest private memory. Instead of reading ciphertext, the host reads
> will see constant default values (0xff).
> 
> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
> ASIDs. All SNP active guests must have an ASID less than or equal to
> MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests
> (SEV and SEV-ES) must be greater than MAX_SNP_ASID.
> 
> This patch-set adds a new module parameter to the CCP driver defined as
> 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 <ashish.kalra@amd.com>
> ---
>  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(-)

I missed this on initial review. This change goes across multiple
maintainers trees, so you should split this patch to do the CCP updates
first and then the KVM updates.

Thanks,
Tom

> 

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

[Sean Christopherson]

On Tue, Sep 17, 2024, Ashish Kalra wrote:
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 564daf748293..77900abb1b46 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
>  module_param(psp_init_on_probe, bool, 0444);
>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>  
> +static bool 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;

Why is this a signed int?  '0' is used as the magic "no override" value, so there's
no reason to allow a negative value.

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

There is zero reason to have multiple variables.  The module param varaibles
should be the single source of true.

I'm also not entirely sure exporting individual variables is the right interface,
which is another reason why I want to see the entire "refactoring" in one series.

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

snp_cipher_text_hiding should be set to %false if CipherTextHiding is unsupported.
I.e. the module params need to reflect reality.

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

s/1/true

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