Re: [PATCH v4 4/5] KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables

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

On 6/3/2025 10:52 AM, Tom Lendacky wrote:
> On 5/19/25 18:57, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> Introduce new min, max sev_es_asid and sev_snp_asid variables.
>>
>> The new {min,max}_{sev_es,snp}_asid variables along with existing
>> {min,max}_sev_asid variable simplifies partitioning of the
>> SEV and SEV-ES+ ASID space.
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>  arch/x86/kvm/svm/sev.c | 37 ++++++++++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index dea9480b9ff6..383db1da8699 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -85,6 +85,10 @@ static DECLARE_RWSEM(sev_deactivate_lock);
>>  static DEFINE_MUTEX(sev_bitmap_lock);
>>  unsigned int max_sev_asid;
>>  static unsigned int min_sev_asid;
>> +static unsigned int max_sev_es_asid;
>> +static unsigned int min_sev_es_asid;
>> +static unsigned int max_snp_asid;
>> +static unsigned int min_snp_asid;
>>  static unsigned long sev_me_mask;
>>  static unsigned int nr_asids;
>>  static unsigned long *sev_asid_bitmap;
>> @@ -172,20 +176,32 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
>>  	misc_cg_uncharge(type, sev->misc_cg, 1);
>>  }
>>  
>> -static int sev_asid_new(struct kvm_sev_info *sev)
>> +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
>>  {
>>  	/*
>>  	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
>>  	 * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
>> -	 * Note: min ASID can end up larger than the max if basic SEV support is
>> -	 * effectively disabled by disallowing use of ASIDs for SEV guests.
>>  	 */
>> -	unsigned int min_asid = sev->es_active ? 1 : min_sev_asid;
>> -	unsigned int max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid;
>> -	unsigned int asid;
>> +	unsigned int min_asid, max_asid, asid;
>>  	bool retry = true;
>>  	int ret;
>>  
>> +	if (vm_type == KVM_X86_SNP_VM) {
>> +		min_asid = min_snp_asid;
>> +		max_asid = max_snp_asid;
>> +	} else if (sev->es_active) {
>> +		min_asid = min_sev_es_asid;
>> +		max_asid = max_sev_es_asid;
>> +	} else {
>> +		min_asid = min_sev_asid;
>> +		max_asid = max_sev_asid;
>> +	}
>> +
>> +	/*
>> +	 * The min ASID can end up larger than the max if basic SEV support is
>> +	 * effectively disabled by disallowing use of ASIDs for SEV guests.
>> +	 */
>> +
> 
> Remove blank line.
> 
>>  	if (min_asid > max_asid)
>>  		return -ENOTTY;
>>  
>> @@ -439,7 +455,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>>  	if (vm_type == KVM_X86_SNP_VM)
>>  		sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
>>  
>> -	ret = sev_asid_new(sev);
>> +	ret = sev_asid_new(sev, vm_type);
>>  	if (ret)
>>  		goto e_no_asid;
>>  
>> @@ -3029,6 +3045,9 @@ void __init sev_hardware_setup(void)
>>  		goto out;
>>  	}
>>  
>> +	min_sev_es_asid = min_snp_asid = 1;
>> +	max_sev_es_asid = max_snp_asid = min_sev_asid - 1;
> 
> Should these be moved to after the min_sev_asid == 1 check ...
> 
>> +
>>  	/* Has the system been allocated ASIDs for SEV-ES? */
>>  	if (min_sev_asid == 1)
>>  		goto out;
>> @@ -3048,11 +3067,11 @@ void __init sev_hardware_setup(void)
>>  	if (boot_cpu_has(X86_FEATURE_SEV_ES))
>>  		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
>>  			str_enabled_disabled(sev_es_supported),
>> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
>> +			min_sev_es_asid, max_sev_es_asid);
> 
> ... so that this becomes 0 and 0 if min_sev_asid == 1 ? (like before)
> 
>>  	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
>>  		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
>>  			str_enabled_disabled(sev_snp_supported),
>> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
>> +			min_snp_asid, max_snp_asid);
> 
> Ditto
>

Yes that makes sense.

Thanks,
Ashish

 
> Thanks,
> Tom
> 
>>  >  	sev_enabled = sev_supported;
>>  	sev_es_enabled = sev_es_supported;