[PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Ashish Kalra]

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

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

The SEV ASID space is split into SEV and SEV-ES/SNP ASID ranges.
Enabling ciphertext hiding further splits the SEV-ES/SEV-SNP ASID space
into separate ASID ranges for SEV-ES and SEV-SNP guests.

Add new module parameter to the KVM module to enable ciphertext hiding
support and a user configurable system-wide maximum SNP ASID value. If
the module parameter value is "max" then the complete SEV-ES/SEV-SNP
space is allocated to SEV-SNP guests.

v7:
- Fix comments.
- Move the check for module parameter ciphertext_hiding_asids inside
check_and_enable_sev_snp_ciphertext_hiding(), this keeps all the logic
related to the parameter in a single function.

v6:
- Fix module parameter ciphertext_hiding_asids=0 case.
- Coalesce multiple cases of handling invalid module parameter
ciphertext_hiding_asids into a single branch/label.
- Fix commit logs.
- Fix Documentation.

v5:
- Add pre-patch to cache SEV platform status and use this cached
information to set api_major/api_minor/build.
- Since the SEV platform status and SNP platform status differ, 
remove the state field from sev_device structure and instead track
SEV platform state from cached SEV platform status.
- If SNP is enabled then cached SNP platform status is used for 
api_major/api_minor/build.
- Fix using sev_do_cmd() instead of __sev_do_cmd_locked().
- Fix commit logs.
- Fix kernel-parameters documentation. 
- Modify KVM module parameter to enable CipherTextHiding to support
"max" option to allow complete SEV-ES+ ASID space to be allocated
to SEV-SNP guests.
- Do not enable ciphertext hiding if module parameter to specify
maximum SNP ASID is invalid.

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

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

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

Ashish Kalra (7):
  crypto: ccp - New bit-field definitions for SNP_PLATFORM_STATUS
    command
  crypto: ccp - Cache SEV platform status and platform state
  crypto: ccp - Add support for SNP_FEATURE_INFO command
  crypto: ccp - Introduce new API interface to indicate SEV-SNP
    Ciphertext hiding feature
  crypto: ccp - Add support to enable CipherTextHiding on SNP_INIT_EX
  KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables
  KVM: SEV: Add SEV-SNP CipherTextHiding support

 .../admin-guide/kernel-parameters.txt         |  18 +++
 arch/x86/kvm/svm/sev.c                        |  96 +++++++++++--
 drivers/crypto/ccp/sev-dev.c                  | 127 ++++++++++++++++--
 drivers/crypto/ccp/sev-dev.h                  |   6 +-
 include/linux/psp-sev.h                       |  44 +++++-
 include/uapi/linux/psp-sev.h                  |  10 +-
 6 files changed, 274 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH v7 1/7] 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.

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

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

[PATCH v7 2/7] crypto: ccp - Cache SEV platform status and platform state

[Ashish Kalra]

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

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

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

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

Suggested-by: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 22 ++++++++++++----------
 drivers/crypto/ccp/sev-dev.h |  3 ++-
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index e058ba027792..528013be1c0a 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1286,7 +1286,7 @@ static int __sev_platform_init_locked(int *error)
 
 	sev = psp_master->sev_data;
 
-	if (sev->state == SEV_STATE_INIT)
+	if (sev->sev_plat_status.state == SEV_STATE_INIT)
 		return 0;
 
 	__sev_platform_init_handle_tmr(sev);
@@ -1318,7 +1318,7 @@ static int __sev_platform_init_locked(int *error)
 		return rc;
 	}
 
-	sev->state = SEV_STATE_INIT;
+	sev->sev_plat_status.state = SEV_STATE_INIT;
 
 	/* Prepare for first SEV guest launch after INIT */
 	wbinvd_on_all_cpus();
@@ -1347,7 +1347,7 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 
 	sev = psp_master->sev_data;
 
-	if (sev->state == SEV_STATE_INIT)
+	if (sev->sev_plat_status.state == SEV_STATE_INIT)
 		return 0;
 
 	rc = __sev_snp_init_locked(&args->error);
@@ -1384,7 +1384,7 @@ static int __sev_platform_shutdown_locked(int *error)
 
 	sev = psp->sev_data;
 
-	if (sev->state == SEV_STATE_UNINIT)
+	if (sev->sev_plat_status.state == SEV_STATE_UNINIT)
 		return 0;
 
 	ret = __sev_do_cmd_locked(SEV_CMD_SHUTDOWN, NULL, error);
@@ -1394,7 +1394,7 @@ static int __sev_platform_shutdown_locked(int *error)
 		return ret;
 	}
 
-	sev->state = SEV_STATE_UNINIT;
+	sev->sev_plat_status.state = SEV_STATE_UNINIT;
 	dev_dbg(sev->dev, "SEV firmware shutdown\n");
 
 	return ret;
@@ -1502,7 +1502,7 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
 	if (!writable)
 		return -EPERM;
 
-	if (sev->state == SEV_STATE_UNINIT) {
+	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
 		rc = sev_move_to_init_state(argp, &shutdown_required);
 		if (rc)
 			return rc;
@@ -1551,7 +1551,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	data.len = input.length;
 
 cmd:
-	if (sev->state == SEV_STATE_UNINIT) {
+	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
 		ret = sev_move_to_init_state(argp, &shutdown_required);
 		if (ret)
 			goto e_free_blob;
@@ -1606,10 +1606,12 @@ static int sev_get_api_version(void)
 		return 1;
 	}
 
+	/* Cache SEV platform status */
+	sev->sev_plat_status = status;
+
 	sev->api_major = status.api_major;
 	sev->api_minor = status.api_minor;
 	sev->build = status.build;
-	sev->state = status.state;
 
 	return 0;
 }
@@ -1837,7 +1839,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	data.oca_cert_len = input.oca_cert_len;
 
 	/* If platform is not in INIT state then transition it to INIT */
-	if (sev->state != SEV_STATE_INIT) {
+	if (sev->sev_plat_status.state != SEV_STATE_INIT) {
 		ret = sev_move_to_init_state(argp, &shutdown_required);
 		if (ret)
 			goto e_free_oca;
@@ -2008,7 +2010,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 
 cmd:
 	/* If platform is not in INIT state then transition it to INIT. */
-	if (sev->state != SEV_STATE_INIT) {
+	if (sev->sev_plat_status.state != SEV_STATE_INIT) {
 		if (!writable) {
 			ret = -EPERM;
 			goto e_free_cert;
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index 3e4e5574e88a..24dd8ff8afaa 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -42,7 +42,6 @@ struct sev_device {
 
 	struct sev_vdata *vdata;
 
-	int state;
 	unsigned int int_rcvd;
 	wait_queue_head_t int_queue;
 	struct sev_misc_dev *misc;
@@ -57,6 +56,8 @@ struct sev_device {
 	bool cmd_buf_backup_active;
 
 	bool snp_initialized;
+
+	struct sev_user_data_status sev_plat_status;
 };
 
 int sev_dev_init(struct psp_device *psp);
-- 
2.34.1

[PATCH v7 3/7] crypto: ccp - Add support for SNP_FEATURE_INFO command

[Ashish Kalra]

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

The FEATURE_INFO command provides hypervisors with a programmatic means
to learn about the supported features of the currently loaded firmware.
This command mimics the CPUID instruction relative to sub-leaf input and
the four unsigned integer output values. To obtain information
regarding the features present in the currently loaded SEV firmware,
use the SNP_FEATURE_INFO command.

Cache the SNP platform status and feature information from CPUID
0x8000_0024 in the sev_device structure. If SNP is enabled, utilize
this cached SNP platform status for the API major, minor and build
version.

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

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 528013be1c0a..a3941254d61f 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -233,6 +233,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct sev_data_snp_feature_info);
 	default:				return 0;
 	}
 
@@ -1073,6 +1074,67 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrq(MSR_VM_HSAVE_PA, 0);
 }
 
+static int snp_get_platform_data(struct sev_device *sev, int *error)
+{
+	struct sev_data_snp_feature_info snp_feat_info;
+	struct snp_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	struct page *page;
+	int rc;
+
+	/*
+	 * This function is expected to be called before SNP is
+	 * initialized.
+	 */
+	if (sev->snp_initialized)
+		return -EINVAL;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = sev_do_cmd(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
+	if (rc) {
+		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
+			rc, *error);
+		return rc;
+	}
+
+	sev->api_major = sev->snp_plat_status.api_major;
+	sev->api_minor = sev->snp_plat_status.api_minor;
+	sev->build = sev->snp_plat_status.build_id;
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!sev->snp_plat_status.feature_info)
+		return 0;
+
+	/*
+	 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+	 * command to ensure structure is 8-byte aligned, and does not
+	 * cross a page boundary.
+	 */
+	page = alloc_page(GFP_KERNEL);
+	if (!page)
+		return -ENOMEM;
+
+	feat_info = page_address(page);
+	snp_feat_info.length = sizeof(snp_feat_info);
+	snp_feat_info.ecx_in = 0;
+	snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+	rc = sev_do_cmd(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
+	if (!rc)
+		sev->snp_feat_info_0 = *feat_info;
+	else
+		dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
+			rc, *error);
+
+	__free_page(page);
+
+	return rc;
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;
@@ -1599,6 +1661,16 @@ static int sev_get_api_version(void)
 	struct sev_user_data_status status;
 	int error = 0, ret;
 
+	/*
+	 * Cache SNP platform status and SNP feature information
+	 * if SNP is available.
+	 */
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
+		ret = snp_get_platform_data(sev, &error);
+		if (ret)
+			return 1;
+	}
+
 	ret = sev_platform_status(&status, &error);
 	if (ret) {
 		dev_err(sev->dev,
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index 24dd8ff8afaa..5aed2595c9ae 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -58,6 +58,9 @@ struct sev_device {
 	bool snp_initialized;
 
 	struct sev_user_data_status sev_plat_status;
+
+	struct sev_user_data_snp_status snp_plat_status;
+	struct snp_feature_info snp_feat_info_0;
 };
 
 int sev_dev_init(struct psp_device *psp);
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 0f5f94137f6d..5fb6ae0f51cc 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,
 };
@@ -814,6 +815,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 : System Physical Address of the FEATURE_INFO structure
+ */
+struct sev_data_snp_feature_info {
+	u32 length;
+	u32 ecx_in;
+	u64 feature_info_paddr;
+} __packed;
+
+/**
+ * struct feature_info - FEATURE_INFO structure
+ *
+ * @eax: output of SNP_FEATURE_INFO command
+ * @ebx: output of SNP_FEATURE_INFO command
+ * @ecx: output of SNP_FEATURE_INFO command
+ * #edx: output of SNP_FEATURE_INFO command
+ */
+struct snp_feature_info {
+	u32 eax;
+	u32 ebx;
+	u32 ecx;
+	u32 edx;
+} __packed;
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**
-- 
2.34.1

[PATCH v7 4/7] crypto: ccp - Introduce new API interface to indicate SEV-SNP Ciphertext hiding feature

[Ashish Kalra]

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

Implement an API that checks the overall feature support for SEV-SNP
ciphertext hiding.

This API verifies both the support of the SEV firmware for the feature
and its enablement in the platform's BIOS.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 21 +++++++++++++++++++++
 include/linux/psp-sev.h      |  5 +++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index a3941254d61f..58c9e040e9ac 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1074,6 +1074,27 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrq(MSR_VM_HSAVE_PA, 0);
 }
 
+bool sev_is_snp_ciphertext_hiding_supported(void)
+{
+	struct psp_device *psp = psp_master;
+	struct sev_device *sev;
+
+	if (!psp || !psp->sev_data)
+		return false;
+
+	sev = psp->sev_data;
+
+	/*
+	 * Feature information indicates if CipherTextHiding feature is
+	 * supported by the SEV firmware and additionally platform status
+	 * indicates if CipherTextHiding feature is enabled in the
+	 * Platform BIOS.
+	 */
+	return ((sev->snp_feat_info_0.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
+		 sev->snp_plat_status.ciphertext_hiding_cap);
+}
+EXPORT_SYMBOL_GPL(sev_is_snp_ciphertext_hiding_supported);
+
 static int snp_get_platform_data(struct sev_device *sev, int *error)
 {
 	struct sev_data_snp_feature_info snp_feat_info;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 5fb6ae0f51cc..d83185b4268b 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -843,6 +843,8 @@ struct snp_feature_info {
 	u32 edx;
 } __packed;
 
+#define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**
@@ -986,6 +988,7 @@ void *psp_copy_user_blob(u64 uaddr, u32 len);
 void *snp_alloc_firmware_page(gfp_t mask);
 void snp_free_firmware_page(void *addr);
 void sev_platform_shutdown(void);
+bool sev_is_snp_ciphertext_hiding_supported(void);
 
 #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
 
@@ -1022,6 +1025,8 @@ static inline void snp_free_firmware_page(void *addr) { }
 
 static inline void sev_platform_shutdown(void) { }
 
+static inline bool sev_is_snp_ciphertext_hiding_supported(void) { return false; }
+
 #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
 
 #endif	/* __PSP_SEV_H__ */
-- 
2.34.1

[PATCH v7 5/7] crypto: ccp - Add support to enable CipherTextHiding on SNP_INIT_EX

[Ashish Kalra]

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

To enable ciphertext hiding, it must be specified in the SNP_INIT_EX
command as part of SNP initialization.

Modify the sev_platform_init_args structure, which is used as input to
sev_platform_init(), to include a field that, when non-zero,
indicates that ciphertext hiding should be enabled and specifies the
maximum ASID that can be used for an SEV-SNP guest.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 12 +++++++++---
 include/linux/psp-sev.h      | 10 ++++++++--
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 58c9e040e9ac..334405461657 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1186,7 +1186,7 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 	return 0;
 }
 
-static int __sev_snp_init_locked(int *error)
+static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 {
 	struct psp_device *psp = psp_master;
 	struct sev_data_snp_init_ex data;
@@ -1247,6 +1247,12 @@ static int __sev_snp_init_locked(int *error)
 		}
 
 		memset(&data, 0, sizeof(data));
+
+		if (max_snp_asid) {
+			data.ciphertext_hiding_en = 1;
+			data.max_snp_asid = max_snp_asid;
+		}
+
 		data.init_rmp = 1;
 		data.list_paddr_en = 1;
 		data.list_paddr = __psp_pa(snp_range_list);
@@ -1433,7 +1439,7 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 	if (sev->sev_plat_status.state == SEV_STATE_INIT)
 		return 0;
 
-	rc = __sev_snp_init_locked(&args->error);
+	rc = __sev_snp_init_locked(&args->error, args->max_snp_asid);
 	if (rc && rc != -ENODEV)
 		return rc;
 
@@ -1516,7 +1522,7 @@ static int snp_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_req
 {
 	int error, rc;
 
-	rc = __sev_snp_init_locked(&error);
+	rc = __sev_snp_init_locked(&error, 0);
 	if (rc) {
 		argp->error = SEV_RET_INVALID_PLATFORM_STATE;
 		return rc;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index d83185b4268b..e0dbcb4b4fd9 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -748,10 +748,13 @@ struct sev_data_snp_guest_request {
 struct sev_data_snp_init_ex {
 	u32 init_rmp:1;
 	u32 list_paddr_en:1;
-	u32 rsvd:30;
+	u32 rapl_dis:1;
+	u32 ciphertext_hiding_en:1;
+	u32 rsvd:28;
 	u32 rsvd1;
 	u64 list_paddr;
-	u8  rsvd2[48];
+	u16 max_snp_asid;
+	u8  rsvd2[46];
 } __packed;
 
 /**
@@ -800,10 +803,13 @@ struct sev_data_snp_shutdown_ex {
  * @probe: True if this is being called as part of CCP module probe, which
  *  will defer SEV_INIT/SEV_INIT_EX firmware initialization until needed
  *  unless psp_init_on_probe module param is set
+ * @max_snp_asid: When non-zero, enable ciphertext hiding and specify the
+ *  maximum ASID that can be used for an SEV-SNP guest.
  */
 struct sev_platform_init_args {
 	int error;
 	bool probe;
+	unsigned int max_snp_asid;
 };
 
 /**
-- 
2.34.1

[PATCH v7 6/7] KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables

[Ashish Kalra]

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

Introduce new min, max sev_es_asid and sev_snp_asid variables.

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

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

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 2fbdebf79fbb..b5f4e69ff579 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -85,6 +85,10 @@ static DECLARE_RWSEM(sev_deactivate_lock);
 static DEFINE_MUTEX(sev_bitmap_lock);
 unsigned int max_sev_asid;
 static unsigned int min_sev_asid;
+static unsigned int max_sev_es_asid;
+static unsigned int min_sev_es_asid;
+static unsigned int max_snp_asid;
+static unsigned int min_snp_asid;
 static unsigned long sev_me_mask;
 static unsigned int nr_asids;
 static unsigned long *sev_asid_bitmap;
@@ -173,20 +177,31 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
 	misc_cg_uncharge(type, sev->misc_cg, 1);
 }
 
-static int sev_asid_new(struct kvm_sev_info *sev)
+static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
 {
 	/*
 	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
 	 * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
-	 * Note: min ASID can end up larger than the max if basic SEV support is
-	 * effectively disabled by disallowing use of ASIDs for SEV guests.
 	 */
-	unsigned int min_asid = sev->es_active ? 1 : min_sev_asid;
-	unsigned int max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid;
-	unsigned int asid;
+	unsigned int min_asid, max_asid, asid;
 	bool retry = true;
 	int ret;
 
+	if (vm_type == KVM_X86_SNP_VM) {
+		min_asid = min_snp_asid;
+		max_asid = max_snp_asid;
+	} else if (sev->es_active) {
+		min_asid = min_sev_es_asid;
+		max_asid = max_sev_es_asid;
+	} else {
+		min_asid = min_sev_asid;
+		max_asid = max_sev_asid;
+	}
+
+	/*
+	 * The min ASID can end up larger than the max if basic SEV support is
+	 * effectively disabled by disallowing use of ASIDs for SEV guests.
+	 */
 	if (min_asid > max_asid)
 		return -ENOTTY;
 
@@ -440,7 +455,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (vm_type == KVM_X86_SNP_VM)
 		sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
 
-	ret = sev_asid_new(sev);
+	ret = sev_asid_new(sev, vm_type);
 	if (ret)
 		goto e_no_asid;
 
@@ -3042,6 +3057,9 @@ void __init sev_hardware_setup(void)
 	if (min_sev_asid == 1)
 		goto out;
 
+	min_sev_es_asid = min_snp_asid = 1;
+	max_sev_es_asid = max_snp_asid = min_sev_asid - 1;
+
 	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;
@@ -3065,11 +3083,11 @@ void __init sev_hardware_setup(void)
 	if (boot_cpu_has(X86_FEATURE_SEV_ES))
 		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
 			str_enabled_disabled(sev_es_supported),
-			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+			min_sev_es_asid, max_sev_es_asid);
 	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
 		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
 			str_enabled_disabled(sev_snp_supported),
-			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+			min_snp_asid, max_snp_asid);
 
 	sev_enabled = sev_supported;
 	sev_es_enabled = sev_es_supported;
-- 
2.34.1

[PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Ashish Kalra]

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

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

The SEV ASID space is split into SEV and SEV-ES/SEV-SNP ASID ranges.
Enabling ciphertext hiding further splits the SEV-ES/SEV-SNP ASID space
into separate ASID ranges for SEV-ES and SEV-SNP guests.

Add new module parameter to the KVM module to enable ciphertext hiding
support and a user configurable system-wide maximum SNP ASID value. If
the module parameter value is "max" then the complete SEV-ES/SEV-SNP
ASID space is allocated to SEV-SNP guests.

Suggested-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 .../admin-guide/kernel-parameters.txt         | 18 ++++++
 arch/x86/kvm/svm/sev.c                        | 60 ++++++++++++++++++-
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index eb2fab9bd0dc..379350d7ae19 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2942,6 +2942,24 @@
 			(enabled). Disable by KVM if hardware lacks support
 			for NPT.
 
+	kvm-amd.ciphertext_hiding_asids=
+			[KVM,AMD] Ciphertext hiding prevents host accesses from reading
+			the ciphertext of SNP guest private memory. Instead of reading
+			ciphertext, the host will see constant default values (0xff).
+			The SEV ASID space is split into SEV and joint SEV-ES and SEV-SNP
+			ASID space. Ciphertext hiding further partitions the joint
+			SEV-ES/SEV-SNP ASID space into separate SEV-ES and SEV-SNP ASID
+			ranges with the SEV-SNP ASID range starting at 1. For SEV-ES/
+			SEV-SNP guests the maximum ASID available is MIN_SEV_ASID - 1
+			where MIN_SEV_ASID value is discovered by CPUID Fn8000_001F[EDX].
+
+			Format: { <unsigned int> | "max" }
+			A non-zero value enables SEV-SNP ciphertext hiding feature and sets
+			the ASID range available for SEV-SNP guests.
+			A Value of "max" assigns all ASIDs available in the joint SEV-ES
+			and SEV-SNP ASID range to SNP guests, effectively disabling
+			SEV-ES.
+
 	kvm-arm.mode=
 			[KVM,ARM,EARLY] Select one of KVM/arm64's modes of
 			operation.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index b5f4e69ff579..7ac0f0f25e68 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -59,6 +59,11 @@ static bool sev_es_debug_swap_enabled = true;
 module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
 static u64 sev_supported_vmsa_features;
 
+static char ciphertext_hiding_asids[16];
+module_param_string(ciphertext_hiding_asids, ciphertext_hiding_asids,
+		    sizeof(ciphertext_hiding_asids), 0444);
+MODULE_PARM_DESC(ciphertext_hiding_asids, "  Enable ciphertext hiding for SEV-SNP guests and specify the number of ASIDs to use ('max' to utilize all available SEV-SNP ASIDs");
+
 #define AP_RESET_HOLD_NONE		0
 #define AP_RESET_HOLD_NAE_EVENT		1
 #define AP_RESET_HOLD_MSR_PROTO		2
@@ -201,6 +206,9 @@ static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
 	/*
 	 * The min ASID can end up larger than the max if basic SEV support is
 	 * effectively disabled by disallowing use of ASIDs for SEV guests.
+	 * Similarly for SEV-ES guests the min ASID can end up larger than the
+	 * max when ciphertext hiding is enabled, effectively disabling SEV-ES
+	 * support.
 	 */
 	if (min_asid > max_asid)
 		return -ENOTTY;
@@ -2269,6 +2277,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
 				ret = -EFAULT;
 				goto err;
 			}
+
 			kunmap_local(vaddr);
 		}
 
@@ -2959,6 +2968,46 @@ static bool is_sev_snp_initialized(void)
 	return initialized;
 }
 
+static bool check_and_enable_sev_snp_ciphertext_hiding(void)
+{
+	unsigned int ciphertext_hiding_asid_nr = 0;
+
+	if (!ciphertext_hiding_asids[0])
+		return false;
+
+	if (!sev_is_snp_ciphertext_hiding_supported()) {
+		pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
+		return false;
+	}
+
+	if (isdigit(ciphertext_hiding_asids[0])) {
+		if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
+			goto invalid_parameter;
+
+		/* Do sanity check on user-defined ciphertext_hiding_asids */
+		if (ciphertext_hiding_asid_nr >= min_sev_asid) {
+			pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
+				ciphertext_hiding_asid_nr, min_sev_asid);
+			return false;
+		}
+	} else if (!strcmp(ciphertext_hiding_asids, "max")) {
+		ciphertext_hiding_asid_nr = min_sev_asid - 1;
+	}
+
+	if (ciphertext_hiding_asid_nr) {
+		max_snp_asid = ciphertext_hiding_asid_nr;
+		min_sev_es_asid = max_snp_asid + 1;
+		pr_info("SEV-SNP ciphertext hiding enabled\n");
+
+		return true;
+	}
+
+invalid_parameter:
+	pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+		ciphertext_hiding_asids);
+	return false;
+}
+
 void __init sev_hardware_setup(void)
 {
 	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
@@ -3068,6 +3117,13 @@ void __init sev_hardware_setup(void)
 out:
 	if (sev_enabled) {
 		init_args.probe = true;
+		/*
+		 * The ciphertext hiding feature partitions the joint SEV-ES/SEV-SNP
+		 * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
+		 * the SEV-SNP ASID starting at 1.
+		 */
+		if (check_and_enable_sev_snp_ciphertext_hiding())
+			init_args.max_snp_asid = max_snp_asid;
 		if (sev_platform_init(&init_args))
 			sev_supported = sev_es_supported = sev_snp_supported = false;
 		else if (sev_snp_supported)
@@ -3082,7 +3138,9 @@ void __init sev_hardware_setup(void)
 			min_sev_asid, max_sev_asid);
 	if (boot_cpu_has(X86_FEATURE_SEV_ES))
 		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
-			str_enabled_disabled(sev_es_supported),
+			sev_es_supported ? min_sev_es_asid <= max_sev_es_asid ? "enabled" :
+										"unusable" :
+										"disabled",
 			min_sev_es_asid, max_sev_es_asid);
 	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
 		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
-- 
2.34.1

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kim Phillips]

Hi Ashish,

For patches 1 through 6 in this series:

Reviewed-by: Kim Phillips <kim.phillips@amd.com>

For this 7/7 patch, consider making the simplification changes I've supplied
in the diff at the bottom of this email: it cuts the number of lines for
check_and_enable_sev_snp_ciphertext_hiding() in half.

Thanks,

Kim

On 7/21/25 9:14 AM, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> Ciphertext hiding prevents host accesses from reading the ciphertext of
> SNP guest private memory. Instead of reading ciphertext, the host reads
> will see constant default values (0xff).
>
> The SEV ASID space is split into SEV and SEV-ES/SEV-SNP ASID ranges.
> Enabling ciphertext hiding further splits the SEV-ES/SEV-SNP ASID space
> into separate ASID ranges for SEV-ES and SEV-SNP guests.
>
> Add new module parameter to the KVM module to enable ciphertext hiding
> support and a user configurable system-wide maximum SNP ASID value. If
> the module parameter value is "max" then the complete SEV-ES/SEV-SNP
> ASID space is allocated to SEV-SNP guests.
>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>   .../admin-guide/kernel-parameters.txt         | 18 ++++++
>   arch/x86/kvm/svm/sev.c                        | 60 ++++++++++++++++++-
>   2 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index eb2fab9bd0dc..379350d7ae19 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2942,6 +2942,24 @@
>   			(enabled). Disable by KVM if hardware lacks support
>   			for NPT.
>   
> +	kvm-amd.ciphertext_hiding_asids=
> +			[KVM,AMD] Ciphertext hiding prevents host accesses from reading
> +			the ciphertext of SNP guest private memory. Instead of reading
> +			ciphertext, the host will see constant default values (0xff).
> +			The SEV ASID space is split into SEV and joint SEV-ES and SEV-SNP
> +			ASID space. Ciphertext hiding further partitions the joint
> +			SEV-ES/SEV-SNP ASID space into separate SEV-ES and SEV-SNP ASID
> +			ranges with the SEV-SNP ASID range starting at 1. For SEV-ES/
> +			SEV-SNP guests the maximum ASID available is MIN_SEV_ASID - 1
> +			where MIN_SEV_ASID value is discovered by CPUID Fn8000_001F[EDX].
> +
> +			Format: { <unsigned int> | "max" }
> +			A non-zero value enables SEV-SNP ciphertext hiding feature and sets
> +			the ASID range available for SEV-SNP guests.
> +			A Value of "max" assigns all ASIDs available in the joint SEV-ES
> +			and SEV-SNP ASID range to SNP guests, effectively disabling
> +			SEV-ES.
> +
>   	kvm-arm.mode=
>   			[KVM,ARM,EARLY] Select one of KVM/arm64's modes of
>   			operation.
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index b5f4e69ff579..7ac0f0f25e68 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -59,6 +59,11 @@ static bool sev_es_debug_swap_enabled = true;
>   module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>   static u64 sev_supported_vmsa_features;
>   
> +static char ciphertext_hiding_asids[16];
> +module_param_string(ciphertext_hiding_asids, ciphertext_hiding_asids,
> +		    sizeof(ciphertext_hiding_asids), 0444);
> +MODULE_PARM_DESC(ciphertext_hiding_asids, "  Enable ciphertext hiding for SEV-SNP guests and specify the number of ASIDs to use ('max' to utilize all available SEV-SNP ASIDs");
> +
>   #define AP_RESET_HOLD_NONE		0
>   #define AP_RESET_HOLD_NAE_EVENT		1
>   #define AP_RESET_HOLD_MSR_PROTO		2
> @@ -201,6 +206,9 @@ static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
>   	/*
>   	 * The min ASID can end up larger than the max if basic SEV support is
>   	 * effectively disabled by disallowing use of ASIDs for SEV guests.
> +	 * Similarly for SEV-ES guests the min ASID can end up larger than the
> +	 * max when ciphertext hiding is enabled, effectively disabling SEV-ES
> +	 * support.
>   	 */
>   	if (min_asid > max_asid)
>   		return -ENOTTY;
> @@ -2269,6 +2277,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
>   				ret = -EFAULT;
>   				goto err;
>   			}
> +
>   			kunmap_local(vaddr);
>   		}
>   
> @@ -2959,6 +2968,46 @@ static bool is_sev_snp_initialized(void)
>   	return initialized;
>   }
>   
> +static bool check_and_enable_sev_snp_ciphertext_hiding(void)
> +{
> +	unsigned int ciphertext_hiding_asid_nr = 0;
> +
> +	if (!ciphertext_hiding_asids[0])
> +		return false;
> +
> +	if (!sev_is_snp_ciphertext_hiding_supported()) {
> +		pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
> +		return false;
> +	}
> +
> +	if (isdigit(ciphertext_hiding_asids[0])) {
> +		if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
> +			goto invalid_parameter;
> +
> +		/* Do sanity check on user-defined ciphertext_hiding_asids */
> +		if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> +			pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
> +				ciphertext_hiding_asid_nr, min_sev_asid);
> +			return false;
> +		}
> +	} else if (!strcmp(ciphertext_hiding_asids, "max")) {
> +		ciphertext_hiding_asid_nr = min_sev_asid - 1;
> +	}
> +
> +	if (ciphertext_hiding_asid_nr) {
> +		max_snp_asid = ciphertext_hiding_asid_nr;
> +		min_sev_es_asid = max_snp_asid + 1;
> +		pr_info("SEV-SNP ciphertext hiding enabled\n");
> +
> +		return true;
> +	}
> +
> +invalid_parameter:
> +	pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> +		ciphertext_hiding_asids);
> +	return false;
> +}
> +
>   void __init sev_hardware_setup(void)
>   {
>   	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
> @@ -3068,6 +3117,13 @@ void __init sev_hardware_setup(void)
>   out:
>   	if (sev_enabled) {
>   		init_args.probe = true;
> +		/*
> +		 * The ciphertext hiding feature partitions the joint SEV-ES/SEV-SNP
> +		 * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
> +		 * the SEV-SNP ASID starting at 1.
> +		 */
> +		if (check_and_enable_sev_snp_ciphertext_hiding())
> +			init_args.max_snp_asid = max_snp_asid;
>   		if (sev_platform_init(&init_args))
>   			sev_supported = sev_es_supported = sev_snp_supported = false;
>   		else if (sev_snp_supported)
> @@ -3082,7 +3138,9 @@ void __init sev_hardware_setup(void)
>   			min_sev_asid, max_sev_asid);
>   	if (boot_cpu_has(X86_FEATURE_SEV_ES))
>   		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
> -			str_enabled_disabled(sev_es_supported),
> +			sev_es_supported ? min_sev_es_asid <= max_sev_es_asid ? "enabled" :
> +										"unusable" :
> +										"disabled",
>   			min_sev_es_asid, max_sev_es_asid);
>   	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
>   		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",


diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7ac0f0f25e68..bd0947360e18 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -59,7 +59,7 @@ static bool sev_es_debug_swap_enabled = true;
  module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
  static u64 sev_supported_vmsa_features;

-static char ciphertext_hiding_asids[16];
+static char ciphertext_hiding_asids[10];
  module_param_string(ciphertext_hiding_asids, ciphertext_hiding_asids,
              sizeof(ciphertext_hiding_asids), 0444);
  MODULE_PARM_DESC(ciphertext_hiding_asids, "  Enable ciphertext hiding 
for SEV-SNP guests and specify the number of ASIDs to use ('max' to 
utilize all available SEV-SNP ASIDs");
@@ -2970,42 +2970,22 @@ static bool is_sev_snp_initialized(void)

  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
  {
-    unsigned int ciphertext_hiding_asid_nr = 0;
-
-    if (!ciphertext_hiding_asids[0])
-        return false;
-
-    if (!sev_is_snp_ciphertext_hiding_supported()) {
-        pr_warn("Module parameter ciphertext_hiding_asids specified but 
ciphertext hiding not supported\n");
-        return false;
-    }
-
-    if (isdigit(ciphertext_hiding_asids[0])) {
-        if (kstrtoint(ciphertext_hiding_asids, 10, 
&ciphertext_hiding_asid_nr))
-            goto invalid_parameter;
-
-        /* Do sanity check on user-defined ciphertext_hiding_asids */
-        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
-            pr_warn("Module parameter ciphertext_hiding_asids (%u) 
exceeds or equals minimum SEV ASID (%u)\n",
-                ciphertext_hiding_asid_nr, min_sev_asid);
-            return false;
-        }
-    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
-        ciphertext_hiding_asid_nr = min_sev_asid - 1;
+    if (!strcmp(ciphertext_hiding_asids, "max")) {
+        max_snp_asid = min_sev_asid - 1;
+        return true;
      }

-    if (ciphertext_hiding_asid_nr) {
-        max_snp_asid = ciphertext_hiding_asid_nr;
-        min_sev_es_asid = max_snp_asid + 1;
-        pr_info("SEV-SNP ciphertext hiding enabled\n");
-
-        return true;
+    /* Do sanity check on user-defined ciphertext_hiding_asids */
+    if (kstrtoint(ciphertext_hiding_asids, 
sizeof(ciphertext_hiding_asids), &max_snp_asid) ||
+        max_snp_asid >= min_sev_asid ||
+        !sev_is_snp_ciphertext_hiding_supported()) {
+        pr_warn("ciphertext_hiding not supported, or invalid 
ciphertext_hiding_asids \"%s\", or !(0 < %u < minimum SEV ASID %u)\n",
+            ciphertext_hiding_asids, max_snp_asid, min_sev_asid);
+        max_snp_asid = min_sev_asid - 1;
+        return false;
      }

-invalid_parameter:
-    pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
-        ciphertext_hiding_asids);
-    return false;
+    return true;
  }

  void __init sev_hardware_setup(void)
@@ -3122,8 +3102,11 @@ void __init sev_hardware_setup(void)
           * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
           * the SEV-SNP ASID starting at 1.
           */
-        if (check_and_enable_sev_snp_ciphertext_hiding())
+        if (check_and_enable_sev_snp_ciphertext_hiding()) {
+            pr_info("SEV-SNP ciphertext hiding enabled\n");
              init_args.max_snp_asid = max_snp_asid;
+            min_sev_es_asid = max_snp_asid + 1;
+        }
          if (sev_platform_init(&init_args))
              sev_supported = sev_es_supported = sev_snp_supported = false;
          else if (sev_snp_supported)

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Tom Lendacky]

On 7/25/25 12:58, Kim Phillips wrote:
> Hi Ashish,
> 
> For patches 1 through 6 in this series:
> 
> Reviewed-by: Kim Phillips <kim.phillips@amd.com>
> 
> For this 7/7 patch, consider making the simplification changes I've supplied
> in the diff at the bottom of this email: it cuts the number of lines for
> check_and_enable_sev_snp_ciphertext_hiding() in half.

Not sure that change works completely... see below.

> 
> Thanks,
> 
> Kim
> 
> On 7/21/25 9:14 AM, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>

> 
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7ac0f0f25e68..bd0947360e18 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -59,7 +59,7 @@ static bool sev_es_debug_swap_enabled = true;
>  module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>  static u64 sev_supported_vmsa_features;
> 
> -static char ciphertext_hiding_asids[16];
> +static char ciphertext_hiding_asids[10];
>  module_param_string(ciphertext_hiding_asids, ciphertext_hiding_asids,
>              sizeof(ciphertext_hiding_asids), 0444);
>  MODULE_PARM_DESC(ciphertext_hiding_asids, "  Enable ciphertext hiding for
> SEV-SNP guests and specify the number of ASIDs to use ('max' to utilize
> all available SEV-SNP ASIDs");
> @@ -2970,42 +2970,22 @@ static bool is_sev_snp_initialized(void)
> 
>  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>  {
> -    unsigned int ciphertext_hiding_asid_nr = 0;
> -
> -    if (!ciphertext_hiding_asids[0])
> -        return false;

If the parameter was never specified
> -
> -    if (!sev_is_snp_ciphertext_hiding_supported()) {
> -        pr_warn("Module parameter ciphertext_hiding_asids specified but
> ciphertext hiding not supported\n");
> -        return false;
> -    }

Removing this block will create an issue below.

> -
> -    if (isdigit(ciphertext_hiding_asids[0])) {
> -        if (kstrtoint(ciphertext_hiding_asids, 10,
> &ciphertext_hiding_asid_nr))
> -            goto invalid_parameter;
> -
> -        /* Do sanity check on user-defined ciphertext_hiding_asids */
> -        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> -            pr_warn("Module parameter ciphertext_hiding_asids (%u)
> exceeds or equals minimum SEV ASID (%u)\n",
> -                ciphertext_hiding_asid_nr, min_sev_asid);
> -            return false;
> -        }
> -    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
> -        ciphertext_hiding_asid_nr = min_sev_asid - 1;
> +    if (!strcmp(ciphertext_hiding_asids, "max")) {
> +        max_snp_asid = min_sev_asid - 1;
> +        return true;
>      }
> 
> -    if (ciphertext_hiding_asid_nr) {
> -        max_snp_asid = ciphertext_hiding_asid_nr;
> -        min_sev_es_asid = max_snp_asid + 1;
> -        pr_info("SEV-SNP ciphertext hiding enabled\n");
> -
> -        return true;
> +    /* Do sanity check on user-defined ciphertext_hiding_asids */
> +    if (kstrtoint(ciphertext_hiding_asids,
> sizeof(ciphertext_hiding_asids), &max_snp_asid) ||

The second parameter is supposed to be the base, this gets lucky because
you changed the size of the ciphertext_hiding_asids to 10.

> +        max_snp_asid >= min_sev_asid ||
> +        !sev_is_snp_ciphertext_hiding_supported()) {
> +        pr_warn("ciphertext_hiding not supported, or invalid
> ciphertext_hiding_asids \"%s\", or !(0 < %u < minimum SEV ASID %u)\n",
> +            ciphertext_hiding_asids, max_snp_asid, min_sev_asid);
> +        max_snp_asid = min_sev_asid - 1;
> +        return false;
>      }
> 
> -invalid_parameter:
> -    pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> -        ciphertext_hiding_asids);
> -    return false;
> +    return true;
>  }
> 
>  void __init sev_hardware_setup(void)
> @@ -3122,8 +3102,11 @@ void __init sev_hardware_setup(void)
>           * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
>           * the SEV-SNP ASID starting at 1.
>           */
> -        if (check_and_enable_sev_snp_ciphertext_hiding())
> +        if (check_and_enable_sev_snp_ciphertext_hiding()) {
> +            pr_info("SEV-SNP ciphertext hiding enabled\n");
>              init_args.max_snp_asid = max_snp_asid;
> +            min_sev_es_asid = max_snp_asid + 1;

If "max" was specified, but ciphertext hiding isn't enabled, you've now
changed min_sev_es_asid to an incorrect value and will be trying to enable
ciphertext hiding during initialization.

Thanks,
Tom

> +        }
>          if (sev_platform_init(&init_args))
>              sev_supported = sev_es_supported = sev_snp_supported = false;
>          else if (sev_snp_supported)
> 

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kalra, Ashish]

On 7/25/2025 1:28 PM, Tom Lendacky wrote:
> On 7/25/25 12:58, Kim Phillips wrote:
>> Hi Ashish,
>>
>> For patches 1 through 6 in this series:
>>
>> Reviewed-by: Kim Phillips <kim.phillips@amd.com>
>>
>> For this 7/7 patch, consider making the simplification changes I've supplied
>> in the diff at the bottom of this email: it cuts the number of lines for
>> check_and_enable_sev_snp_ciphertext_hiding() in half.
> 
> Not sure that change works completely... see below.
> 
>>
>> Thanks,
>>
>> Kim
>>
>> On 7/21/25 9:14 AM, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@amd.com>
> 
>>
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 7ac0f0f25e68..bd0947360e18 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -59,7 +59,7 @@ static bool sev_es_debug_swap_enabled = true;
>>  module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>>  static u64 sev_supported_vmsa_features;
>>
>> -static char ciphertext_hiding_asids[16];
>> +static char ciphertext_hiding_asids[10];
>>  module_param_string(ciphertext_hiding_asids, ciphertext_hiding_asids,
>>              sizeof(ciphertext_hiding_asids), 0444);
>>  MODULE_PARM_DESC(ciphertext_hiding_asids, "  Enable ciphertext hiding for
>> SEV-SNP guests and specify the number of ASIDs to use ('max' to utilize
>> all available SEV-SNP ASIDs");
>> @@ -2970,42 +2970,22 @@ static bool is_sev_snp_initialized(void)
>>
>>  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>>  {
>> -    unsigned int ciphertext_hiding_asid_nr = 0;
>> -
>> -    if (!ciphertext_hiding_asids[0])
>> -        return false;
> 
> If the parameter was never specified
>> -
>> -    if (!sev_is_snp_ciphertext_hiding_supported()) {
>> -        pr_warn("Module parameter ciphertext_hiding_asids specified but
>> ciphertext hiding not supported\n");
>> -        return false;
>> -    }
> 
> Removing this block will create an issue below.
> 
>> -
>> -    if (isdigit(ciphertext_hiding_asids[0])) {
>> -        if (kstrtoint(ciphertext_hiding_asids, 10,
>> &ciphertext_hiding_asid_nr))
>> -            goto invalid_parameter;
>> -
>> -        /* Do sanity check on user-defined ciphertext_hiding_asids */
>> -        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>> -            pr_warn("Module parameter ciphertext_hiding_asids (%u)
>> exceeds or equals minimum SEV ASID (%u)\n",
>> -                ciphertext_hiding_asid_nr, min_sev_asid);
>> -            return false;
>> -        }
>> -    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
>> -        ciphertext_hiding_asid_nr = min_sev_asid - 1;
>> +    if (!strcmp(ciphertext_hiding_asids, "max")) {
>> +        max_snp_asid = min_sev_asid - 1;
>> +        return true;
>>      }

As Tom has already pointed out, we will try enabling ciphertext hiding with SNP_INIT_EX even if ciphertext hiding feature is not supported and enabled.

We do need to make these basic checks, i.e., if the parameter has been specified and if ciphertext hiding feature is supported and enabled, 
before doing any further processing.

Why should we even attempt to do any parameter comparison, parameter conversion or sanity checks if the parameter has not been specified and/or
ciphertext hiding feature itself is not supported and enabled.

I believe this function should be simple and understandable which it is.

Thanks,
Ashish

>>
>> -    if (ciphertext_hiding_asid_nr) {
>> -        max_snp_asid = ciphertext_hiding_asid_nr;
>> -        min_sev_es_asid = max_snp_asid + 1;
>> -        pr_info("SEV-SNP ciphertext hiding enabled\n");
>> -
>> -        return true;
>> +    /* Do sanity check on user-defined ciphertext_hiding_asids */
>> +    if (kstrtoint(ciphertext_hiding_asids,
>> sizeof(ciphertext_hiding_asids), &max_snp_asid) ||
> 
> The second parameter is supposed to be the base, this gets lucky because
> you changed the size of the ciphertext_hiding_asids to 10.
> 
>> +        max_snp_asid >= min_sev_asid ||
>> +        !sev_is_snp_ciphertext_hiding_supported()) {
>> +        pr_warn("ciphertext_hiding not supported, or invalid
>> ciphertext_hiding_asids \"%s\", or !(0 < %u < minimum SEV ASID %u)\n",
>> +            ciphertext_hiding_asids, max_snp_asid, min_sev_asid);
>> +        max_snp_asid = min_sev_asid - 1;
>> +        return false;
>>      }
>>
>> -invalid_parameter:
>> -    pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>> -        ciphertext_hiding_asids);
>> -    return false;
>> +    return true;
>>  }
>>
>>  void __init sev_hardware_setup(void)
>> @@ -3122,8 +3102,11 @@ void __init sev_hardware_setup(void)
>>           * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
>>           * the SEV-SNP ASID starting at 1.
>>           */
>> -        if (check_and_enable_sev_snp_ciphertext_hiding())
>> +        if (check_and_enable_sev_snp_ciphertext_hiding()) {
>> +            pr_info("SEV-SNP ciphertext hiding enabled\n");
>>              init_args.max_snp_asid = max_snp_asid;
>> +            min_sev_es_asid = max_snp_asid + 1;
> 
> If "max" was specified, but ciphertext hiding isn't enabled, you've now
> changed min_sev_es_asid to an incorrect value and will be trying to enable
> ciphertext hiding during initialization.
> 
> Thanks,
> Tom
> 
>> +        }
>>          if (sev_platform_init(&init_args))
>>              sev_supported = sev_es_supported = sev_snp_supported = false;
>>          else if (sev_snp_supported)
>>
> 

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Ashish Kalra]

Hi Herbert, can you please merge patches 1-5.

Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
for patches 6 & 7.

Thanks,
Ashish

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kim Phillips]

On 7/25/25 1:46 PM, Kalra, Ashish wrote:
> On 7/25/2025 1:28 PM, Tom Lendacky wrote:
>> On 7/25/25 12:58, Kim Phillips wrote:
>>> Hi Ashish,
>>>
>>> For patches 1 through 6 in this series:
>>>
>>> Reviewed-by: Kim Phillips <kim.phillips@amd.com>
>>>
>>> For this 7/7 patch, consider making the simplification changes I've supplied
>>> in the diff at the bottom of this email: it cuts the number of lines for
>>> check_and_enable_sev_snp_ciphertext_hiding() in half.
>> Not sure that change works completely... see below.
>>
>>> Thanks,
>>>
>>> Kim
>>>
>>> On 7/21/25 9:14 AM, Ashish Kalra wrote:
>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 7ac0f0f25e68..bd0947360e18 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -59,7 +59,7 @@ static bool sev_es_debug_swap_enabled = true;
>>>   module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>>>   static u64 sev_supported_vmsa_features;
>>>
>>> -static char ciphertext_hiding_asids[16];
>>> +static char ciphertext_hiding_asids[10];
>>>   module_param_string(ciphertext_hiding_asids, ciphertext_hiding_asids,
>>>               sizeof(ciphertext_hiding_asids), 0444);
>>>   MODULE_PARM_DESC(ciphertext_hiding_asids, "  Enable ciphertext hiding for
>>> SEV-SNP guests and specify the number of ASIDs to use ('max' to utilize
>>> all available SEV-SNP ASIDs");
>>> @@ -2970,42 +2970,22 @@ static bool is_sev_snp_initialized(void)
>>>
>>>   static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>>>   {
>>> -    unsigned int ciphertext_hiding_asid_nr = 0;
>>> -
>>> -    if (!ciphertext_hiding_asids[0])
>>> -        return false;
>> If the parameter was never specified
>>> -
>>> -    if (!sev_is_snp_ciphertext_hiding_supported()) {
>>> -        pr_warn("Module parameter ciphertext_hiding_asids specified but
>>> ciphertext hiding not supported\n");
>>> -        return false;
>>> -    }
>> Removing this block will create an issue below.
>>
>>> -
>>> -    if (isdigit(ciphertext_hiding_asids[0])) {
>>> -        if (kstrtoint(ciphertext_hiding_asids, 10,
>>> &ciphertext_hiding_asid_nr))
>>> -            goto invalid_parameter;
>>> -
>>> -        /* Do sanity check on user-defined ciphertext_hiding_asids */
>>> -        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>>> -            pr_warn("Module parameter ciphertext_hiding_asids (%u)
>>> exceeds or equals minimum SEV ASID (%u)\n",
>>> -                ciphertext_hiding_asid_nr, min_sev_asid);
>>> -            return false;
>>> -        }
>>> -    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
>>> -        ciphertext_hiding_asid_nr = min_sev_asid - 1;
>>> +    if (!strcmp(ciphertext_hiding_asids, "max")) {
>>> +        max_snp_asid = min_sev_asid - 1;
>>> +        return true;
>>>       }
> As Tom has already pointed out, we will try enabling ciphertext hiding with SNP_INIT_EX even if ciphertext hiding feature is not supported and enabled.
AFAICT, Tom pointed out two bugs with my changes: the 'base' argument to 
kstrtoint(), and bad min_sev_es_asid assignment if ciphertext hiding 
isn't supported.
> We do need to make these basic checks, i.e., if the parameter has been specified and if ciphertext hiding feature is supported and enabled,
> before doing any further processing.
>
> Why should we even attempt to do any parameter comparison, parameter conversion or sanity checks if the parameter has not been specified and/or
> ciphertext hiding feature itself is not supported and enabled.
Agreed.
> I believe this function should be simple and understandable which it is.
Please take a look at the new diff below: I believe it's even simpler 
and more understandable as it's less code, and now alerts the user if 
they provide an empty "ciphertext_hiding_asids= ".

Thanks,

Kim

  arch/x86/kvm/svm/sev.c | 47 
++++++++++++++++++-----------------------------
  1 file changed, 18 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7ac0f0f25e68..57c6e4717e51 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2970,42 +2970,29 @@ static bool is_sev_snp_initialized(void)

  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
  {
-       unsigned int ciphertext_hiding_asid_nr = 0;
-
-       if (!ciphertext_hiding_asids[0])
-               return false;
-
-       if (!sev_is_snp_ciphertext_hiding_supported()) {
+       if (ciphertext_hiding_asids[0] && 
!sev_is_snp_ciphertext_hiding_supported()) {
                 pr_warn("Module parameter ciphertext_hiding_asids 
specified but ciphertext hiding not supported\n");
                 return false;
         }

-       if (isdigit(ciphertext_hiding_asids[0])) {
-               if (kstrtoint(ciphertext_hiding_asids, 10, 
&ciphertext_hiding_asid_nr))
-                       goto invalid_parameter;
-
-               /* Do sanity check on user-defined 
ciphertext_hiding_asids */
-               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
-                       pr_warn("Module parameter 
ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
-                               ciphertext_hiding_asid_nr, min_sev_asid);
-                       return false;
-               }
-       } else if (!strcmp(ciphertext_hiding_asids, "max")) {
-               ciphertext_hiding_asid_nr = min_sev_asid - 1;
-       }
-
-       if (ciphertext_hiding_asid_nr) {
-               max_snp_asid = ciphertext_hiding_asid_nr;
+       if (!strcmp(ciphertext_hiding_asids, "max")) {
+               max_snp_asid = min_sev_asid - 1;
                 min_sev_es_asid = max_snp_asid + 1;
-               pr_info("SEV-SNP ciphertext hiding enabled\n");
-
                 return true;
         }

-invalid_parameter:
-       pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
-               ciphertext_hiding_asids);
-       return false;
+       /* Do sanity check on user-defined ciphertext_hiding_asids */
+       if (kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid) ||
+           max_snp_asid >= min_sev_asid) {
+               pr_warn("invalid ciphertext_hiding_asids \"%s\" or !(0 < 
%u < minimum SEV ASID %u)\n",
+                       ciphertext_hiding_asids, max_snp_asid, 
min_sev_asid);
+               max_snp_asid = min_sev_asid - 1;
+               return false;
+       }
+
+       min_sev_es_asid = max_snp_asid + 1;
+
+       return true;
  }

  void __init sev_hardware_setup(void)
@@ -3122,8 +3109,10 @@ void __init sev_hardware_setup(void)
                  * ASID range into separate SEV-ES and SEV-SNP ASID 
ranges with
                  * the SEV-SNP ASID starting at 1.
                  */
-               if (check_and_enable_sev_snp_ciphertext_hiding())
+               if (check_and_enable_sev_snp_ciphertext_hiding()) {
+                       pr_info("SEV-SNP ciphertext hiding enabled\n");
                         init_args.max_snp_asid = max_snp_asid;
+               }
                 if (sev_platform_init(&init_args))
                         sev_supported = sev_es_supported = 
sev_snp_supported = false;
                 else if (sev_snp_supported)


> Thanks,
> Ashish
>
>>> -    if (ciphertext_hiding_asid_nr) {
>>> -        max_snp_asid = ciphertext_hiding_asid_nr;
>>> -        min_sev_es_asid = max_snp_asid + 1;
>>> -        pr_info("SEV-SNP ciphertext hiding enabled\n");
>>> -
>>> -        return true;
>>> +    /* Do sanity check on user-defined ciphertext_hiding_asids */
>>> +    if (kstrtoint(ciphertext_hiding_asids,
>>> sizeof(ciphertext_hiding_asids), &max_snp_asid) ||
>> The second parameter is supposed to be the base, this gets lucky because
>> you changed the size of the ciphertext_hiding_asids to 10.
>>
>>> +        max_snp_asid >= min_sev_asid ||
>>> +        !sev_is_snp_ciphertext_hiding_supported()) {
>>> +        pr_warn("ciphertext_hiding not supported, or invalid
>>> ciphertext_hiding_asids \"%s\", or !(0 < %u < minimum SEV ASID %u)\n",
>>> +            ciphertext_hiding_asids, max_snp_asid, min_sev_asid);
>>> +        max_snp_asid = min_sev_asid - 1;
>>> +        return false;
>>>       }
>>>
>>> -invalid_parameter:
>>> -    pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>>> -        ciphertext_hiding_asids);
>>> -    return false;
>>> +    return true;
>>>   }
>>>
>>>   void __init sev_hardware_setup(void)
>>> @@ -3122,8 +3102,11 @@ void __init sev_hardware_setup(void)
>>>            * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
>>>            * the SEV-SNP ASID starting at 1.
>>>            */
>>> -        if (check_and_enable_sev_snp_ciphertext_hiding())
>>> +        if (check_and_enable_sev_snp_ciphertext_hiding()) {
>>> +            pr_info("SEV-SNP ciphertext hiding enabled\n");
>>>               init_args.max_snp_asid = max_snp_asid;
>>> +            min_sev_es_asid = max_snp_asid + 1;
>> If "max" was specified, but ciphertext hiding isn't enabled, you've now
>> changed min_sev_es_asid to an incorrect value and will be trying to enable
>> ciphertext hiding during initialization.
>>
>> Thanks,
>> Tom
>>
>>> +        }
>>>           if (sev_platform_init(&init_args))
>>>               sev_supported = sev_es_supported = sev_snp_supported = false;
>>>           else if (sev_snp_supported)
>>>

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kalra, Ashish]

On 8/12/2025 7:06 AM, Kim Phillips wrote:
> On 7/25/25 1:46 PM, Kalra, Ashish wrote:
>> On 7/25/2025 1:28 PM, Tom Lendacky wrote:
>>> On 7/25/25 12:58, Kim Phillips wrote:
>>>> Hi Ashish,
>>>>
>>>> For patches 1 through 6 in this series:
>>>>
>>>> Reviewed-by: Kim Phillips <kim.phillips@amd.com>
>>>>
>>>> For this 7/7 patch, consider making the simplification changes I've supplied
>>>> in the diff at the bottom of this email: it cuts the number of lines for
>>>> check_and_enable_sev_snp_ciphertext_hiding() in half.
>>> Not sure that change works completely... see below.
>>>
>>>> Thanks,
>>>>
>>>> Kim
>>>>
>>>> On 7/21/25 9:14 AM, Ashish Kalra wrote:
>>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>> index 7ac0f0f25e68..bd0947360e18 100644
>>>> --- a/arch/x86/kvm/svm/sev.c
>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>> @@ -59,7 +59,7 @@ static bool sev_es_debug_swap_enabled = true;
>>>>   module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>>>>   static u64 sev_supported_vmsa_features;
>>>>
>>>> -static char ciphertext_hiding_asids[16];
>>>> +static char ciphertext_hiding_asids[10];
>>>>   module_param_string(ciphertext_hiding_asids, ciphertext_hiding_asids,
>>>>               sizeof(ciphertext_hiding_asids), 0444);
>>>>   MODULE_PARM_DESC(ciphertext_hiding_asids, "  Enable ciphertext hiding for
>>>> SEV-SNP guests and specify the number of ASIDs to use ('max' to utilize
>>>> all available SEV-SNP ASIDs");
>>>> @@ -2970,42 +2970,22 @@ static bool is_sev_snp_initialized(void)
>>>>
>>>>   static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>>>>   {
>>>> -    unsigned int ciphertext_hiding_asid_nr = 0;
>>>> -
>>>> -    if (!ciphertext_hiding_asids[0])
>>>> -        return false;
>>> If the parameter was never specified
>>>> -
>>>> -    if (!sev_is_snp_ciphertext_hiding_supported()) {
>>>> -        pr_warn("Module parameter ciphertext_hiding_asids specified but
>>>> ciphertext hiding not supported\n");
>>>> -        return false;
>>>> -    }
>>> Removing this block will create an issue below.
>>>
>>>> -
>>>> -    if (isdigit(ciphertext_hiding_asids[0])) {
>>>> -        if (kstrtoint(ciphertext_hiding_asids, 10,
>>>> &ciphertext_hiding_asid_nr))
>>>> -            goto invalid_parameter;
>>>> -
>>>> -        /* Do sanity check on user-defined ciphertext_hiding_asids */
>>>> -        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>>>> -            pr_warn("Module parameter ciphertext_hiding_asids (%u)
>>>> exceeds or equals minimum SEV ASID (%u)\n",
>>>> -                ciphertext_hiding_asid_nr, min_sev_asid);
>>>> -            return false;
>>>> -        }
>>>> -    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
>>>> -        ciphertext_hiding_asid_nr = min_sev_asid - 1;
>>>> +    if (!strcmp(ciphertext_hiding_asids, "max")) {
>>>> +        max_snp_asid = min_sev_asid - 1;
>>>> +        return true;
>>>>       }
>> As Tom has already pointed out, we will try enabling ciphertext hiding with SNP_INIT_EX even if ciphertext hiding feature is not supported and enabled.
> AFAICT, Tom pointed out two bugs with my changes: the 'base' argument to kstrtoint(), and bad min_sev_es_asid assignment if ciphertext hiding isn't supported.
>> We do need to make these basic checks, i.e., if the parameter has been specified and if ciphertext hiding feature is supported and enabled,
>> before doing any further processing.
>>
>> Why should we even attempt to do any parameter comparison, parameter conversion or sanity checks if the parameter has not been specified and/or
>> ciphertext hiding feature itself is not supported and enabled.
> Agreed.
>> I believe this function should be simple and understandable which it is.
> Please take a look at the new diff below: I believe it's even simpler and more understandable as it's less code, and now alerts the user if they provide an empty "ciphertext_hiding_asids= ".
> 
> Thanks,
> 
> Kim
> 
>  arch/x86/kvm/svm/sev.c | 47 ++++++++++++++++++-----------------------------
>  1 file changed, 18 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7ac0f0f25e68..57c6e4717e51 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2970,42 +2970,29 @@ static bool is_sev_snp_initialized(void)
> 
>  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>  {
> -       unsigned int ciphertext_hiding_asid_nr = 0;
> -
> -       if (!ciphertext_hiding_asids[0])
> -               return false;
> -
> -       if (!sev_is_snp_ciphertext_hiding_supported()) {
> +       if (ciphertext_hiding_asids[0] && !sev_is_snp_ciphertext_hiding_supported()) {
>                 pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
>                 return false;
>         }
> 

This is incorrect, if ciphertext_hiding_asids module parameter is never specified, user will always 
get a warning of an invalid ciphertext_hiding_asids module parameter.

When this module parameter is optional why should the user get a warning about an invalid module parameter.

Again, why do we want to do all these checks below if this module parameter has not been specified by
the user ?

> -       if (isdigit(ciphertext_hiding_asids[0])) {
> -               if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
> -                       goto invalid_parameter;
> -
> -               /* Do sanity check on user-defined ciphertext_hiding_asids */
> -               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> -                       pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
> -                               ciphertext_hiding_asid_nr, min_sev_asid);

A *combined* error message such as this: 
"invalid ciphertext_hiding_asids XXX or !(0 < XXX < minimum SEV ASID 100)"

is going to be really confusing to the user.

It is much simpler for user to understand if the error/warning is: 
"Module parameter ciphertext_hiding_asids XXX exceeds or equals minimum SEV ASID YYY"
OR
"Module parameter ciphertext_hiding_asids XXX invalid"

Thanks,
Ashish

> -                       return false;
> -               }
> -       } else if (!strcmp(ciphertext_hiding_asids, "max")) {
> -               ciphertext_hiding_asid_nr = min_sev_asid - 1;
> -       }
> -
> -       if (ciphertext_hiding_asid_nr) {
> -               max_snp_asid = ciphertext_hiding_asid_nr;
> +       if (!strcmp(ciphertext_hiding_asids, "max")) {
> +               max_snp_asid = min_sev_asid - 1;
>                 min_sev_es_asid = max_snp_asid + 1;
> -               pr_info("SEV-SNP ciphertext hiding enabled\n");
> -
>                 return true;
>         }
> 
> -invalid_parameter:
> -       pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> -               ciphertext_hiding_asids);
> -       return false;
> +       /* Do sanity check on user-defined ciphertext_hiding_asids */
> +       if (kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid) ||
> +           max_snp_asid >= min_sev_asid) {
> +               pr_warn("invalid ciphertext_hiding_asids \"%s\" or !(0 < %u < minimum SEV ASID %u)\n",
> +                       ciphertext_hiding_asids, max_snp_asid, min_sev_asid);
> +               max_snp_asid = min_sev_asid - 1;
> +               return false;
> +       }
> +
> +       min_sev_es_asid = max_snp_asid + 1;
> +
> +       return true;
>  }
> 
>  void __init sev_hardware_setup(void)
> @@ -3122,8 +3109,10 @@ void __init sev_hardware_setup(void)
>                  * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
>                  * the SEV-SNP ASID starting at 1.
>                  */
> -               if (check_and_enable_sev_snp_ciphertext_hiding())
> +               if (check_and_enable_sev_snp_ciphertext_hiding()) {
> +                       pr_info("SEV-SNP ciphertext hiding enabled\n");
>                         init_args.max_snp_asid = max_snp_asid;
> +               }
>                 if (sev_platform_init(&init_args))
>                         sev_supported = sev_es_supported = sev_snp_supported = false;
>                 else if (sev_snp_supported)
> 
> 
>> Thanks,
>> Ashish
>>
>>>> -    if (ciphertext_hiding_asid_nr) {
>>>> -        max_snp_asid = ciphertext_hiding_asid_nr;
>>>> -        min_sev_es_asid = max_snp_asid + 1;
>>>> -        pr_info("SEV-SNP ciphertext hiding enabled\n");
>>>> -
>>>> -        return true;
>>>> +    /* Do sanity check on user-defined ciphertext_hiding_asids */
>>>> +    if (kstrtoint(ciphertext_hiding_asids,
>>>> sizeof(ciphertext_hiding_asids), &max_snp_asid) ||
>>> The second parameter is supposed to be the base, this gets lucky because
>>> you changed the size of the ciphertext_hiding_asids to 10.
>>>
>>>> +        max_snp_asid >= min_sev_asid ||
>>>> +        !sev_is_snp_ciphertext_hiding_supported()) {
>>>> +        pr_warn("ciphertext_hiding not supported, or invalid
>>>> ciphertext_hiding_asids \"%s\", or !(0 < %u < minimum SEV ASID %u)\n",
>>>> +            ciphertext_hiding_asids, max_snp_asid, min_sev_asid);
>>>> +        max_snp_asid = min_sev_asid - 1;
>>>> +        return false;
>>>>       }
>>>>
>>>> -invalid_parameter:
>>>> -    pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>>>> -        ciphertext_hiding_asids);
>>>> -    return false;
>>>> +    return true;
>>>>   }
>>>>
>>>>   void __init sev_hardware_setup(void)
>>>> @@ -3122,8 +3102,11 @@ void __init sev_hardware_setup(void)
>>>>            * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
>>>>            * the SEV-SNP ASID starting at 1.
>>>>            */
>>>> -        if (check_and_enable_sev_snp_ciphertext_hiding())
>>>> +        if (check_and_enable_sev_snp_ciphertext_hiding()) {
>>>> +            pr_info("SEV-SNP ciphertext hiding enabled\n");
>>>>               init_args.max_snp_asid = max_snp_asid;
>>>> +            min_sev_es_asid = max_snp_asid + 1;
>>> If "max" was specified, but ciphertext hiding isn't enabled, you've now
>>> changed min_sev_es_asid to an incorrect value and will be trying to enable
>>> ciphertext hiding during initialization.
>>>
>>> Thanks,
>>> Tom
>>>
>>>> +        }
>>>>           if (sev_platform_init(&init_args))
>>>>               sev_supported = sev_es_supported = sev_snp_supported = false;
>>>>           else if (sev_snp_supported)
>>>>
> 

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kim Phillips]

On 8/12/25 9:40 AM, Kalra, Ashish wrote:
> On 8/12/2025 7:06 AM, Kim Phillips wrote:
>>   arch/x86/kvm/svm/sev.c | 47 ++++++++++++++++++-----------------------------
>>   1 file changed, 18 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 7ac0f0f25e68..57c6e4717e51 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2970,42 +2970,29 @@ static bool is_sev_snp_initialized(void)
>>
>>   static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>>   {
>> -       unsigned int ciphertext_hiding_asid_nr = 0;
>> -
>> -       if (!ciphertext_hiding_asids[0])
>> -               return false;
>> -
>> -       if (!sev_is_snp_ciphertext_hiding_supported()) {
>> +       if (ciphertext_hiding_asids[0] && !sev_is_snp_ciphertext_hiding_supported()) {
>>                  pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
>>                  return false;
>>          }
>>
> This is incorrect, if ciphertext_hiding_asids module parameter is never specified, user will always
> get a warning of an invalid ciphertext_hiding_asids module parameter.
>
> When this module parameter is optional why should the user get a warning about an invalid module parameter.

Ack, sorry, new diff below that fixes this.

> Again, why do we want to do all these checks below if this module parameter has not been specified by
> the user ?

Not sure what you mean by 'below' here (assuming in the resulting code), 
but, in general, there are less checks with this diff than the original 
v7 code.

>> -       if (isdigit(ciphertext_hiding_asids[0])) {
>> -               if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
>> -                       goto invalid_parameter;
>> -
>> -               /* Do sanity check on user-defined ciphertext_hiding_asids */
>> -               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>> -                       pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
>> -                               ciphertext_hiding_asid_nr, min_sev_asid);
> A *combined* error message such as this:
> "invalid ciphertext_hiding_asids XXX or !(0 < XXX < minimum SEV ASID 100)"
>
> is going to be really confusing to the user.
>
> It is much simpler for user to understand if the error/warning is:
> "Module parameter ciphertext_hiding_asids XXX exceeds or equals minimum SEV ASID YYY"
> OR
> "Module parameter ciphertext_hiding_asids XXX invalid"

I tend to disagree. If, e.g., the user sets ciphertext_hiding_asids=100, 
they see:

      kvm_amd: invalid ciphertext_hiding_asids "100" or !(0 < 100 < 
minimum SEV ASID 100)

which the user can easily unmistakably and quickly deduce that the 
problem is the latter - not the former - condition that has the problem.

The original v7 code in that same case would emit:

kvm_amd: Module parameter ciphertext_hiding_asids (100) exceeds or 
equals minimum SEV ASID (100)

...to which the user would ask themselves "What's wrong with equalling 
the minimum SEV ASID (100)"?

It's not as immediately obvious that it needs to (0 < x < minimum SEV 
ASID 100).

OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:

      kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < 
minimum SEV ASID 100)

which - unlike the original v7 code - shows the user that the '0x1' was 
not interpreted as a number at all: thus the 99 in the latter condition.

But all this is nothing compared to the added simplicity resulting from 
making the change to the original v7 code.

New diff from original v7 below:

  arch/x86/kvm/svm/sev.c | 42 +++++++++++++++++-------------------------
  1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7ac0f0f25e68..a879ea5f53f2 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2970,8 +2970,6 @@ static bool is_sev_snp_initialized(void)

  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
  {
-       unsigned int ciphertext_hiding_asid_nr = 0;
-
         if (!ciphertext_hiding_asids[0])
                 return false;

@@ -2980,32 +2978,24 @@ static bool 
check_and_enable_sev_snp_ciphertext_hiding(void)
                 return false;
         }

-       if (isdigit(ciphertext_hiding_asids[0])) {
-               if (kstrtoint(ciphertext_hiding_asids, 10, 
&ciphertext_hiding_asid_nr))
-                       goto invalid_parameter;
-
-               /* Do sanity check on user-defined 
ciphertext_hiding_asids */
-               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
-                       pr_warn("Module parameter 
ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
-                               ciphertext_hiding_asid_nr, min_sev_asid);
-                       return false;
-               }
-       } else if (!strcmp(ciphertext_hiding_asids, "max")) {
-               ciphertext_hiding_asid_nr = min_sev_asid - 1;
-       }
-
-       if (ciphertext_hiding_asid_nr) {
-               max_snp_asid = ciphertext_hiding_asid_nr;
+       if (!strcmp(ciphertext_hiding_asids, "max")) {
+               max_snp_asid = min_sev_asid - 1;
                 min_sev_es_asid = max_snp_asid + 1;
-               pr_info("SEV-SNP ciphertext hiding enabled\n");
-
                 return true;
         }

-invalid_parameter:
-       pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
-               ciphertext_hiding_asids);
-       return false;
+       /* Do sanity check on user-defined ciphertext_hiding_asids */
+       if (kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid) ||
+           !max_snp_asid || max_snp_asid >= min_sev_asid) {
+               pr_warn("invalid ciphertext_hiding_asids \"%s\" or !(0 < 
%u < minimum SEV ASID %u)\n",
+                       ciphertext_hiding_asids, max_snp_asid, 
min_sev_asid);
+               max_snp_asid = min_sev_asid - 1;
+               return false;
+       }
+
+       min_sev_es_asid = max_snp_asid + 1;
+
+       return true;
  }

  void __init sev_hardware_setup(void)
@@ -3122,8 +3112,10 @@ void __init sev_hardware_setup(void)
                  * ASID range into separate SEV-ES and SEV-SNP ASID 
ranges with
                  * the SEV-SNP ASID starting at 1.
                  */
-               if (check_and_enable_sev_snp_ciphertext_hiding())
+               if (check_and_enable_sev_snp_ciphertext_hiding()) {
+                       pr_info("SEV-SNP ciphertext hiding enabled\n");
                         init_args.max_snp_asid = max_snp_asid;
+               }
                 if (sev_platform_init(&init_args))
                         sev_supported = sev_es_supported = 
sev_snp_supported = false;
                 else if (sev_snp_supported)

Thanks,

Kim

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kalra, Ashish]

On 8/12/2025 11:45 AM, Kim Phillips wrote:
> On 8/12/25 9:40 AM, Kalra, Ashish wrote:
>> On 8/12/2025 7:06 AM, Kim Phillips wrote:
>>>   arch/x86/kvm/svm/sev.c | 47 ++++++++++++++++++-----------------------------
>>>   1 file changed, 18 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 7ac0f0f25e68..57c6e4717e51 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -2970,42 +2970,29 @@ static bool is_sev_snp_initialized(void)
>>>
>>>   static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>>>   {
>>> -       unsigned int ciphertext_hiding_asid_nr = 0;
>>> -
>>> -       if (!ciphertext_hiding_asids[0])
>>> -               return false;
>>> -
>>> -       if (!sev_is_snp_ciphertext_hiding_supported()) {
>>> +       if (ciphertext_hiding_asids[0] && !sev_is_snp_ciphertext_hiding_supported()) {
>>>                  pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
>>>                  return false;
>>>          }
>>>
>> This is incorrect, if ciphertext_hiding_asids module parameter is never specified, user will always
>> get a warning of an invalid ciphertext_hiding_asids module parameter.
>>
>> When this module parameter is optional why should the user get a warning about an invalid module parameter.
> 
> Ack, sorry, new diff below that fixes this.
> 
>> Again, why do we want to do all these checks below if this module parameter has not been specified by
>> the user ?
> 
> Not sure what you mean by 'below' here (assuming in the resulting code), but, in general, there are less checks with this diff than the original v7 code.
> 
>>> -       if (isdigit(ciphertext_hiding_asids[0])) {
>>> -               if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
>>> -                       goto invalid_parameter;
>>> -
>>> -               /* Do sanity check on user-defined ciphertext_hiding_asids */
>>> -               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>>> -                       pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
>>> -                               ciphertext_hiding_asid_nr, min_sev_asid);
>> A *combined* error message such as this:
>> "invalid ciphertext_hiding_asids XXX or !(0 < XXX < minimum SEV ASID 100)"
>>
>> is going to be really confusing to the user.
>>
>> It is much simpler for user to understand if the error/warning is:
>> "Module parameter ciphertext_hiding_asids XXX exceeds or equals minimum SEV ASID YYY"
>> OR
>> "Module parameter ciphertext_hiding_asids XXX invalid"
> 
> I tend to disagree. If, e.g., the user sets ciphertext_hiding_asids=100, they see:
> 
>      kvm_amd: invalid ciphertext_hiding_asids "100" or !(0 < 100 < minimum SEV ASID 100)
> 
> which the user can easily unmistakably and quickly deduce that the problem is the latter - not the former - condition that has the problem.
> 
> The original v7 code in that same case would emit:
> 
> kvm_amd: Module parameter ciphertext_hiding_asids (100) exceeds or equals minimum SEV ASID (100)
> 
> ...to which the user would ask themselves "What's wrong with equalling the minimum SEV ASID (100)"?

I disagree, the documentation mentions clearly that: 
For SEV-ES/SEV-SNP guests the maximum ASID available is MIN_SEV_ASID - 1.

Which the above message conveys quite clearly.

> 
> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).

> 
> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
> 
>      kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
> 
> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.

This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!

And how can user input of 0x1, result in max_snp_asid == 99 ?

This is the issue with combining the checks and emitting a combined error message:

Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information : 
!(0 < 99 < minimum SEV ASID 100)

The original message is much simpler to understand and correct too: 
Module parameter ciphertext_hiding_asids (-1) invalid

> 
> But all this is nothing compared to the added simplicity resulting from making the change to the original v7 code.

I disagree, combining checks and emitting a combined error message is going to be more confusing to the user as the above case of (ciphertext_hiding_asids=0x1) shows.

Thanks,
Ashish

> 
> New diff from original v7 below:
> 
>  arch/x86/kvm/svm/sev.c | 42 +++++++++++++++++-------------------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7ac0f0f25e68..a879ea5f53f2 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2970,8 +2970,6 @@ static bool is_sev_snp_initialized(void)
> 
>  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>  {
> -       unsigned int ciphertext_hiding_asid_nr = 0;
> -
>         if (!ciphertext_hiding_asids[0])
>                 return false;
> 
> @@ -2980,32 +2978,24 @@ static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>                 return false;
>         }
> 
> -       if (isdigit(ciphertext_hiding_asids[0])) {
> -               if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
> -                       goto invalid_parameter;
> -
> -               /* Do sanity check on user-defined ciphertext_hiding_asids */
> -               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> -                       pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
> -                               ciphertext_hiding_asid_nr, min_sev_asid);
> -                       return false;
> -               }
> -       } else if (!strcmp(ciphertext_hiding_asids, "max")) {
> -               ciphertext_hiding_asid_nr = min_sev_asid - 1;
> -       }
> -
> -       if (ciphertext_hiding_asid_nr) {
> -               max_snp_asid = ciphertext_hiding_asid_nr;
> +       if (!strcmp(ciphertext_hiding_asids, "max")) {
> +               max_snp_asid = min_sev_asid - 1;
>                 min_sev_es_asid = max_snp_asid + 1;
> -               pr_info("SEV-SNP ciphertext hiding enabled\n");
> -
>                 return true;
>         }
> 
> -invalid_parameter:
> -       pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> -               ciphertext_hiding_asids);
> -       return false;
> +       /* Do sanity check on user-defined ciphertext_hiding_asids */
> +       if (kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid) ||
> +           !max_snp_asid || max_snp_asid >= min_sev_asid) {
> +               pr_warn("invalid ciphertext_hiding_asids \"%s\" or !(0 < %u < minimum SEV ASID %u)\n",
> +                       ciphertext_hiding_asids, max_snp_asid, min_sev_asid);
> +               max_snp_asid = min_sev_asid - 1;
> +               return false;
> +       }
> +
> +       min_sev_es_asid = max_snp_asid + 1;
> +
> +       return true;
>  }
> 
>  void __init sev_hardware_setup(void)
> @@ -3122,8 +3112,10 @@ void __init sev_hardware_setup(void)
>                  * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
>                  * the SEV-SNP ASID starting at 1.
>                  */
> -               if (check_and_enable_sev_snp_ciphertext_hiding())
> +               if (check_and_enable_sev_snp_ciphertext_hiding()) {
> +                       pr_info("SEV-SNP ciphertext hiding enabled\n");
>                         init_args.max_snp_asid = max_snp_asid;
> +               }
>                 if (sev_platform_init(&init_args))
>                         sev_supported = sev_es_supported = sev_snp_supported = false;
>                 else if (sev_snp_supported)
> 
> Thanks,
> 
> Kim
> 

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kim Phillips]

On 8/12/25 1:29 PM, Kalra, Ashish wrote:
>
> On 8/12/2025 11:45 AM, Kim Phillips wrote:
>> On 8/12/25 9:40 AM, Kalra, Ashish wrote:
>>> On 8/12/2025 7:06 AM, Kim Phillips wrote:
>>>>    arch/x86/kvm/svm/sev.c | 47 ++++++++++++++++++-----------------------------
>>>>    1 file changed, 18 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>> index 7ac0f0f25e68..57c6e4717e51 100644
>>>> --- a/arch/x86/kvm/svm/sev.c
>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>> @@ -2970,42 +2970,29 @@ static bool is_sev_snp_initialized(void)
>>>>
>>>>    static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>>>>    {
>>>> -       unsigned int ciphertext_hiding_asid_nr = 0;
>>>> -
>>>> -       if (!ciphertext_hiding_asids[0])
>>>> -               return false;
>>>> -
>>>> -       if (!sev_is_snp_ciphertext_hiding_supported()) {
>>>> +       if (ciphertext_hiding_asids[0] && !sev_is_snp_ciphertext_hiding_supported()) {
>>>>                   pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
>>>>                   return false;
>>>>           }
>>>>
>>> This is incorrect, if ciphertext_hiding_asids module parameter is never specified, user will always
>>> get a warning of an invalid ciphertext_hiding_asids module parameter.
>>>
>>> When this module parameter is optional why should the user get a warning about an invalid module parameter.
>> Ack, sorry, new diff below that fixes this.
>>
>>> Again, why do we want to do all these checks below if this module parameter has not been specified by
>>> the user ?
>> Not sure what you mean by 'below' here (assuming in the resulting code), but, in general, there are less checks with this diff than the original v7 code.
>>
>>>> -       if (isdigit(ciphertext_hiding_asids[0])) {
>>>> -               if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
>>>> -                       goto invalid_parameter;
>>>> -
>>>> -               /* Do sanity check on user-defined ciphertext_hiding_asids */
>>>> -               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>>>> -                       pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
>>>> -                               ciphertext_hiding_asid_nr, min_sev_asid);
>>> A *combined* error message such as this:
>>> "invalid ciphertext_hiding_asids XXX or !(0 < XXX < minimum SEV ASID 100)"
>>>
>>> is going to be really confusing to the user.
>>>
>>> It is much simpler for user to understand if the error/warning is:
>>> "Module parameter ciphertext_hiding_asids XXX exceeds or equals minimum SEV ASID YYY"
>>> OR
>>> "Module parameter ciphertext_hiding_asids XXX invalid"
>> I tend to disagree. If, e.g., the user sets ciphertext_hiding_asids=100, they see:
>>
>>       kvm_amd: invalid ciphertext_hiding_asids "100" or !(0 < 100 < minimum SEV ASID 100)
>>
>> which the user can easily unmistakably and quickly deduce that the problem is the latter - not the former - condition that has the problem.
>>
>> The original v7 code in that same case would emit:
>>
>> kvm_amd: Module parameter ciphertext_hiding_asids (100) exceeds or equals minimum SEV ASID (100)
>>
>> ...to which the user would ask themselves "What's wrong with equalling the minimum SEV ASID (100)"?
> I disagree, the documentation mentions clearly that:
> For SEV-ES/SEV-SNP guests the maximum ASID available is MIN_SEV_ASID - 1.
>
> Which the above message conveys quite clearly.

The point of clear error messaging is to avoid the user having to 
(re-)read the documentation.

>
>> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).
>> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
>>
>>       kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>
>> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.
> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!

Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.

> And how can user input of 0x1, result in max_snp_asid == 99 ?

It doesn't, again, the 0x is the invalid part.

> This is the issue with combining the checks and emitting a combined error message:
>
> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information :
> !(0 < 99 < minimum SEV ASID 100)

It's not, it says it's *OR* that condition.

> The original message is much simpler to understand and correct too:
> Module parameter ciphertext_hiding_asids (-1) invalid

Which is wildly different from any possible derivation of 0x1.

>> But all this is nothing compared to the added simplicity resulting from making the change to the original v7 code.
> I disagree, combining checks and emitting a combined error message is going to be more confusing to the user as the above case of (ciphertext_hiding_asids=0x1) shows.

I don't, but nevertheless, it can still be differentiated and still be 
cleaner code than the original v7...

> Thanks,
> Ashish

Thanks,

Kim

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kalra, Ashish]

On 8/12/2025 1:40 PM, Kim Phillips wrote:

>>
>>> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).
>>> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
>>>
>>>       kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>>
>>> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.
>> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!
> 
> Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.
> 
>> And how can user input of 0x1, result in max_snp_asid == 99 ?
> 
> It doesn't, again, the 0x is the invalid part.
> 
>> This is the issue with combining the checks and emitting a combined error message:
>>
>> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information :
>> !(0 < 99 < minimum SEV ASID 100)
> 
> It's not, it says it's *OR* that condition.

To me this is wrong as 
!(0 < 99 < minimum SEV ASID 100) is simply not a correct statement!

Thanks,
Ashish 

> 
>> The original message is much simpler to understand and correct too:
>> Module parameter ciphertext_hiding_asids (-1) invalid
> 

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kim Phillips]

On 8/12/25 1:52 PM, Kalra, Ashish wrote:
>
> On 8/12/2025 1:40 PM, Kim Phillips wrote:
>
>>>> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).
>>>> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
>>>>
>>>>        kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>>>
>>>> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.
>>> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!
>> Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.
>>
>>> And how can user input of 0x1, result in max_snp_asid == 99 ?
>> It doesn't, again, the 0x is the invalid part.
>>
>>> This is the issue with combining the checks and emitting a combined error message:
>>>
>>> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information :
>>> !(0 < 99 < minimum SEV ASID 100)
>> It's not, it says it's *OR* that condition.
> To me this is wrong as
> !(0 < 99 < minimum SEV ASID 100) is simply not a correct statement!

The diff I provided emits exactly this:

kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)


which means *EITHER*:

invalid ciphertext_hiding_asids "0x1"

*OR*

!(0 < 99 < minimum SEV ASID 100)

but since the latter is 'true', the user is pointed to the former
"0x1" as being the interpretation problem.

Would adding the word "Either" help?:

kvm_amd: Either invalid ciphertext_hiding_asids "0x1", or !(0 < 99 < minimum SEV ASID 100)

?

If not, feel free to separate them: the code is still much cleaner.

Thanks,

Kim

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kalra, Ashish]

On 8/12/2025 2:11 PM, Kim Phillips wrote:
> 
> 
> On 8/12/25 1:52 PM, Kalra, Ashish wrote:
>>
>> On 8/12/2025 1:40 PM, Kim Phillips wrote:
>>
>>>>> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).
>>>>> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
>>>>>
>>>>>        kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>>>>
>>>>> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.
>>>> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!
>>> Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.
>>>
>>>> And how can user input of 0x1, result in max_snp_asid == 99 ?
>>> It doesn't, again, the 0x is the invalid part.
>>>
>>>> This is the issue with combining the checks and emitting a combined error message:
>>>>
>>>> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information :
>>>> !(0 < 99 < minimum SEV ASID 100)
>>> It's not, it says it's *OR* that condition.
>> To me this is wrong as
>> !(0 < 99 < minimum SEV ASID 100) is simply not a correct statement!
> 
> The diff I provided emits exactly this:
> 
> kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
> 
> 
> which means *EITHER*:
> 
> invalid ciphertext_hiding_asids "0x1"
> 
> *OR*
> 
> !(0 < 99 < minimum SEV ASID 100)
> 
> but since the latter is 'true', the user is pointed to the former
> "0x1" as being the interpretation problem.
> 
> Would adding the word "Either" help?:
> 
> kvm_amd: Either invalid ciphertext_hiding_asids "0x1", or !(0 < 99 < minimum SEV ASID 100)
> 
> ?

No, i simply won't put an invalid expression out there:

!(0 < 99 < minimum SEV ASID 100)

> 
> If not, feel free to separate them: the code is still much cleaner.
>

Separating the checks will make the code not very different from the original function, so i am going to keep the original code.

Thanks,
Ashish
 
> Thanks,
> 
> Kim
> 

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kim Phillips]

On 8/12/25 2:38 PM, Kalra, Ashish wrote:
> On 8/12/2025 2:11 PM, Kim Phillips wrote:
>> On 8/12/25 1:52 PM, Kalra, Ashish wrote:
>>> On 8/12/2025 1:40 PM, Kim Phillips wrote:
>>>>>> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).
>>>>>> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
>>>>>>
>>>>>>         kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>>>>>
>>>>>> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.
>>>>> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!
>>>> Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.
>>>>> And how can user input of 0x1, result in max_snp_asid == 99 ?
>>>> It doesn't, again, the 0x is the invalid part.
>>>>
>>>>> This is the issue with combining the checks and emitting a combined error message:
>>>>>
>>>>> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information :
>>>>> !(0 < 99 < minimum SEV ASID 100)
>>>> It's not, it says it's *OR* that condition.
>>> To me this is wrong as
>>> !(0 < 99 < minimum SEV ASID 100) is simply not a correct statement!
>> The diff I provided emits exactly this:
>>
>> kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>
>>
>> which means *EITHER*:
>>
>> invalid ciphertext_hiding_asids "0x1"
>>
>> *OR*
>>
>> !(0 < 99 < minimum SEV ASID 100)
>>
>> but since the latter is 'true', the user is pointed to the former
>> "0x1" as being the interpretation problem.
>>
>> Would adding the word "Either" help?:
>>
>> kvm_amd: Either invalid ciphertext_hiding_asids "0x1", or !(0 < 99 < minimum SEV ASID 100)
>>
>> ?
> No, i simply won't put an invalid expression out there:
>
> !(0 < 99 < minimum SEV ASID 100)

When not quoted out of context, it's not an invalid expression (in the 
99 case) because it's preceded with the word "or:"

..., or !(0 < 99 < minimum SEV ASID 100)

>> If not, feel free to separate them: the code is still much cleaner.
> Separating the checks will make the code not very different from the original function, so i am going to keep the original code.

Take a look at the example diff below, then.  It's still less, simpler 
code because it eliminates:

1. the unnecessary ciphertext_hiding_asid_nr variable

2. the redundant isdigit(ciphertext_hiding_asids[0])) check
and 3. the 'invalid_parameter:' label referenced by only one goto 
statement. Thanks, Kim arch/x86/kvm/svm/sev.c | 44 
++++++++++++++++++++------------------------ 1 file changed, 20 
insertions(+), 24 deletions(-) diff --git a/arch/x86/kvm/svm/sev.c 
b/arch/x86/kvm/svm/sev.c index 7ac0f0f25e68..d0a13f1b0572 100644 --- 
a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2970,8 +2970,6 
@@ static bool is_sev_snp_initialized(void) static bool 
check_and_enable_sev_snp_ciphertext_hiding(void) { - unsigned int 
ciphertext_hiding_asid_nr = 0; - if (!ciphertext_hiding_asids[0]) return 
false; @@ -2980,32 +2978,28 @@ static bool 
check_and_enable_sev_snp_ciphertext_hiding(void) return false; } - if 
(isdigit(ciphertext_hiding_asids[0])) { - if 
(kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) - 
goto invalid_parameter; - - /* Do sanity check on user-defined 
ciphertext_hiding_asids */ - if (ciphertext_hiding_asid_nr >= 
min_sev_asid) { - pr_warn("Module parameter ciphertext_hiding_asids (%u) 
exceeds or equals minimum SEV ASID (%u)\n", - ciphertext_hiding_asid_nr, 
min_sev_asid); - return false; - } - } else if 
(!strcmp(ciphertext_hiding_asids, "max")) { - ciphertext_hiding_asid_nr 
= min_sev_asid - 1; + if (!strcmp(ciphertext_hiding_asids, "max")) { + 
max_snp_asid = min_sev_asid - 1; + min_sev_es_asid = max_snp_asid + 1; + 
return true; } - if (ciphertext_hiding_asid_nr) { - max_snp_asid = 
ciphertext_hiding_asid_nr; - min_sev_es_asid = max_snp_asid + 1; - 
pr_info("SEV-SNP ciphertext hiding enabled\n"); + if 
(kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid)) { + 
pr_warn("ciphertext_hiding_asids \"%s\" is not an integer\n", 
ciphertext_hiding_asids); + return false; + } - return true; + /* Do 
sanity check on user-defined ciphertext_hiding_asids */ + if 
(max_snp_asid < 1 || max_snp_asid >= min_sev_asid) { + pr_warn("!(0 < 
ciphertext_hiding_asids %u < minimum SEV ASID %u)\n", + max_snp_asid, 
min_sev_asid); + max_snp_asid = min_sev_asid - 1; + return false; } 
-invalid_parameter: - pr_warn("Module parameter ciphertext_hiding_asids 
(%s) invalid\n", - ciphertext_hiding_asids); - return false; + 
min_sev_es_asid = max_snp_asid + 1; + + return true; } void __init 
sev_hardware_setup(void) @@ -3122,8 +3116,10 @@ void __init 
sev_hardware_setup(void) * ASID range into separate SEV-ES and SEV-SNP 
ASID ranges with * the SEV-SNP ASID starting at 1. */ - if 
(check_and_enable_sev_snp_ciphertext_hiding()) + if 
(check_and_enable_sev_snp_ciphertext_hiding()) { + pr_info("SEV-SNP 
ciphertext hiding enabled\n"); init_args.max_snp_asid = max_snp_asid; + 
} if (sev_platform_init(&init_args)) sev_supported = sev_es_supported = 
sev_snp_supported = false; else if (sev_snp_supported)

Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kim Phillips]

On 8/12/25 6:30 PM, Kim Phillips wrote:
> On 8/12/25 2:38 PM, Kalra, Ashish wrote:
>> On 8/12/2025 2:11 PM, Kim Phillips wrote:
>>> On 8/12/25 1:52 PM, Kalra, Ashish wrote:
>>>> On 8/12/2025 1:40 PM, Kim Phillips wrote:
>>>>>>> It's not as immediately obvious that it needs to (0 < x < 
>>>>>>> minimum SEV ASID 100).
>>>>>>> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now 
>>>>>>> see:
>>>>>>>
>>>>>>>         kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 
>>>>>>> 99 < minimum SEV ASID 100)
>>>>>>>
>>>>>>> which - unlike the original v7 code - shows the user that the 
>>>>>>> '0x1' was not interpreted as a number at all: thus the 99 in the 
>>>>>>> latter condition.
>>>>>> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid 
>>>>>> condition!
>>>>> Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.
>>>>>> And how can user input of 0x1, result in max_snp_asid == 99 ?
>>>>> It doesn't, again, the 0x is the invalid part.
>>>>>
>>>>>> This is the issue with combining the checks and emitting a 
>>>>>> combined error message:
>>>>>>
>>>>>> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid 
>>>>>> remains set to 99 and then the combined error conveys a wrong 
>>>>>> information :
>>>>>> !(0 < 99 < minimum SEV ASID 100)
>>>>> It's not, it says it's *OR* that condition.
>>>> To me this is wrong as
>>>> !(0 < 99 < minimum SEV ASID 100) is simply not a correct statement!
>>> The diff I provided emits exactly this:
>>>
>>> kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum 
>>> SEV ASID 100)
>>>
>>>
>>> which means *EITHER*:
>>>
>>> invalid ciphertext_hiding_asids "0x1"
>>>
>>> *OR*
>>>
>>> !(0 < 99 < minimum SEV ASID 100)
>>>
>>> but since the latter is 'true', the user is pointed to the former
>>> "0x1" as being the interpretation problem.
>>>
>>> Would adding the word "Either" help?:
>>>
>>> kvm_amd: Either invalid ciphertext_hiding_asids "0x1", or !(0 < 99 < 
>>> minimum SEV ASID 100)
>>>
>>> ?
>> No, i simply won't put an invalid expression out there:
>>
>> !(0 < 99 < minimum SEV ASID 100)
>
> When not quoted out of context, it's not an invalid expression (in the 
> 99 case) because it's preceded with the word "or:"
>
> ..., or !(0 < 99 < minimum SEV ASID 100)
>
>>> If not, feel free to separate them: the code is still much cleaner.
>> Separating the checks will make the code not very different from the 
>> original function, so i am going to keep the original code.
>
> Take a look at the example diff below, then.  It's still less, simpler 
> code because it eliminates:
>
> 1. the unnecessary ciphertext_hiding_asid_nr variable
>
> 2. the redundant isdigit(ciphertext_hiding_asids[0])) check
> and 3. the 'invalid_parameter:' label referenced by only one goto 
> statement. 

Re-posting, since I believe the previous email's diff got mangled:

  arch/x86/kvm/svm/sev.c | 44 ++++++++++++++++++++------------------------
  1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7ac0f0f25e68..1b9702500c73 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2970,8 +2970,6 @@ static bool is_sev_snp_initialized(void)

  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
  {
-       unsigned int ciphertext_hiding_asid_nr = 0;
-
         if (!ciphertext_hiding_asids[0])
                 return false;

@@ -2980,32 +2978,28 @@ static bool 
check_and_enable_sev_snp_ciphertext_hiding(void)
                 return false;
         }

-       if (isdigit(ciphertext_hiding_asids[0])) {
-               if (kstrtoint(ciphertext_hiding_asids, 10, 
&ciphertext_hiding_asid_nr))
-                       goto invalid_parameter;
-
-               /* Do sanity check on user-defined 
ciphertext_hiding_asids */
-               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
-                       pr_warn("Module parameter 
ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
-                               ciphertext_hiding_asid_nr, min_sev_asid);
-                       return false;
-               }
-       } else if (!strcmp(ciphertext_hiding_asids, "max")) {
-               ciphertext_hiding_asid_nr = min_sev_asid - 1;
+       if (!strcmp(ciphertext_hiding_asids, "max")) {
+               max_snp_asid = min_sev_asid - 1;
+               min_sev_es_asid = max_snp_asid + 1;
+               return true;
         }

-       if (ciphertext_hiding_asid_nr) {
-               max_snp_asid = ciphertext_hiding_asid_nr;
-               min_sev_es_asid = max_snp_asid + 1;
-               pr_info("SEV-SNP ciphertext hiding enabled\n");
+       if (kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid)) {
+               pr_warn("ciphertext_hiding_asids \"%s\" is not an 
integer or 'max'\n", ciphertext_hiding_asids);
+               return false;
+       }

-               return true;
+       /* Do sanity check on user-defined ciphertext_hiding_asids */
+       if (max_snp_asid < 1 || max_snp_asid >= min_sev_asid) {
+               pr_warn("!(0 < ciphertext_hiding_asids %u < minimum SEV 
ASID %u)\n",
+                       max_snp_asid, min_sev_asid);
+               max_snp_asid = min_sev_asid - 1;
+               return false;
         }

-invalid_parameter:
-       pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
-               ciphertext_hiding_asids);
-       return false;
+       min_sev_es_asid = max_snp_asid + 1;
+
+       return true;
  }

  void __init sev_hardware_setup(void)
@@ -3122,8 +3116,10 @@ void __init sev_hardware_setup(void)
                  * ASID range into separate SEV-ES and SEV-SNP ASID 
ranges with
                  * the SEV-SNP ASID starting at 1.
                  */
-               if (check_and_enable_sev_snp_ciphertext_hiding())
+               if (check_and_enable_sev_snp_ciphertext_hiding()) {
+                       pr_info("SEV-SNP ciphertext hiding enabled\n");
                         init_args.max_snp_asid = max_snp_asid;
+               }
                 if (sev_platform_init(&init_args))
                         sev_supported = sev_es_supported = 
sev_snp_supported = false;
                 else if (sev_snp_supported)

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Herbert Xu]

On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
> Hi Herbert, can you please merge patches 1-5.
> 
> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
> for patches 6 & 7.

These patches will be at the base of the cryptodev tree for 6.17
so it could be pulled into another tree without any risks.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Herbert Xu]

On Mon, Jul 21, 2025 at 02:12:15PM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Ciphertext hiding prevents host accesses from reading the ciphertext
> of SNP guest private memory. Instead of reading ciphertext, the host
> will see constant default values (0xff).
> 
> The SEV ASID space is split into SEV and SEV-ES/SNP ASID ranges.
> Enabling ciphertext hiding further splits the SEV-ES/SEV-SNP ASID space
> into separate ASID ranges for SEV-ES and SEV-SNP guests.
> 
> Add new module parameter to the KVM module to enable ciphertext hiding
> support and a user configurable system-wide maximum SNP ASID value. If
> the module parameter value is "max" then the complete SEV-ES/SEV-SNP
> space is allocated to SEV-SNP guests.
> 
> v7:
> - Fix comments.
> - Move the check for module parameter ciphertext_hiding_asids inside
> check_and_enable_sev_snp_ciphertext_hiding(), this keeps all the logic
> related to the parameter in a single function.
> 
> v6:
> - Fix module parameter ciphertext_hiding_asids=0 case.
> - Coalesce multiple cases of handling invalid module parameter
> ciphertext_hiding_asids into a single branch/label.
> - Fix commit logs.
> - Fix Documentation.
> 
> v5:
> - Add pre-patch to cache SEV platform status and use this cached
> information to set api_major/api_minor/build.
> - Since the SEV platform status and SNP platform status differ, 
> remove the state field from sev_device structure and instead track
> SEV platform state from cached SEV platform status.
> - If SNP is enabled then cached SNP platform status is used for 
> api_major/api_minor/build.
> - Fix using sev_do_cmd() instead of __sev_do_cmd_locked().
> - Fix commit logs.
> - Fix kernel-parameters documentation. 
> - Modify KVM module parameter to enable CipherTextHiding to support
> "max" option to allow complete SEV-ES+ ASID space to be allocated
> to SEV-SNP guests.
> - Do not enable ciphertext hiding if module parameter to specify
> maximum SNP ASID is invalid.
> 
> v4:
> - Fix buffer allocation for SNP_FEATURE_INFO command to correctly
> handle page boundary check requirements.
> - Return correct length for SNP_FEATURE_INFO command from
> sev_cmd_buffer_len().
> - Switch to using SNP platform status instead of SEV platform status if
> SNP is enabled and cache SNP platform status and feature information.
> Modify sev_get_api_version() accordingly.
> - Fix commit logs.
> - Expand the comments on why both the feature info and the platform
> status fields have to be checked for CipherTextHiding feature 
> detection and enablement.
> - Add new preperation patch for CipherTextHiding feature which
> introduces new {min,max}_{sev_es,snp}_asid variables along with
> existing {min,max}_sev_asid variable to simplify partitioning of the
> SEV and SEV-ES+ ASID space.
> - Switch to single KVM module parameter to enable CipherTextHiding
> feature and the maximum SNP ASID usable for SNP guests when 
> CipherTextHiding feature is enabled.
> 
> v3:
> - rebase to linux-next.
> - rebase on top of support to move SEV-SNP initialization to
> KVM module from CCP driver.
> - Split CipherTextHiding support between CCP driver and KVM module
> with KVM module calling into CCP driver to initialize SNP with
> CipherTextHiding enabled and MAX ASID usable for SNP guest if
> KVM is enabling CipherTextHiding feature.
> - Move module parameters to enable CipherTextHiding feature and
> MAX ASID usable for SNP guests from CCP driver to KVM module
> which allows KVM to be responsible for enabling CipherTextHiding
> feature if end-user requests it.
> 
> v2:
> - Fix and add more description to commit logs.
> - Rename sev_cache_snp_platform_status_and_discover_features() to 
> snp_get_platform_data().
> - Add check in snp_get_platform_data to guard against being called
> after SNP_INIT_EX.
> - Fix comments for new structure field definitions being added.
> - Fix naming for new structure being added.
> - Add new vm-type parameter to sev_asid_new().
> - Fix identation.
> - Rename CCP module parameters psp_cth_enabled to cipher_text_hiding and 
> psp_max_snp_asid to max_snp_asid.
> - Rename max_snp_asid to snp_max_snp_asid. 
> 
> Ashish Kalra (7):
>   crypto: ccp - New bit-field definitions for SNP_PLATFORM_STATUS
>     command
>   crypto: ccp - Cache SEV platform status and platform state
>   crypto: ccp - Add support for SNP_FEATURE_INFO command
>   crypto: ccp - Introduce new API interface to indicate SEV-SNP
>     Ciphertext hiding feature
>   crypto: ccp - Add support to enable CipherTextHiding on SNP_INIT_EX
>   KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables
>   KVM: SEV: Add SEV-SNP CipherTextHiding support
> 
>  .../admin-guide/kernel-parameters.txt         |  18 +++
>  arch/x86/kvm/svm/sev.c                        |  96 +++++++++++--
>  drivers/crypto/ccp/sev-dev.c                  | 127 ++++++++++++++++--
>  drivers/crypto/ccp/sev-dev.h                  |   6 +-
>  include/linux/psp-sev.h                       |  44 +++++-
>  include/uapi/linux/psp-sev.h                  |  10 +-
>  6 files changed, 274 insertions(+), 27 deletions(-)
> 
> -- 
> 2.34.1

Patches 1-5 applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Kalra, Ashish]

On 8/16/2025 3:39 AM, Herbert Xu wrote:
> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>> Hi Herbert, can you please merge patches 1-5.
>>
>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>> for patches 6 & 7.
> 
> These patches will be at the base of the cryptodev tree for 6.17
> so it could be pulled into another tree without any risks.
> 
> Cheers,

Thanks Herbert for pulling in patches 1-5.

Paolo, can you please merge patches 6 and 7 into the KVM tree.

Thanks,
Ashish

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Kim Phillips]

On 8/18/25 2:16 PM, Kalra, Ashish wrote:
> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>> Hi Herbert, can you please merge patches 1-5.
>>>
>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>> for patches 6 & 7.
>> These patches will be at the base of the cryptodev tree for 6.17
>> so it could be pulled into another tree without any risks.
>>
>> Cheers,
> Thanks Herbert for pulling in patches 1-5.
>
> Paolo, can you please merge patches 6 and 7 into the KVM tree.
Hi Ashish,

I have pending comments on patch 7:

https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/

If still not welcome, can you say why you think:

1. The ciphertext_hiding_asid_nr variable is necessary

2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's 
immediately followed by kstrtoint which effectively makes the open-coded 
isdigit check  redundant?

3. Why the 'invalid_parameter:' label referenced by only one goto 
statement can't be folded and removed.

Thanks,

Kim

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Kalra, Ashish]

On 8/18/2025 2:38 PM, Kim Phillips wrote:
> On 8/18/25 2:16 PM, Kalra, Ashish wrote:
>> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>>> Hi Herbert, can you please merge patches 1-5.
>>>>
>>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>>> for patches 6 & 7.
>>> These patches will be at the base of the cryptodev tree for 6.17
>>> so it could be pulled into another tree without any risks.
>>>
>>> Cheers,
>> Thanks Herbert for pulling in patches 1-5.
>>
>> Paolo, can you please merge patches 6 and 7 into the KVM tree.
> Hi Ashish,
> 
> I have pending comments on patch 7:
> 
> https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/
> 
> If still not welcome, can you say why you think:
> 
> 1. The ciphertext_hiding_asid_nr variable is necessary

I prefer safe coding, and i don't want to update max_snp_asid, until i am sure there are no sanity 
check failures and that's why i prefer using a *temp* variable and then updating max_snp_asid when i
am sure all sanity checks have been done.

Otherwise, in your case you are updating max_snp_asid and then rolling it back in case of sanity check
failures, i don't like that. 

> 
> 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check  redundant?

isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid
calling kstrtoint() if the parameter is not a number.

> 
> 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed.

This is for understandable code flow :

1). Check module parameter is set by user.
2). Check ciphertext_hiding_feature enabled.
3). Check if parameter is numeric.
    Sanity checks on numeric parameter
    If checks fail goto invalid_parameter
4). Check if parameter is the string "max".
5). Set max_snp_asid and min_sev_es_asid. 
6). Fall-through to invalid parameter.
invalid_parameter: 

This is overall a more understandable code flow.

Again, this is just a module parameter checking function and not something which will affect runtime performance by eliminating a single temporary variable or jump label.

Thanks,
Ashish

> 
> Thanks,
> 
> Kim

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Kim Phillips]

On 8/18/25 3:39 PM, Kalra, Ashish wrote:
> On 8/18/2025 2:38 PM, Kim Phillips wrote:
>> On 8/18/25 2:16 PM, Kalra, Ashish wrote:
>>> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>>>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>>>> Hi Herbert, can you please merge patches 1-5.
>>>>>
>>>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>>>> for patches 6 & 7.
>>>> These patches will be at the base of the cryptodev tree for 6.17
>>>> so it could be pulled into another tree without any risks.
>>>>
>>>> Cheers,
>>> Thanks Herbert for pulling in patches 1-5.
>>>
>>> Paolo, can you please merge patches 6 and 7 into the KVM tree.
>> Hi Ashish,
>>
>> I have pending comments on patch 7:
>>
>> https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/
>>
>> If still not welcome, can you say why you think:
>>
>> 1. The ciphertext_hiding_asid_nr variable is necessary
> I prefer safe coding, and i don't want to update max_snp_asid, until i am sure there are no sanity
> check failures and that's why i prefer using a *temp* variable and then updating max_snp_asid when i
> am sure all sanity checks have been done.
>
> Otherwise, in your case you are updating max_snp_asid and then rolling it back in case of sanity check
> failures, i don't like that.

Item (1):

The rollback is in a single place, and the extra variable's existence 
can be avoided, or, at least have 'temp' somewhere in its name.

FWIW, any "Safe coding" practices should have been performed prior to 
the submission of the patch, IMO.

>> 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check  redundant?
> isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid
> calling kstrtoint() if the parameter is not a number.

Item (2):

This is module initialization code, it's better optimized for 
readability than for performance.  As a reader of the code, I'm 
constantly wondering why the redundancy exists, and am sure it is made 
objectively easier to read if the isdigit() check were removed.

>> 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed.
> This is for understandable code flow :
>
> 1). Check module parameter is set by user.
> 2). Check ciphertext_hiding_feature enabled.
> 3). Check if parameter is numeric.
>      Sanity checks on numeric parameter
>      If checks fail goto invalid_parameter
> 4). Check if parameter is the string "max".
> 5). Set max_snp_asid and min_sev_es_asid.
> 6). Fall-through to invalid parameter.
> invalid_parameter:
>
> This is overall a more understandable code flow.

Item (3):

That's not how your original v7 flows, but I do now see the non-obvious 
fall-through from the else if (...'max'...).  I believe I am not alone 
in missing that, and that a comment would have helped. Also, the 'else' 
isn't required

Flow readability-wise, comparing the two, after the two common if()s, 
your original v7 goes:

{
...
     if (isdigit() {
         if (kstrtoint())
             goto invalid_parameter
         if (temporary variable >= min_sev_asid) {
             pr_warn()
             return false;
     } else if (..."max"...) {
         temporary variable = ...
         /* non-obvious fallthrough to invalid_parameter iff 
temporary_variable == 0 */
     }

     if (temporary variable) {
         max_snp_asid = ...
         min_sev_es_asid = ...
         pr_info(..."enabled"...)
         return true;
     }

invalid_parameter:
     pr_warn()
     return false;
}

vs the result of my latest diff:

{
...
     if (..."max"...) {
         max_snp_asid =
         min_sev_es_asid = ...
         return true;
     }

     if (kstrtoint()) {
         pr_warn()
         return false
     }

     if (max_snp_asid < 1 || >= min_sev_asid) {
         pr_warn()
         max_snp_asid = /* rollback */
         return false;
     }

     min_sev_es_asid = ...

     return true;
}

So, just from an outright flow perspective, I believe my latest diff is 
objectively easier to follow.

> Again, this is just a module parameter checking function and not something which will affect runtime performance by eliminating a single temporary variable or jump label.
With this statement, you self-contradict your rationale to keep your 
version of the above to Item (2): "isdigit() is a MACRO compared to 
kstrtoint() call, it is more optimal to do an inline check and avoid 
calling kstrtoint() if the parameter is not a number". If not willing to 
take my latest diff as-is, I really would like to see:

Item (1)'s variable get a temporary-sounding name,
item (2)'s the isdigit() check (and thus a whole indentation level) 
removed, and
item (3)'s flow reconsidered given the (IMO objective) readability 
enhancement.

Thanks,

Kim

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Kalra, Ashish]

On 8/18/2025 6:23 PM, Kim Phillips wrote:
> On 8/18/25 3:39 PM, Kalra, Ashish wrote:
>> On 8/18/2025 2:38 PM, Kim Phillips wrote:
>>> On 8/18/25 2:16 PM, Kalra, Ashish wrote:
>>>> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>>>>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>>>>> Hi Herbert, can you please merge patches 1-5.
>>>>>>
>>>>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>>>>> for patches 6 & 7.
>>>>> These patches will be at the base of the cryptodev tree for 6.17
>>>>> so it could be pulled into another tree without any risks.
>>>>>
>>>>> Cheers,
>>>> Thanks Herbert for pulling in patches 1-5.
>>>>
>>>> Paolo, can you please merge patches 6 and 7 into the KVM tree.
>>> Hi Ashish,
>>>
>>> I have pending comments on patch 7:
>>>
>>> https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/
>>>
>>> If still not welcome, can you say why you think:
>>>
>>> 1. The ciphertext_hiding_asid_nr variable is necessary
>> I prefer safe coding, and i don't want to update max_snp_asid, until i am sure there are no sanity
>> check failures and that's why i prefer using a *temp* variable and then updating max_snp_asid when i
>> am sure all sanity checks have been done.
>>
>> Otherwise, in your case you are updating max_snp_asid and then rolling it back in case of sanity check
>> failures, i don't like that.
> 
> Item (1):
> 
> The rollback is in a single place, and the extra variable's existence can be avoided, or, at least have 'temp' somewhere in its name.
> 
> FWIW, any "Safe coding" practices should have been performed prior to the submission of the patch, IMO.
> 
>>> 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check  redundant?
>> isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid
>> calling kstrtoint() if the parameter is not a number.
> 
> Item (2):
> 
> This is module initialization code, it's better optimized for readability than for performance.  As a reader of the code, I'm constantly wondering why the redundancy exists, and am sure it is made objectively easier to read if the isdigit() check were removed.
> 
>>> 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed.
>> This is for understandable code flow :
>>
>> 1). Check module parameter is set by user.
>> 2). Check ciphertext_hiding_feature enabled.
>> 3). Check if parameter is numeric.
>>      Sanity checks on numeric parameter
>>      If checks fail goto invalid_parameter
>> 4). Check if parameter is the string "max".
>> 5). Set max_snp_asid and min_sev_es_asid.
>> 6). Fall-through to invalid parameter.
>> invalid_parameter:
>>
>> This is overall a more understandable code flow.
> 
> Item (3):
> 
> That's not how your original v7 flows, but I do now see the non-obvious fall-through from the else if (...'max'...).  I believe I am not alone in missing that, and that a comment would have helped. Also, the 'else' isn't required
> 
> Flow readability-wise, comparing the two, after the two common if()s, your original v7 goes:
> 
> {
> ...
>     if (isdigit() {
>         if (kstrtoint())
>             goto invalid_parameter
>         if (temporary variable >= min_sev_asid) {
>             pr_warn()
>             return false;
>     } else if (..."max"...) {
>         temporary variable = ...
>         /* non-obvious fallthrough to invalid_parameter iff temporary_variable == 0 */
>     }
> 
>     if (temporary variable) {
>         max_snp_asid = ...
>         min_sev_es_asid = ...
>         pr_info(..."enabled"...)
>         return true;
>     }
> 
> invalid_parameter:
>     pr_warn()
>     return false;
> }
> 
> vs the result of my latest diff:
> 
> {
> ...
>     if (..."max"...) {
>         max_snp_asid =
>         min_sev_es_asid = ...
>         return true;
>     }
> 
>     if (kstrtoint()) {
>         pr_warn()
>         return false
>     }
> 
>     if (max_snp_asid < 1 || >= min_sev_asid) {
>         pr_warn()
>         max_snp_asid = /* rollback */
>         return false;
>     }
> 
>     min_sev_es_asid = ...
> 
>     return true;
> }
> 
> So, just from an outright flow perspective, I believe my latest diff is objectively easier to follow.
> 

All the above are your opinions and i don't agree with them and my patch is bug-free and simple and easy to understand and works perfectly, and i am not making any more changes to it.

Thanks,
Ashish

>> Again, this is just a module parameter checking function and not something which will affect runtime performance by eliminating a single temporary variable or jump label.
> With this statement, you self-contradict your rationale to keep your version of the above to Item (2): "isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid calling kstrtoint() if the parameter is not a number". If not willing to take my latest diff as-is, I really would like to see:
> 
> Item (1)'s variable get a temporary-sounding name,
> item (2)'s the isdigit() check (and thus a whole indentation level) removed, and
> item (3)'s flow reconsidered given the (IMO objective) readability enhancement.
>
> Thanks,
> 
> Kim
> 

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Borislav Petkov]

On Mon, Aug 18, 2025 at 02:38:38PM -0500, Kim Phillips wrote:
> I have pending comments on patch 7:

If you're so hell-bent on doing your improvements on-top or aside of them, you
take his patch, add your stuff ontop or rewrite it, test it and then you send
it out and say why yours is better.

Then the maintainer decides.

There's no need to debate ad absurdum - you simply offer your idea and the
maintainer decides which one is better. As it has always been done.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Sean Christopherson]

On Tue, Aug 19, 2025, Borislav Petkov wrote:
> On Mon, Aug 18, 2025 at 02:38:38PM -0500, Kim Phillips wrote:
> > I have pending comments on patch 7:
> 
> If you're so hell-bent on doing your improvements on-top or aside of them, you
> take his patch, add your stuff ontop or rewrite it, test it and then you send
> it out and say why yours is better.
> 
> Then the maintainer decides.
> 
> There's no need to debate ad absurdum - you simply offer your idea and the
> maintainer decides which one is better. As it has always been done.

Or, the maintainer says "huh!?" and goes with option C.

Why take a string in the first place?  Just use '-1' as "max/auto".

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Kalra, Ashish]

Hello Sean,

On 8/19/2025 7:05 PM, Sean Christopherson wrote:
> On Tue, Aug 19, 2025, Borislav Petkov wrote:
>> On Mon, Aug 18, 2025 at 02:38:38PM -0500, Kim Phillips wrote:
>>> I have pending comments on patch 7:
>>
>> If you're so hell-bent on doing your improvements on-top or aside of them, you
>> take his patch, add your stuff ontop or rewrite it, test it and then you send
>> it out and say why yours is better.
>>
>> Then the maintainer decides.
>>
>> There's no need to debate ad absurdum - you simply offer your idea and the
>> maintainer decides which one is better. As it has always been done.
> 
> Or, the maintainer says "huh!?" and goes with option C.
> 
> Why take a string in the first place?  Just use '-1' as "max/auto".

It's just that there was general feedback to use a string like "max".

But as a maintainer if you are suggesting to use '-1' as "max/auto", i can do that.

Thanks,
Ashish

Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

[Sean Christopherson]

On Tue, Aug 19, 2025, Ashish Kalra wrote:
> Hello Sean,
> 
> On 8/19/2025 7:05 PM, Sean Christopherson wrote:
> > On Tue, Aug 19, 2025, Borislav Petkov wrote:
> >> On Mon, Aug 18, 2025 at 02:38:38PM -0500, Kim Phillips wrote:
> >>> I have pending comments on patch 7:
> >>
> >> If you're so hell-bent on doing your improvements on-top or aside of them, you
> >> take his patch, add your stuff ontop or rewrite it, test it and then you send
> >> it out and say why yours is better.
> >>
> >> Then the maintainer decides.
> >>
> >> There's no need to debate ad absurdum - you simply offer your idea and the
> >> maintainer decides which one is better. As it has always been done.
> > 
> > Or, the maintainer says "huh!?" and goes with option C.
> > 
> > Why take a string in the first place?  Just use '-1' as "max/auto".
> 
> It's just that there was general feedback to use a string like "max".

From who?  There's definitely value in providing/using human-friendly names for
things like this, but that needs to be weighed against the cost and complexity
in the kernel.  And the cost+complexity is quite high:

> +static bool check_and_enable_sev_snp_ciphertext_hiding(void)
> +{
> +	unsigned int ciphertext_hiding_asid_nr = 0;
> +
> +	if (!ciphertext_hiding_asids[0])
> +		return false;
> +
> +	if (!sev_is_snp_ciphertext_hiding_supported()) {
> +		pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");

This will print a spurious message if the admin explicitly loading KVM with
ciphertext_hiding_asids=0.

> +		return false;
> +	}
> +
> +	if (isdigit(ciphertext_hiding_asids[0])) {
> +		if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
> +			goto invalid_parameter;
> +
> +		/* Do sanity check on user-defined ciphertext_hiding_asids */
> +		if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> +			pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
> +				ciphertext_hiding_asid_nr, min_sev_asid);
> +			return false;

This is unfortunate and probably unexpected behavior, because if the admin
provided a super large value, odds are good they would make SEV-ES unusable than
disable ciphertext hiding.

> +		}
> +	} else if (!strcmp(ciphertext_hiding_asids, "max")) {
> +		ciphertext_hiding_asid_nr = min_sev_asid - 1;

The actual resolved value isn't captured in the module param.  

> +	}
> +
> +	if (ciphertext_hiding_asid_nr) {
> +		max_snp_asid = ciphertext_hiding_asid_nr;
> +		min_sev_es_asid = max_snp_asid + 1;
> +		pr_info("SEV-SNP ciphertext hiding enabled\n");
> +
> +		return true;
> +	}

This will fallthrough on ciphertext_hiding_asids=0 as well and yell about '0'
being invalid.

> +
> +invalid_parameter:
> +	pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> +		ciphertext_hiding_asids);
> +	return false;
> +}

> But as a maintainer if you are suggesting to use '-1' as "max/auto", i can do
> that.

Looking at this again, I don't see any reason to special case -1.  Just make the
param a uint and cap it at that maximum possible value.  As above, disabling
ciphertext hiding if the number of request SNP ASIDs is higher than what hardware
supports is probably not want the admin wants.

Compile tested only.

--
From: Ashish Kalra <ashish.kalra@amd.com>
Date: Mon, 21 Jul 2025 14:14:34 +0000
Subject: [PATCH] KVM: SEV: Add SEV-SNP CipherTextHiding support

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

The SEV ASID space is split into SEV and SEV-ES/SEV-SNP ASID ranges.
Enabling ciphertext hiding further splits the SEV-ES/SEV-SNP ASID space
into separate ASID ranges for SEV-ES and SEV-SNP guests.

Add a new off-by-default kvm-amd module parameter enable ciphertext
hiding and allow the admin to configure the SEV-ES and SEV-SNP ASID
ranges.  Simply cap the maximum SEV-SNP ASID as appropriate, i.e. don't
reject loading KVM or disable ciphertest hiding for a too-big value, as
KVM's general approach for module params is to sanitize inputs based on
hardware/kernel support, not burn the world down.  This also allows the
admin to use -1u to assign all SEV-ES/SNP ASIDs to SNP without needing
dedicated handling in KVM.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../admin-guide/kernel-parameters.txt         | 19 +++++++++++
 arch/x86/kvm/svm/sev.c                        | 32 ++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 747a55abf494..f4735931661e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2957,6 +2957,25 @@
 			(enabled). Disable by KVM if hardware lacks support
 			for NPT.
 
+	kvm-amd.ciphertext_hiding_asids=
+			[KVM,AMD] Ciphertext hiding prevents disallowed accesses
+			to SNP private memory from reading ciphertext.  Instead,
+			reads will see constant default values (0xff).
+
+			If ciphertext hiding is enabled, the joint SEV-ES+SEV-SNP
+			ASID space is paritioned separate SEV-ES and SEV-SNP ASID
+			ranges, with the SEV-SNP ASID range starting at 1.  For
+			SEV-ES/SEV-SNP guests the maximum ASID is MIN_SEV_ASID-1,
+			where MIN_SEV_ASID value is discovered by CPUID
+			Fn8000_001F[EDX].
+
+			A non-zero value enables SEV-SNP ciphertext hiding and
+			adjusts the ASID ranges for SEV-ES and SEV-SNP guests.
+			KVM caps the number of SEV-SNP ASIDs at the maximum
+			possible value, e.g. specifying -1u will assign all
+			join SEV-ES+SEV-SNP ASIDs to SEV-SNP and make SEV-ES
+			unusable.
+
 	kvm-arm.mode=
 			[KVM,ARM,EARLY] Select one of KVM/arm64's modes of
 			operation.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index cd9ce100627e..52efd43c333a 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -59,6 +59,9 @@ static bool sev_es_debug_swap_enabled = true;
 module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
 static u64 sev_supported_vmsa_features;
 
+static unsigned int nr_ciphertext_hiding_asids;
+module_param_named(ciphertext_hiding_asids, nr_ciphertext_hiding_asids, uint, 0444);
+
 #define AP_RESET_HOLD_NONE		0
 #define AP_RESET_HOLD_NAE_EVENT		1
 #define AP_RESET_HOLD_MSR_PROTO		2
@@ -201,6 +204,9 @@ static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
 	/*
 	 * The min ASID can end up larger than the max if basic SEV support is
 	 * effectively disabled by disallowing use of ASIDs for SEV guests.
+	 * Similarly for SEV-ES guests the min ASID can end up larger than the
+	 * max when ciphertext hiding is enabled, effectively disabling SEV-ES
+	 * support.
 	 */
 	if (min_asid > max_asid)
 		return -ENOTTY;
@@ -3064,10 +3070,32 @@ void __init sev_hardware_setup(void)
 out:
 	if (sev_enabled) {
 		init_args.probe = true;
+
+		if (sev_is_snp_ciphertext_hiding_supported())
+			init_args.max_snp_asid = min(nr_ciphertext_hiding_asids,
+						     min_sev_asid - 1);
+
 		if (sev_platform_init(&init_args))
 			sev_supported = sev_es_supported = sev_snp_supported = false;
 		else if (sev_snp_supported)
 			sev_snp_supported = is_sev_snp_initialized();
+
+		if (sev_snp_supported)
+			nr_ciphertext_hiding_asids = init_args.max_snp_asid;
+
+		/*
+		 * If ciphertext hiding is enabled, the joint SEV-ES/SEV-SNP
+		 * ASID range is partitioned into separate SEV-ES and SEV-SNP
+		 * ASID ranges, with the SEV-SNP range being [1..max_snp_asid]
+		 * and the SEV-ES range being (max_snp_asid..max_sev_es_asid].
+		 * Note, SEV-ES may effectively be disabled if all ASIDs from
+		 * the joint range are assigned to SEV-SNP.
+		 */
+		if (nr_ciphertext_hiding_asids) {
+			max_snp_asid = nr_ciphertext_hiding_asids;
+			min_sev_es_asid = max_snp_asid + 1;
+			pr_info("SEV-SNP ciphertext hiding enabled\n");
+		}
 	}
 
 	if (boot_cpu_has(X86_FEATURE_SEV))
@@ -3078,7 +3106,9 @@ void __init sev_hardware_setup(void)
 			min_sev_asid, max_sev_asid);
 	if (boot_cpu_has(X86_FEATURE_SEV_ES))
 		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
-			str_enabled_disabled(sev_es_supported),
+			sev_es_supported ? min_sev_es_asid <= max_sev_es_asid ? "enabled" :
+										"unusable" :
+										"disabled",
 			min_sev_es_asid, max_sev_es_asid);
 	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
 		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",

base-commit: fba22ac9ea05ee5e15318823333114104045be2d
--