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