Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 21 Nov 2022 17:18:56 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Sandipan Das <sandipan.das@amd.com>,
        Daniel Sneddon <daniel.sneddon@linux.intel.com>,
        Jing Liu <jing2.liu@intel.com>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Wyes Karny <wyes.karny@amd.com>,
        Borislav Petkov <bp@alien8.de>,
        Babu Moger <babu.moger@amd.com>,
        Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
        Jim Mattson <jmattson@google.com>, x86@kernel.org,
        Santosh Shukla <santosh.shukla@amd.com>
Subject: Re: [PATCH 07/13] KVM: SVM: Add VNMI support in get/set_nmi_mask
Message-ID: <Y3uzAM3/XrUPRpgH@google.com>
References: <20221117143242.102721-1-mlevitsk@redhat.com>
 <20221117143242.102721-8-mlevitsk@redhat.com>
 <Y3aDTvglaSfhG8Tg@google.com>
 <4aadf4616e4f1c6219e7c83ee491494feefa78e1.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4aadf4616e4f1c6219e7c83ee491494feefa78e1.camel@redhat.com>
Precedence: bulk

On Mon, Nov 21, 2022, Maxim Levitsky wrote:
> On Thu, 2022-11-17 at 18:54 +0000, Sean Christopherson wrote:
> > E.g. with HF_NMI_MASK => svm->nmi_masked, the end result can be something like:
> > 
> > static bool __is_vnmi_enabled(struct *vmcb)
> > {
> > ????????return !!(vmcb->control.int_ctl & V_NMI_ENABLE);
> > }
> > 
> > static bool is_vnmi_enabled(struct vcpu_svm *svm)
> > {
> > ????????struct vmcb *vmcb = get_vnmi_vmcb(svm);
> > 
> > ????????return vmcb && __is_vnmi_enabled(vmcb);
> > }
> > 
> > static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
> > {
> > ????????struct vcpu_svm *svm = to_svm(vcpu);
> > ????????struct vmcb *vmcb = get_vnmi_vmcb(svm);
> > 
> > ????????if (vmcb && __is_vnmi_enabled(vmcb))
> > ????????????????return !!(vmcb->control.int_ctl & V_NMI_MASK);
> > ????????else
> > ????????????????return !!(vcpu->arch.hflags & HF_NMI_MASK);
> > }
> > 
> > static void svm_set_or_clear_vnmi_mask(struct vmcb *vmcb, bool set)
> > {
> > ????????if (set)
> > ????????????????vmcb->control.int_ctl |= V_NMI_MASK;
> > ????????else
> > ????????????????vmcb->control.int_ctl &= ~V_NMI_MASK;
> > }
> > 
> > static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
> > {
> > ????????struct vcpu_svm *svm = to_svm(vcpu);
> > ????????struct vmcb *vmcb = get_vnmi_vmcb(svm);
> > 
> > ????????if (vmcb && __is_vnmi_enabled(vmcb)) {
> > ????????????????if (masked)
> > ????????????????????????vmcb->control.int_ctl |= V_NMI_MASK;
> > ????????????????else
> > ????????????????????????vmcb->control.int_ctl &= ~V_NMI_MASK;
> > ????????} else {
> > ????????????????svm->nmi_masked = masked;
> > ????????}
> > 
> > ????????if (!masked)
> > ????????????????svm_disable_iret_interception(svm);
> > }
> 
> OK, this is one of the ways to do it, makes sense overall.
> I actualy wanted to do something like that but opted to not touch
> the original code too much, but only what I needed. I can do this
> in a next version.

After looking at more of this code, I think having get_vnmi_vmcb() is a mistake.
It just ends up being a funky wrapper to the current svm->vmcb.  And the manual
check on the "vnmi" global is pointless.  If KVM sets V_NMI_ENABLE in any VMCB
when vnmi=false, then that's a KVM bug.

Dropping the wrapper eliminates the possibility of a NULL VMCB pointer, and IMO
yields far more readable code.


static bool is_vnmi_enabled(struct vcpu_svm *svm)
{
	return !!(svm->vmcb->control.int_ctl & V_NMI_ENABLE);
}

static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
{
	struct vcpu_svm *svm = to_svm(vcpu);

	if (is_vnmi_enabled(svm))
		return !!(svm->vmcb->control.int_ctl & V_NMI_MASK);
	else
		return !!(vcpu->arch.hflags & HF_NMI_MASK);
}

static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
{
	struct vcpu_svm *svm = to_svm(vcpu);

	if (is_vnmi_enabled(svm)) {
		if (masked)
			svm->vmcb->control.int_ctl |= V_NMI_MASK;
		else
			svm->vmcb->control.int_ctl &= ~V_NMI_MASK;
	} else {
		svm->nmi_masked = masked;
	}

	if (!masked)
		svm_disable_iret_interception(svm);
}