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
>
