Re: [PATCH v7 041/102] KVM: VMX: Introduce test mode related to EPT violation VE

Isaku Yamahata
2022-08-09T00:48:53Z

On Thu, Jul 28, 2022 at 08:11:59PM +0000,
Sean Christopherson <seanjc@google.com> wrote:

> 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

Thanks, I finally ended up with the following.

diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
index ac290a44a693..9277676057a7 100644
--- a/arch/x86/kvm/vmx/vmcs.h
+++ b/arch/x86/kvm/vmx/vmcs.h
@@ -140,6 +140,11 @@ static inline bool is_nm_fault(u32 intr_info)
return is_exception_n(intr_info, NM_VECTOR);
}

+static inline bool is_ve_fault(u32 intr_info)
+{
+ return is_exception_n(intr_info, VE_VECTOR);
+}
+
/* Undocumented: icebp/int1 */
static inline bool is_icebp(u32 intr_info)
{
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 881db80ceee9..c3e4c0d17b63 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5047,6 +5047,12 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
if (is_invalid_opcode(intr_info))
return handle_ud(vcpu);

+ /*
+ * #VE isn't supposed to happen. Although vcpu can send
+ */
+ 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);
@@ -5167,14 +5173,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
if (handle_guest_split_lock(kvm_rip_read(vcpu)))
return 1;
fallthrough;
- case VE_VECTOR:
default:
- if (ept_violation_ve_test && ex_no == VE_VECTOR) {
- pr_err("VMEXIT due to unexpected #VE.\n");
- secondary_exec_controls_clearbit(
- vmx, SECONDARY_EXEC_EPT_VIOLATION_VE);
- return 1;
- }
kvm_run->exit_reason = KVM_EXIT_EXCEPTION;
kvm_run->ex.exception = ex_no;
kvm_run->ex.error_code = error_code;

--
Isaku Yamahata <isaku.yamahata@gmail.com>