Re: [PATCH v10 039/108] KVM: VMX: Introduce test mode related to EPT violation VE

[Isaku Yamahata <isaku.yamahata@gmail.com>]

On Thu, Nov 03, 2022 at 09:41:44PM +0800,
Binbin Wu <binbin.wu@linux.intel.com> wrote:

> 
> On 2022/10/30 14:22, 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.
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >   arch/x86/include/asm/vmx.h | 12 +++++++
> >   arch/x86/kvm/vmx/vmcs.h    |  5 +++
> >   arch/x86/kvm/vmx/vmx.c     | 69 +++++++++++++++++++++++++++++++++++++-
> >   arch/x86/kvm/vmx/vmx.h     |  6 +++-
> >   4 files changed, 90 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> > index cdbf12c1a83c..752d53652007 100644
> > --- a/arch/x86/include/asm/vmx.h
> > +++ b/arch/x86/include/asm/vmx.h
> > @@ -68,6 +68,7 @@
> >   #define SECONDARY_EXEC_ENCLS_EXITING		VMCS_CONTROL_BIT(ENCLS_EXITING)
> >   #define SECONDARY_EXEC_RDSEED_EXITING		VMCS_CONTROL_BIT(RDSEED_EXITING)
> >   #define SECONDARY_EXEC_ENABLE_PML               VMCS_CONTROL_BIT(PAGE_MOD_LOGGING)
> > +#define SECONDARY_EXEC_EPT_VIOLATION_VE		VMCS_CONTROL_BIT(EPT_VIOLATION_VE)
> >   #define SECONDARY_EXEC_PT_CONCEAL_VMX		VMCS_CONTROL_BIT(PT_CONCEAL_VMX)
> >   #define SECONDARY_EXEC_XSAVES			VMCS_CONTROL_BIT(XSAVES)
> >   #define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	VMCS_CONTROL_BIT(MODE_BASED_EPT_EXEC)
> > @@ -223,6 +224,8 @@ enum vmcs_field {
> >   	VMREAD_BITMAP_HIGH              = 0x00002027,
> >   	VMWRITE_BITMAP                  = 0x00002028,
> >   	VMWRITE_BITMAP_HIGH             = 0x00002029,
> > +	VE_INFORMATION_ADDRESS		= 0x0000202A,
> > +	VE_INFORMATION_ADDRESS_HIGH	= 0x0000202B,
> >   	XSS_EXIT_BITMAP                 = 0x0000202C,
> >   	XSS_EXIT_BITMAP_HIGH            = 0x0000202D,
> >   	ENCLS_EXITING_BITMAP		= 0x0000202E,
> > @@ -628,4 +631,13 @@ enum vmx_l1d_flush_state {
> >   extern enum vmx_l1d_flush_state l1tf_vmx_mitigation;
> > +struct vmx_ve_information {
> > +	u32 exit_reason;
> > +	u32 delivery;
> > +	u64 exit_qualification;
> > +	u64 guest_linear_address;
> > +	u64 guest_physical_address;
> > +	u16 eptp_index;
> > +};
> > +
> >   #endif
> > 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 f890191e8580..dd3fde9d3c32 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -126,6 +126,9 @@ module_param(error_on_inconsistent_vmcs_config, bool, 0444);
> >   static bool __read_mostly dump_invalid_vmcs = 0;
> >   module_param(dump_invalid_vmcs, bool, 0644);
> > +static bool __read_mostly ept_violation_ve_test;
> > +module_param(ept_violation_ve_test, bool, 0444);
> > +
> >   #define MSR_BITMAP_MODE_X2APIC		1
> >   #define MSR_BITMAP_MODE_X2APIC_APICV	2
> > @@ -783,6 +786,13 @@ void vmx_update_exception_bitmap(struct kvm_vcpu *vcpu)
> >   	eb = (1u << PF_VECTOR) | (1u << UD_VECTOR) | (1u << MC_VECTOR) |
> >   	     (1u << DB_VECTOR) | (1u << AC_VECTOR);
> > +	/*
> > +	 * #VE isn't used for VMX, but for TDX.  To test against unexpected
> > +	 * change related to #VE for VMX, intercept unexpected #VE and warn on
> > +	 * it.
> > +	 */
> > +	if (ept_violation_ve_test)
> > +		eb |= 1u << VE_VECTOR;
> >   	/*
> >   	 * Guest access to VMware backdoor ports could legitimately
> >   	 * trigger #GP because of TSS I/O permission bitmap.
> > @@ -2644,6 +2654,9 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> >   					&_cpu_based_2nd_exec_control))
> >   			return -EIO;
> >   	}
> > +	if (!ept_violation_ve_test)
> > +		_cpu_based_exec_control &= ~SECONDARY_EXEC_EPT_VIOLATION_VE;
> 
> should be _cpu_based_2nd_exec_control

Oops, thanks.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>