Re: [PATCH v4 3/4] KVM: nSVM: implement the nested idle halt intercept

[Manali Shukla <manali.shukla@amd.com>]

Hi Sean,

Thank you for reviewing my patches.

On 12/20/2024 6:31 AM, Sean Christopherson wrote:
> On Tue, Oct 22, 2024, Manali Shukla wrote:
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index d5314cb7dff4..feb241110f1a 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -178,6 +178,14 @@ void recalc_intercepts(struct vcpu_svm *svm)
>>  	} else {
>>  		WARN_ON(!(c->virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
>>  	}
>> +
>> +	/*
>> +	 * Clear the HLT intercept for L2 guest when the Idle HLT intercept feature
>> +	 * is enabled on the platform and the guest can use the Idle HLT intercept
>> +	 * feature.
>> +	 */
>> +	if (guest_can_use(&svm->vcpu, X86_FEATURE_IDLE_HLT))
>> +		vmcb_clr_intercept(c, INTERCEPT_HLT);
> 
> This is wrong.  KVM needs to honor the intercept of vmcb12.  If L1 wants to
> intercept HLT, then KVM needs to configure vmcb02 to intercept HLT, regradless
> of whether or not L1 is utilizing INTERCEPT_IDLE_HLT.
> 
> Given how KVM currently handles intercepts for nested SVM, I'm pretty sure you
> can simply do nothing.  recalc_intercepts() starts with KVM's intercepts (from
> vmcb01), and adds in L1's intercepts.  So unless there is a special case, the
> default behavior should Just Work.
> 
> 	for (i = 0; i < MAX_INTERCEPT; i++)
> 		c->intercepts[i] = h->intercepts[i];
> 
> 	...
> 
> 	for (i = 0; i < MAX_INTERCEPT; i++)
> 		c->intercepts[i] |= g->intercepts[i];
> 
> KVM's approach creates all kinds of virtualization holes, e.g. L1 can utilize
> IDLE_HLT even if the feature isn't advertised to L1.  But that's true for quite
> literally all feature-based intercepts, so for better or worse, I don't think
> it makes sense to try and change that approach for this feature.
> 

Yeah. Makes sense. I will remove the above condition from V5, so that intercept of
vmcb12 is honored.

>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index e86b79e975d3..38d546788fc6 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -4425,6 +4425,7 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>  	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
>>  	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VGIF);
>>  	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VNMI);
>> +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_IDLE_HLT);
>>  
>>  	svm_recalc_instruction_intercepts(vcpu, svm);
>>  
>> @@ -5228,6 +5229,9 @@ static __init void svm_set_cpu_caps(void)
>>  		if (vnmi)
>>  			kvm_cpu_cap_set(X86_FEATURE_VNMI);
>>  
>> +		if (cpu_feature_enabled(X86_FEATURE_IDLE_HLT))
>> +			kvm_cpu_cap_set(X86_FEATURE_IDLE_HLT);
> 
> kvm_cpu_cap_check_and_set() does this for you.
> 
>> +
>>  		/* Nested VM can receive #VMEXIT instead of triggering #GP */
>>  		kvm_cpu_cap_set(X86_FEATURE_SVME_ADDR_CHK);
>>  	}
>> -- 
>> 2.34.1
>>
- Manali