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.1Hi 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)
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)
>
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)
>>
>
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)
>>>
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)
>>>>
>
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
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
>
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
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 >
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
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 >
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)
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)
© 2016 - 2025 Red Hat, Inc.