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:   Thu, 28 Jul 2022 20:11:59 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Kai Huang <kai.huang@intel.com>
Cc:     Isaku Yamahata <isaku.yamahata@gmail.com>,
        isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v7 041/102] KVM: VMX: Introduce test mode related to EPT
 violation VE
Message-ID: <YuLtj4/pgUZBc6f9@google.com>
References: <cover.1656366337.git.isaku.yamahata@intel.com>
 <cadf3221e3f7b911c810f15cfe300dd5337a966d.1656366338.git.isaku.yamahata@intel.com>
 <52915310c9118a124da2380daf3d753a818de05e.camel@intel.com>
 <20220719144936.GX1379820@ls.amr.corp.intel.com>
 <9945dbf586d8738b7cf0af53bfb760da9eb9e882.camel@intel.com>
 <20220727233955.GC3669189@ls.amr.corp.intel.com>
 <af9e3b06ba9e16df4bfd768dfdd78f2e0277cbe5.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <af9e3b06ba9e16df4bfd768dfdd78f2e0277cbe5.camel@intel.com>
Precedence: bulk

On Thu, Jul 28, 2022, Kai Huang wrote:
> On Wed, 2022-07-27 at 16:39 -0700, Isaku Yamahata wrote:
> > On Wed, Jul 20, 2022 at 05:13:08PM +1200,
> > Kai Huang <kai.huang@intel.com> wrote:
> > 
> > > On Tue, 2022-07-19 at 07:49 -0700, Isaku Yamahata wrote:
> > > > On Fri, Jul 08, 2022 at 02:23:43PM +1200,
> > > > Kai Huang <kai.huang@intel.com> wrote:
> > > > 
> > > > > On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@intel.com wrote:
> > > > > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > > > > 
> > > > > > To support TDX, KVM is enhanced to operate with #VE.  For TDX, KVM programs
> > > > > > to inject #VE conditionally and set #VE suppress bit in EPT entry.  For VMX
> > > > > > case, #VE isn't used.  If #VE happens for VMX, it's a bug.  To be
> > > > > > defensive (test that VMX case isn't broken), introduce option
> > > > > > ept_violation_ve_test and when it's set, set error.
> > > > > 
> > > > > I don't see why we need this patch.  It may be helpful during your test, but why
> > > > > do we need this patch for formal submission?
> > > > > 
> > > > > And for a normal guest, what prevents one vcpu from sending #VE IPI to another
> > > > > vcpu?
> > > > 
> > > > Paolo suggested it as follows.  Maybe it should be kernel config.
> > > > (I forgot to add suggested-by. I'll add it)
> > > > 
> > > > https://lore.kernel.org/lkml/84d56339-4a8a-6ddb-17cb-12074588ba9c@redhat.com/
> > > > 
> > > > > 
> > > 
> > > OK.  But can we assume a normal guest won't sending #VE IPI?
> > 
> > Theoretically nothing prevents that.  I wouldn't way "normal".
> > Anyway this is off by default.
> 
> I don't think whether it is on or off by default matters.

It matters in the sense that the module param is intended purely for testing, i.e.
there's zero reason to ever enable it in production.  That changes what is and
wasn't isn't a reasonable response to an unexpected #VE.

> If it can happen legitimately in the guest, it doesn't look right to print
> out something like below:
> 
> 	pr_err("VMEXIT due to unexpected #VE.\n");

Agreed.  In this particular case I think the right approach is to treat an
unexpected #VE as a fatal KVM bug.  Yes, disabling EPT violation #VEs would likely
allow the guest to live, but as above the module param should never be enabled in
production.  And if we get a #VE with the module param disabled, then KVM is truly
in the weeds and killing the VM is the safe option.

E.g. something like

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4fd25e1d6ec9..54b9cb56f6e2 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5010,6 +5010,9 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
        if (is_invalid_opcode(intr_info))
                return handle_ud(vcpu);

+       if (KVM_BUG_ON(is_ve_fault(intr_info), vcpu->kvm))
+               return -EIO;
+
        error_code = 0;
        if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
                error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);