Re: [PATCH v4 1/3] KVM: x86: add support for user wait instructions

[Tao Xu]

On 6/19/2019 3:16 PM, Xiaoyao Li wrote:
> On 6/19/2019 3:01 PM, Tao Xu wrote:
> > On 6/19/2019 2:23 PM, Xiaoyao Li wrote:
> > > On 6/19/2019 2:09 PM, Tao Xu wrote:
> > > > UMONITOR, UMWAIT and TPAUSE are a set of user wait instructions.
> > > > This patch adds support for user wait instructions in KVM. Availability
> > > > of the user wait instructions is indicated by the presence of the CPUID
> > > > feature flag WAITPKG CPUID.0x07.0x0:ECX[5]. User wait instructions may
> > > > be executed at any privilege level, and use IA32_UMWAIT_CONTROL MSR to
> > > > set the maximum time.
> > > >
> > > > The behavior of user wait instructions in VMX non-root operation is
> > > > determined first by the setting of the "enable user wait and pause"
> > > > secondary processor-based VM-execution control bit 26.
> > > > ÂÂÂÂIf the VM-execution control is 0, UMONITOR/UMWAIT/TPAUSE cause
> > > > an invalid-opcode exception (#UD).
> > > > ÂÂÂÂIf the VM-execution control is 1, treatment is based on the
> > > > setting of the âRDTSC exitingâ VM-execution control. Because KVM never
> > > > enables RDTSC exiting, if the instruction causes a delay, the amount of
> > > > time delayed is called here the physical delay. The physical delay is
> > > > first computed by determining the virtual delay. If
> > > > IA32_UMWAIT_CONTROL[31:2] is zero, the virtual delay is the value in
> > > > EDX:EAX minus the value that RDTSC would return; if
> > > > IA32_UMWAIT_CONTROL[31:2] is not zero, the virtual delay is the minimum
> > > > of that difference and AND(IA32_UMWAIT_CONTROL,FFFFFFFCH).
> > > >
> > > > Because umwait and tpause can put a (psysical) CPU into a power saving
> > > > state, by default we dont't expose it to kvm and enable it only when
> > > > guest CPUID has it.
> > > >
> > > > Detailed information about user wait instructions can be found in the
> > > > latest Intel 64 and IA-32 Architectures Software Developer's Manual.
> > > >
> > > > Co-developed-by: Jingqi Liu <jingqi.liu@xxxxxxxxx>
> > > > Signed-off-by: Jingqi Liu <jingqi.liu@xxxxxxxxx>
> > > > Signed-off-by: Tao Xu <tao3.xu@xxxxxxxxx>
> > > > ---
> > > >
> > > > no changes in v4.
> > > > ---
> > > > Â arch/x86/include/asm/vmx.hÂÂÂÂÂ | 1 +
> > > > Â arch/x86/kvm/cpuid.cÂÂÂÂÂÂÂÂÂÂÂ | 2 +-
> > > > Â arch/x86/kvm/vmx/capabilities.h | 6 ++++++
> > > > Â arch/x86/kvm/vmx/vmx.cÂÂÂÂÂÂÂÂÂ | 4 ++++
> > > > Â 4 files changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> > > > index a39136b0d509..8f00882664d3 100644
> > > > --- a/arch/x86/include/asm/vmx.h
> > > > +++ b/arch/x86/include/asm/vmx.h
> > > > @@ -69,6 +69,7 @@
> > > > Â #define SECONDARY_EXEC_PT_USE_GPAÂÂÂÂÂÂÂ 0x01000000
> > > > Â #define SECONDARY_EXEC_MODE_BASED_EPT_EXECÂÂÂ 0x00400000
> > > > Â #define SECONDARY_EXEC_TSC_SCALINGÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x02000000
> > > > +#define SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSEÂÂÂ 0x04000000
> > > > Â #define PIN_BASED_EXT_INTR_MASKÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00000001
> > > > Â #define PIN_BASED_NMI_EXITINGÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00000008
> > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > > index e18a9f9f65b5..48bd851a6ae5 100644
> > > > --- a/arch/x86/kvm/cpuid.c
> > > > +++ b/arch/x86/kvm/cpuid.c
> > > > @@ -405,7 +405,7 @@ static inline int __do_cpuid_ent(struct 
> > > > kvm_cpuid_entry2 *entry, u32 function,
> > > > ÂÂÂÂÂÂÂÂÂ F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
> > > > ÂÂÂÂÂÂÂÂÂ F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
> > > > ÂÂÂÂÂÂÂÂÂ F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
> > > > -ÂÂÂÂÂÂÂ F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B);
> > > > +ÂÂÂÂÂÂÂ F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/;
> > > > ÂÂÂÂÂ /* cpuid 7.0.edx*/
> > > > ÂÂÂÂÂ const u32 kvm_cpuid_7_0_edx_x86_features =
> > > > diff --git a/arch/x86/kvm/vmx/capabilities.h 
> > > > b/arch/x86/kvm/vmx/capabilities.h
> > > > index d6664ee3d127..fd77e17651b4 100644
> > > > --- a/arch/x86/kvm/vmx/capabilities.h
> > > > +++ b/arch/x86/kvm/vmx/capabilities.h
> > > > @@ -253,6 +253,12 @@ static inline bool cpu_has_vmx_tsc_scaling(void)
> > > > ÂÂÂÂÂÂÂÂÂ SECONDARY_EXEC_TSC_SCALING;
> > > > Â }
> > > > +static inline bool vmx_waitpkg_supported(void)
> > > > +{
> > > > +ÂÂÂ return vmcs_config.cpu_based_2nd_exec_ctrl &
> > > > +ÂÂÂÂÂÂÂ SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
> > >
> > > Shouldn't it be
> > > ÂÂÂÂÂreturn vmx->secondary_exec_control &
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;ÂÂ ?
> >
> > vmx->secondary_exec_control is another way to get 
> > SECONDARY_EXEC_CONTROL but it need to add vmx as input. Use 
> > vmcs_config.cpu_based_2nd_exec_ctrl ÂÂis easy expand to use in other 
> > place.
>
> This patch want to enable waitpkg for guest only when 
> guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG), and it only updates 
> vmx->secondary_exec_control based on guest's cpuid when setup vcpu. But 
> vmcs_config remains unchanged. So using vmcs_config there is wrong.

Xiaoyao, you are right. And MSR_IA32_XSS has the same logic. I will fix 
my patch. And we may need to submit a fix patch for MSR_IA32_XSS.