Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

["Kalra, Ashish" <ashish.kalra@amd.com>]

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)
>>
>