On 8/12/2025 7:06 AM, Kim Phillips wrote:
> 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;
> }
>
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.
Again, why do we want to do all these checks below if this module parameter has not been specified by
the user ?
> - 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"
Thanks,
Ashish
> - 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)
>>>>
>
