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@xxxxxxx>
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
