Date:   Fri, 10 Feb 2023 16:44:53 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Santosh Shukla <santosh.shukla@amd.com>
Cc:     Maxim Levitsky <mlevitsk@redhat.com>, kvm@vger.kernel.org,
        Sandipan Das <sandipan.das@amd.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Borislav Petkov <bp@alien8.de>,
        Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Daniel Sneddon <daniel.sneddon@linux.intel.com>,
        Jiaxi Chen <jiaxi.chen@linux.intel.com>,
        Babu Moger <babu.moger@amd.com>, linux-kernel@vger.kernel.org,
        Jing Liu <jing2.liu@intel.com>,
        Wyes Karny <wyes.karny@amd.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH v2 10/11] KVM: SVM: implement support for vNMI
Message-ID: <Y+Z0hUGLkwADyTNu@google.com>
References: <20221129193717.513824-1-mlevitsk@redhat.com>
 <20221129193717.513824-11-mlevitsk@redhat.com>
 <Y9mwz/G6+G8NSX3+@google.com>
 <439a8cfc-fe2d-436d-2ea2-b795eb983b9a@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <439a8cfc-fe2d-436d-2ea2-b795eb983b9a@amd.com>
Precedence: bulk

On Fri, Feb 10, 2023, Santosh Shukla wrote:
> On 2/1/2023 5:52 AM, Sean Christopherson wrote:
> So you mean.. In vNMI mode, KVM should never need to request NMI window and eventually
> it reaches to NMI window then WARN_ON and cont.. to single step... so modified code change
> may look something like below:
> 
> static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
> {
>         struct vcpu_svm *svm = to_svm(vcpu);
> 
>         /*
>          * With vNMI we should never need an NMI window.
>          * and if we reach here then better WARN and continue to single step.
>          */
>         WARN_ON_ONCE(is_vnmi_enabled(svm));
> 
>         if (svm_get_nmi_mask(vcpu) && !svm->awaiting_iret_completion)
>                 return; /* IRET will cause a vm exit */
> 
>         if (!gif_set(svm)) {
>                 if (vgif)
>                         svm_set_intercept(svm, INTERCEPT_STGI);
>                 return; /* STGI will cause a vm exit */
>         }
> 
>         /*
>          * Something prevents NMI from been injected. Single step over possible
>          * problem (IRET or exception injection or interrupt shadow)
>          */
> 
>         svm->nmi_singlestep_guest_rflags = svm_get_rflags(vcpu);
>         svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
>         svm->nmi_singlestep = true;
> }
> 
> Does that make sense?

Yep.  Though please avoid "we" and other pronouns in changelogs and comments,
and wrap as close to the boundary as possible.