Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <199c74446ffc18ee61939b0141f56a36142342b7.camel@redhat.com>
Subject: Re: [PATCH 6/7] KVM: nSVM: implement nested VNMI
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Santosh Shukla <santosh.shukla@amd.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     Sean Christopherson <seanjc@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Tom Lendacky <thomas.lendacky@amd.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Tue, 07 Jun 2022 16:22:23 +0300
In-Reply-To: <20220602142620.3196-7-santosh.shukla@amd.com>
References: <20220602142620.3196-1-santosh.shukla@amd.com>
         <20220602142620.3196-7-santosh.shukla@amd.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.4 (3.40.4-2.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@amd.com>
> ---
>  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.


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;