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 basically split into legacy SEV and SEV-ES+.
Ciphertext hiding further partitions the SEV-ES+ ASID space into SEV-ES
and SEV-SNP.
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+ ASID
space is allocated to SEV-SNP guests.
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
.../admin-guide/kernel-parameters.txt | 19 ++++++
arch/x86/kvm/svm/sev.c | 58 ++++++++++++++++++-
2 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index ee0735c6b8e2..05e50c37969e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2942,6 +2942,25 @@
(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 legacy 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 and MIN_SEV_ASID value is
+ discovered by CPUID Fn8000_001F[EDX].
+
+ Format: { <unsigned int> | "max" }
+ A non-zero value enables SEV-SNP CipherTextHiding feature and sets
+ how many ASIDs are 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 and also effectively disables
+ 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 89ce9e298201..16723b8e0e37 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 set the number of ASIDs to use ('max' to use 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
@@ -200,6 +205,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;
@@ -2913,10 +2921,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 (!sev_is_snp_ciphertext_hiding_supported()) {
+ pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
+ return false;
+ }
+
+ if (isdigit(ciphertext_hiding_asids[0])) {
+ if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
+ pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+ ciphertext_hiding_asids);
+ return false;
+ }
+ /* Do sanity checks on user-defined ciphertext_hiding_asids */
+ if (ciphertext_hiding_asid_nr >= min_sev_asid) {
+ pr_warn("Requested 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;
+ } else {
+ pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
+ ciphertext_hiding_asids);
+ return false;
+ }
+
+ 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;
+}
+
void __init sev_hardware_setup(void)
{
unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
struct sev_platform_init_args init_args = {0};
+ bool snp_ciphertext_hiding_enabled = false;
bool sev_snp_supported = false;
bool sev_es_supported = false;
bool sev_supported = false;
@@ -3014,6 +3058,14 @@ void __init sev_hardware_setup(void)
min_sev_es_asid = min_snp_asid = 1;
max_sev_es_asid = max_snp_asid = min_sev_asid - 1;
+ /*
+ * 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 (ciphertext_hiding_asids[0])
+ snp_ciphertext_hiding_enabled = check_and_enable_sev_snp_ciphertext_hiding();
+
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;
@@ -3022,6 +3074,8 @@ void __init sev_hardware_setup(void)
out:
if (sev_enabled) {
init_args.probe = true;
+ if (snp_ciphertext_hiding_enabled)
+ 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)
@@ -3036,7 +3090,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 < min_sev_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.1On 7/1/25 15:16, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> Ciphertext hiding prevents host accesses from reading the ciphertext of
> SNP guest private memory. Instead of reading ciphertext, the host reads
> will see constant default values (0xff).
>
> The SEV ASID space is basically split into legacy SEV and SEV-ES+.
s/basically//
s/legacy//
s|SEV-ES+.|SEV-ES/SEV-SNP ASID ranges.|
> Ciphertext hiding further partitions the SEV-ES+ ASID space into SEV-ES
> and SEV-SNP.
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
s/Ciphertext/ciphertext/
> support and a user configurable system-wide maximum SNP ASID value. If
> the module parameter value is "max" then the complete SEV-ES+ ASID
s|SEV-ES+|SEV-ES/SEV-SNP|
> space is allocated to SEV-SNP guests.
>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
> .../admin-guide/kernel-parameters.txt | 19 ++++++
> arch/x86/kvm/svm/sev.c | 58 ++++++++++++++++++-
> 2 files changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index ee0735c6b8e2..05e50c37969e 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2942,6 +2942,25 @@
> (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 legacy SEV and joint
s/legacy//
> + 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 and MIN_SEV_ASID value is
s/and/where/
> + discovered by CPUID Fn8000_001F[EDX].
> +
> + Format: { <unsigned int> | "max" }
> + A non-zero value enables SEV-SNP CipherTextHiding feature and sets
s/CipherTextHiding feature/ciphertext hiding/
> + how many ASIDs are available for SEV-SNP guests.
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 and also effectively disables
s/and also effectively disables/, 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 89ce9e298201..16723b8e0e37 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 set the number of ASIDs to use ('max' to use all available SEV-SNP ASIDs");
This reads a bit awkward, but I don't really have a suggestion for a
short, concise message.
> +
> #define AP_RESET_HOLD_NONE 0
> #define AP_RESET_HOLD_NAE_EVENT 1
> #define AP_RESET_HOLD_MSR_PROTO 2
> @@ -200,6 +205,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;
> @@ -2913,10 +2921,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 (!sev_is_snp_ciphertext_hiding_supported()) {
> + pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
s/or enabled//
> + return false;
> + }
> +
> + if (isdigit(ciphertext_hiding_asids[0])) {
> + if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> + ciphertext_hiding_asids);
> + return false;
> + }
Add a blank line.
> + /* Do sanity checks on user-defined ciphertext_hiding_asids */
s/checks/check/
> + if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> + pr_warn("Requested ciphertext hiding ASIDs (%u) exceeds or equals minimum SEV ASID (%u)\n",
Module parameter ciphertext_hiding_asids (%u) ...
> + ciphertext_hiding_asid_nr, min_sev_asid);
> + return false;
> + }
> + } else if (!strcmp(ciphertext_hiding_asids, "max")) {
> + ciphertext_hiding_asid_nr = min_sev_asid - 1;
> + } else {
> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> + ciphertext_hiding_asids);
> + return false;
> + }
> +
> + max_snp_asid = ciphertext_hiding_asid_nr;
> + min_sev_es_asid = max_snp_asid + 1;
> + pr_info("SEV-SNP ciphertext hiding enabled\n");
Add a blank line.
Thanks,
Tom
> + return true;
> +}
> +
> void __init sev_hardware_setup(void)
> {
> unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
> struct sev_platform_init_args init_args = {0};
> + bool snp_ciphertext_hiding_enabled = false;
> bool sev_snp_supported = false;
> bool sev_es_supported = false;
> bool sev_supported = false;
> @@ -3014,6 +3058,14 @@ void __init sev_hardware_setup(void)
> min_sev_es_asid = min_snp_asid = 1;
> max_sev_es_asid = max_snp_asid = min_sev_asid - 1;
>
> + /*
> + * 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 (ciphertext_hiding_asids[0])
> + snp_ciphertext_hiding_enabled = check_and_enable_sev_snp_ciphertext_hiding();
> +
> 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;
> @@ -3022,6 +3074,8 @@ void __init sev_hardware_setup(void)
> out:
> if (sev_enabled) {
> init_args.probe = true;
> + if (snp_ciphertext_hiding_enabled)
> + 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)
> @@ -3036,7 +3090,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 < min_sev_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",
Hi Ashish,
I can confirm that this v5 series fixes v4's __sev_do_cmd_locked
assertion failure problem, thanks. More comments inline:
On 7/1/25 3:16 PM, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
Extra From: line not necessary.
> @@ -2913,10 +2921,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 (!sev_is_snp_ciphertext_hiding_supported()) {
> + pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
> + return false;
> + }
> +
> + if (isdigit(ciphertext_hiding_asids[0])) {
> + if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> + ciphertext_hiding_asids);
> + return false;
> + }
> + /* Do sanity checks on user-defined ciphertext_hiding_asids */
> + if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> + pr_warn("Requested 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;
> + } else {
> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> + ciphertext_hiding_asids);
> + return false;
> + }
This code can be made much simpler if all the invalid
cases were combined to emit a single pr_warn().
> @@ -3036,7 +3090,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 < min_sev_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",
If I set ciphertext_hiding_asids=99, I get the new 'unusable':
kvm_amd: SEV-SNP ciphertext hiding enabled
...
kvm_amd: SEV enabled (ASIDs 100 - 1006)
kvm_amd: SEV-ES unusable (ASIDs 100 - 99)
kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
Ok.
Now, if I set ciphertext_hiding_asids=0, I get:
kvm_amd: SEV-SNP ciphertext hiding enabled
...
kvm_amd: SEV enabled (ASIDs 100 - 1006)
kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
kvm_amd: SEV-SNP enabled (ASIDs 1 - 0)
..where SNP is unusable this time, yet it's not flagged as such.
If there's no difference between "unusable" and not enabled, then
I think it's better to keep the not enabled messaging behaviour
and just not emit the line at all: It's confusing to see the
invalid "100 - 99" and "1 - 0" ranges.
Thanks,
Kim
Hello Kim,
On 7/2/2025 4:46 PM, Kim Phillips wrote:
> Hi Ashish,
>
> I can confirm that this v5 series fixes v4's __sev_do_cmd_locked
> assertion failure problem, thanks. More comments inline:
>
> On 7/1/25 3:16 PM, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>
> Extra From: line not necessary.
>
>> @@ -2913,10 +2921,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 (!sev_is_snp_ciphertext_hiding_supported()) {
>> + pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
>> + return false;
>> + }
>> +
>> + if (isdigit(ciphertext_hiding_asids[0])) {
>> + if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
>> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>> + ciphertext_hiding_asids);
>> + return false;
>> + }
>> + /* Do sanity checks on user-defined ciphertext_hiding_asids */
>> + if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>> + pr_warn("Requested 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;
>> + } else {
>> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>> + ciphertext_hiding_asids);
>> + return false;
>> + }
>
> This code can be made much simpler if all the invalid
> cases were combined to emit a single pr_warn().
>
There definitely has to be a different pr_warn() for the sanity check case and invalid parameter cases and sanity check has to be done if the
specified parameter is an unsigned int, so the check needs to be done separately.
I can definitely add a branch just for the invalid cases.
>> @@ -3036,7 +3090,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 < min_sev_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",
>
> If I set ciphertext_hiding_asids=99, I get the new 'unusable':
>
> kvm_amd: SEV-SNP ciphertext hiding enabled
> ...
> kvm_amd: SEV enabled (ASIDs 100 - 1006)
> kvm_amd: SEV-ES unusable (ASIDs 100 - 99)
> kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
>
> Ok.
Which is correct.
This is similar to the SEV case where min_sev_asid can be greater than max_sev_asid and that also emits similarly :
SEV unusable (ASIDs 1007 - 1006) (this is an example of that case).
>
> Now, if I set ciphertext_hiding_asids=0, I get:
>
> kvm_amd: SEV-SNP ciphertext hiding enabled
> ...
> kvm_amd: SEV enabled (ASIDs 100 - 1006)
> kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
> kvm_amd: SEV-SNP enabled (ASIDs 1 - 0)
>
> ..where SNP is unusable this time, yet it's not flagged as such.
>
Actually SNP still needs to be usable/enabled in this case, as specifying ciphertext_hiding_asids=0 is same as specifying that ciphertext hiding feature should
not be enabled, so code-wise this is behaving correctly, but messaging needs to be fixed, which i will fix.
Thanks,
Ashish
> If there's no difference between "unusable" and not enabled, then
> I think it's better to keep the not enabled messaging behaviour
> and just not emit the line at all: It's confusing to see the
> invalid "100 - 99" and "1 - 0" ranges.
>
> Thanks,
>
> Kim
On 7/2/2025 5:43 PM, Kalra, Ashish wrote:
> Hello Kim,
>
> On 7/2/2025 4:46 PM, Kim Phillips wrote:
>> Hi Ashish,
>>
>> I can confirm that this v5 series fixes v4's __sev_do_cmd_locked
>> assertion failure problem, thanks. More comments inline:
>>
>> On 7/1/25 3:16 PM, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> Extra From: line not necessary.
>>
>>> @@ -2913,10 +2921,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 (!sev_is_snp_ciphertext_hiding_supported()) {
>>> + pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
>>> + return false;
>>> + }
>>> +
>>> + if (isdigit(ciphertext_hiding_asids[0])) {
>>> + if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
>>> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>>> + ciphertext_hiding_asids);
>>> + return false;
>>> + }
>>> + /* Do sanity checks on user-defined ciphertext_hiding_asids */
>>> + if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>>> + pr_warn("Requested 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;
>>> + } else {
>>> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>>> + ciphertext_hiding_asids);
>>> + return false;
>>> + }
>>
>> This code can be made much simpler if all the invalid
>> cases were combined to emit a single pr_warn().
>>
>
> There definitely has to be a different pr_warn() for the sanity check case and invalid parameter cases and sanity check has to be done if the
> specified parameter is an unsigned int, so the check needs to be done separately.
>
> I can definitely add a branch just for the invalid cases.
>
>>> @@ -3036,7 +3090,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 < min_sev_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",
>>
>> If I set ciphertext_hiding_asids=99, I get the new 'unusable':
>>
>> kvm_amd: SEV-SNP ciphertext hiding enabled
>> ...
>> kvm_amd: SEV enabled (ASIDs 100 - 1006)
>> kvm_amd: SEV-ES unusable (ASIDs 100 - 99)
>> kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
>>
>> Ok.
>
> Which is correct.
>
> This is similar to the SEV case where min_sev_asid can be greater than max_sev_asid and that also emits similarly :
> SEV unusable (ASIDs 1007 - 1006) (this is an example of that case).
>
Also do note that the message above is printing the exact values of min_sev_es_asid and max_sev_es_asid, as they have been computed.
And it adds that SEV-ES is now unusable as now min_sev_es_asid > max_sev_es_asid.
>>
>> Now, if I set ciphertext_hiding_asids=0, I get:
>>
>> kvm_amd: SEV-SNP ciphertext hiding enabled
>> ...
>> kvm_amd: SEV enabled (ASIDs 100 - 1006)
>> kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
>> kvm_amd: SEV-SNP enabled (ASIDs 1 - 0)
>>
>> ..where SNP is unusable this time, yet it's not flagged as such.
>>
>
> Actually SNP still needs to be usable/enabled in this case, as specifying ciphertext_hiding_asids=0 is same as specifying that ciphertext hiding feature should
> not be enabled, so code-wise this is behaving correctly, but messaging needs to be fixed, which i will fix.
>
And i do need to fix this case for ciphertext_hiding_asids==0, i.e., ciphertext hiding feature is not enabled, as the above is not functioning correctly.
Thanks,
Ashish
>
>> If there's no difference between "unusable" and not enabled, then
>> I think it's better to keep the not enabled messaging behaviour
>> and just not emit the line at all: It's confusing to see the
>> invalid "100 - 99" and "1 - 0" ranges.
>>
>> Thanks,
>>
>> Kim
>
On 7/7/25 1:16 AM, Kalra, Ashish wrote:
>
> On 7/2/2025 5:43 PM, Kalra, Ashish wrote:
>> Hello Kim,
>>
>> On 7/2/2025 4:46 PM, Kim Phillips wrote:
>>> Hi Ashish,
>>>
>>> I can confirm that this v5 series fixes v4's __sev_do_cmd_locked
>>> assertion failure problem, thanks. More comments inline:
>>>
>>> On 7/1/25 3:16 PM, Ashish Kalra wrote:
>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>
>>> Extra From: line not necessary.
>>>
>>>> @@ -2913,10 +2921,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 (!sev_is_snp_ciphertext_hiding_supported()) {
>>>> + pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
>>>> + return false;
>>>> + }
>>>> +
>>>> + if (isdigit(ciphertext_hiding_asids[0])) {
>>>> + if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
>>>> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>>>> + ciphertext_hiding_asids);
>>>> + return false;
>>>> + }
>>>> + /* Do sanity checks on user-defined ciphertext_hiding_asids */
>>>> + if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>>>> + pr_warn("Requested 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;
>>>> + } else {
>>>> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>>>> + ciphertext_hiding_asids);
>>>> + return false;
>>>> + }
>>>
>>> This code can be made much simpler if all the invalid
>>> cases were combined to emit a single pr_warn().
>>>
>>
>> There definitely has to be a different pr_warn() for the sanity check case and invalid parameter cases and sanity check has to be done if the
>> specified parameter is an unsigned int, so the check needs to be done separately.
>>
>> I can definitely add a branch just for the invalid cases.
>>
>>>> @@ -3036,7 +3090,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 < min_sev_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",
>>>
>>> If I set ciphertext_hiding_asids=99, I get the new 'unusable':
>>>
>>> kvm_amd: SEV-SNP ciphertext hiding enabled
>>> ...
>>> kvm_amd: SEV enabled (ASIDs 100 - 1006)
>>> kvm_amd: SEV-ES unusable (ASIDs 100 - 99)
>>> kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
>>>
>>> Ok.
>>
>> Which is correct.
>>
>> This is similar to the SEV case where min_sev_asid can be greater than max_sev_asid and that also emits similarly :
>> SEV unusable (ASIDs 1007 - 1006) (this is an example of that case).
>>
>
> Also do note that the message above is printing the exact values of min_sev_es_asid and max_sev_es_asid, as they have been computed.
>
> And it adds that SEV-ES is now unusable as now min_sev_es_asid > max_sev_es_asid.
Right, it'd be nice if that were made clearer to the user, too:
min_sev_es_asid 100 > max_sev_es_asid 99
>>> Now, if I set ciphertext_hiding_asids=0, I get:
>>>
>>> kvm_amd: SEV-SNP ciphertext hiding enabled
>>> ...
>>> kvm_amd: SEV enabled (ASIDs 100 - 1006)
>>> kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
>>> kvm_amd: SEV-SNP enabled (ASIDs 1 - 0)
>>>
>>> ..where SNP is unusable this time, yet it's not flagged as such.
>>>
>>
>> Actually SNP still needs to be usable/enabled in this case, as specifying ciphertext_hiding_asids=0 is same as specifying that ciphertext hiding feature should
>> not be enabled, so code-wise this is behaving correctly, but messaging needs to be fixed, which i will fix.
>>
>
> And i do need to fix this case for ciphertext_hiding_asids==0, i.e., ciphertext hiding feature is not enabled, as the above is not functioning correctly.
Right, it's not just messaging, SNP should still be enabled when ciphertext_hiding_asids==0,
whereas this is not the case with this patchset.
>
> Thanks,
> Ashish
>
>>
>>> If there's no difference between "unusable" and not enabled, then
>>> I think it's better to keep the not enabled messaging behaviour
>>> and just not emit the line at all: It's confusing to see the
>>> invalid "100 - 99" and "1 - 0" ranges.
Please also consider this.
Thanks,
Kim
© 2016 - 2025 Red Hat, Inc.