Re: [PATCH 6/7] KVM: nSVM: implement nested VNMI

[Shukla, Santosh]

On 6/7/2022 6:52 PM, Maxim Levitsky wrote:
> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
>> Currently nested_vmcb02_prepare_control func checks and programs bits
>> (V_TPR,_INTR, _IRQ) in nested mode, To support nested VNMI,
>> extending the check for VNMI bits if VNMI is enabled.
>>
>> Tested with the KVM-unit-test that is developed for this purpose.
>>
>> Signed-off-by: Santosh Shukla <santosh.shukla@xxxxxxx>
>> ---
>>  arch/x86/kvm/svm/nested.c | 8 ++++++++
>>  arch/x86/kvm/svm/svm.c    | 5 +++++
>>  arch/x86/kvm/svm/svm.h    | 1 +
>>  3 files changed, 14 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index bed5e1692cef..ce83739bae50 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -608,6 +608,11 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>>         }
>>  }
>>  
>> +static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
>> +{
>> +       return svm->vnmi_enabled && (svm->nested.ctl.int_ctl & V_NMI_ENABLE);
>> +}
>> +
>>  static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>>  {
>>         u32 int_ctl_vmcb01_bits = V_INTR_MASKING_MASK;
>> @@ -627,6 +632,9 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>>         else
>>                 int_ctl_vmcb01_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
>>  
>> +       if (nested_vnmi_enabled(svm))
>> +               int_ctl_vmcb12_bits |= (V_NMI_PENDING | V_NMI_ENABLE);
> 
> This is for sure not enough - we also need to at least copy V_NMI_PENDING/V_NMI_MASK
> back to vmc12 on vmexit, and also think about what happens with L1's VNMI while L2 is running.
> 
> E.g functions like is_vnmi_mask_set, likely should always reference vmcb01, and I *think*
> that while L2 is running L1's vNMI should be sort of 'inhibited' like I did with AVIC.
> 
> For example the svm_nmi_blocked should probably first check for 'is_guest_mode(vcpu) && nested_exit_on_nmi(svm)'
> and only then start checking for vNMI.
> 
> There also are interactions with vGIF and nested vGIF that should be checked as well.
> 
> Finally the patch series needs tests, several tests, including a test when a nested guest
> runs and the L1 receives NMI, and check that it works both when L1 intercepts NMI and doesn't intercept NMIs,
> and if vNMI is enabled L1, and both enabled and not enabled in L2.
> 
> 

In V2.

Thank-you Maxim for the review comment.
Santosh.

> Best regards,
> 	Maxim Levitsky
> 
>> +
>>         /* Copied from vmcb01.  msrpm_base can be overwritten later.  */
>>         vmcb02->control.nested_ctl = vmcb01->control.nested_ctl;
>>         vmcb02->control.iopm_base_pa = vmcb01->control.iopm_base_pa;
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 200f979169e0..c91af728420b 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -4075,6 +4075,8 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>  
>>         svm->vgif_enabled = vgif && guest_cpuid_has(vcpu, X86_FEATURE_VGIF);
>>  
>> +       svm->vnmi_enabled = vnmi && guest_cpuid_has(vcpu, X86_FEATURE_V_NMI);
>> +
>>         svm_recalc_instruction_intercepts(vcpu, svm);
>>  
>>         /* For sev guests, the memory encryption bit is not reserved in CR3.  */
>> @@ -4831,6 +4833,9 @@ static __init void svm_set_cpu_caps(void)
>>                 if (vgif)
>>                         kvm_cpu_cap_set(X86_FEATURE_VGIF);
>>  
>> +               if (vnmi)
>> +                       kvm_cpu_cap_set(X86_FEATURE_V_NMI);
>> +
>>                 /* Nested VM can receive #VMEXIT instead of triggering #GP */
>>                 kvm_cpu_cap_set(X86_FEATURE_SVME_ADDR_CHK);
>>         }
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> index 21c5460e947a..f926c77bf857 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -240,6 +240,7 @@ struct vcpu_svm {
>>         bool pause_filter_enabled         : 1;
>>         bool pause_threshold_enabled      : 1;
>>         bool vgif_enabled                 : 1;
>> +       bool vnmi_enabled                 : 1;
>>  
>>         u32 ldr_reg;
>>         u32 dfr_reg;
> 
>