Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support
From: Kim Phillips
Date: Tue Aug 12 2025 - 08:07:42 EST
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@xxxxxxx>
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@xxxxxxx>
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)