Re: [PATCH v2 2/2] kvm/vmx: Using hardware cpuid faulting to avoid emulation overhead

2019-03-20 Thread Xiaoyao Li
On Tue, 2019-03-19 at 17:09 -0700, Sean Christopherson wrote:
> On Wed, Mar 20, 2019 at 01:51:28AM +0800, Xiaoyao Li wrote:
> > On Tue, 2019-03-19 at 07:28 -0700, Sean Christopherson wrote:
> > > On Tue, Mar 19, 2019 at 12:37:23PM +0800, Xiaoyao Li wrote:
> > > > On Mon, 2019-03-18 at 09:38 -0700, Sean Christopherson wrote:
> > > > > On Mon, Mar 18, 2019 at 07:43:24PM +0800, Xiaoyao Li wrote:
> > > > > > Current cpuid faulting of guest is purely emulated in kvm, which
> > > > > > exploits
> > > > > > CPUID vm exit to inject #GP to guest. However, if host hardware cpu
> > > > > > has
> > > > > > X86_FEATURE_CPUID_FAULT, we can just use the hardware cpuid faulting
> > > > > > for
> > > > > > guest to avoid the vm exit overhead.
> > > > > 
> > > > > Heh, I obviously didn't look at this patch before responding to patch
> > > > > 1/2.
> > > > > 
> > > > > > Note: cpuid faulting takes higher priority over CPUID instruction vm
> > > > > > exit (Intel SDM vol3.25.1.1).
> > > > > > 
> > > > > > Since cpuid faulting only exists on some Intel's cpu, just apply
> > > > > > this
> > > > > > optimization to vmx.
> > > > > > 
> > > > > > Signed-off-by: Xiaoyao Li 
> > > > > > ---
> > > > > >  arch/x86/include/asm/kvm_host.h |  2 ++
> > > > > >  arch/x86/kvm/vmx/vmx.c  | 19 +++
> > > > > >  arch/x86/kvm/x86.c  | 15 ---
> > > > > >  3 files changed, 29 insertions(+), 7 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/x86/include/asm/kvm_host.h
> > > > > > b/arch/x86/include/asm/kvm_host.h
> > > > > > index ce79d7bfe1fd..14cad587b804 100644
> > > > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > > > @@ -1339,6 +1339,8 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned
> > > > > > long
> > > > > > msw);
> > > > > >  void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
> > > > > >  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
> > > > > >  
> > > > > > +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu,
> > > > > > u64
> > > > > > data);
> > > > > > +
> > > > > >  int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data
> > > > > > *msr);
> > > > > >  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data
> > > > > > *msr);
> > > > > >  
> > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > > > > index 2c59e0209e36..6b413e471dca 100644
> > > > > > --- a/arch/x86/kvm/vmx/vmx.c
> > > > > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > > > > @@ -1037,7 +1037,7 @@ static void pt_guest_exit(struct vcpu_vmx
> > > > > > *vmx)
> > > > > >  
> > > > > >  static void vmx_save_host_cpuid_fault(struct vcpu_vmx *vmx)
> > > > > >  {
> > > > > > -   u64 host_val;
> > > > > > +   u64 host_val, guest_val;
> > > > > >  
> > > > > > if (!boot_cpu_has(X86_FEATURE_CPUID_FAULT))
> > > > > > return;
> > > > > > @@ -1045,10 +1045,12 @@ static void vmx_save_host_cpuid_fault(struct
> > > > > > vcpu_vmx *vmx)
> > > > > > rdmsrl(MSR_MISC_FEATURES_ENABLES, host_val);
> > > > > > vmx->host_msr_misc_features_enables = host_val;
> > > > > >  
> > > > > > -   /* clear cpuid fault bit to avoid it leak to guest */
> > > > > > -   if (host_val & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > > > > > +   guest_val = vmx->vcpu.arch.msr_misc_features_enables;
> > > > > > +
> > > > > > +   /* we can use the hardware cpuid faulting to avoid emulation
> > > > > > overhead */
> > > > > > +   if ((host_val ^ guest_val) &
> > > > > > MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > > > > > wrmsrl(MSR_MISC_FEATURES_ENABLES,
> > > > > > -  host_val &
> > > > > > ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > > > > > +  host_val ^
> > > > > > MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > > > > > }
> > > > > >  }
> > > > > >  
> > > > > > @@ -2057,6 +2059,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
> > > > > > struct
> > > > > > msr_data *msr_info)
> > > > > > else
> > > > > > vmx->pt_desc.guest.addr_a[index / 2] = data;
> > > > > > break;
> > > > > > +   case MSR_MISC_FEATURES_ENABLES:
> > > > > > +   if (!kvm_supported_msr_misc_features_enables(vcpu,
> > > > > > data))
> > > > > > +   return 1;
> > > > > > +   if (boot_cpu_has(X86_FEATURE_CPUID_FAULT)) {
> > > > > > +   if (vmx->loaded_cpu_state)
> > > > > 
> > > > > No need for two separate if statements.  And assuming we're checking
> > > > > the
> > > > > existing shadow value when loading guest/host state, the WRMSR should
> > > > > only be done if the host's value is non-zero.
> > > > 
> > > > I'll combine these two if statements into one.
> > > > 
> > > > I cannot understand why the WRMSR should only be done if the host's
> > > > value is
> > > > non-zero. I think there is no depedency with host's value, if using the
> > > > hardware
> > > > cpuid faulting. We just need to set the 

Re: [PATCH v2 2/2] kvm/vmx: Using hardware cpuid faulting to avoid emulation overhead

2019-03-19 Thread Sean Christopherson
On Wed, Mar 20, 2019 at 01:51:28AM +0800, Xiaoyao Li wrote:
> On Tue, 2019-03-19 at 07:28 -0700, Sean Christopherson wrote:
> > On Tue, Mar 19, 2019 at 12:37:23PM +0800, Xiaoyao Li wrote:
> > > On Mon, 2019-03-18 at 09:38 -0700, Sean Christopherson wrote:
> > > > On Mon, Mar 18, 2019 at 07:43:24PM +0800, Xiaoyao Li wrote:
> > > > > Current cpuid faulting of guest is purely emulated in kvm, which
> > > > > exploits
> > > > > CPUID vm exit to inject #GP to guest. However, if host hardware cpu 
> > > > > has
> > > > > X86_FEATURE_CPUID_FAULT, we can just use the hardware cpuid faulting 
> > > > > for
> > > > > guest to avoid the vm exit overhead.
> > > > 
> > > > Heh, I obviously didn't look at this patch before responding to patch 
> > > > 1/2.
> > > > 
> > > > > Note: cpuid faulting takes higher priority over CPUID instruction vm
> > > > > exit (Intel SDM vol3.25.1.1).
> > > > > 
> > > > > Since cpuid faulting only exists on some Intel's cpu, just apply this
> > > > > optimization to vmx.
> > > > > 
> > > > > Signed-off-by: Xiaoyao Li 
> > > > > ---
> > > > >  arch/x86/include/asm/kvm_host.h |  2 ++
> > > > >  arch/x86/kvm/vmx/vmx.c  | 19 +++
> > > > >  arch/x86/kvm/x86.c  | 15 ---
> > > > >  3 files changed, 29 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/arch/x86/include/asm/kvm_host.h
> > > > > b/arch/x86/include/asm/kvm_host.h
> > > > > index ce79d7bfe1fd..14cad587b804 100644
> > > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > > @@ -1339,6 +1339,8 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned 
> > > > > long
> > > > > msw);
> > > > >  void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
> > > > >  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
> > > > >  
> > > > > +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, 
> > > > > u64
> > > > > data);
> > > > > +
> > > > >  int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> > > > >  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> > > > >  
> > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > > > index 2c59e0209e36..6b413e471dca 100644
> > > > > --- a/arch/x86/kvm/vmx/vmx.c
> > > > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > > > @@ -1037,7 +1037,7 @@ static void pt_guest_exit(struct vcpu_vmx *vmx)
> > > > >  
> > > > >  static void vmx_save_host_cpuid_fault(struct vcpu_vmx *vmx)
> > > > >  {
> > > > > - u64 host_val;
> > > > > + u64 host_val, guest_val;
> > > > >  
> > > > >   if (!boot_cpu_has(X86_FEATURE_CPUID_FAULT))
> > > > >   return;
> > > > > @@ -1045,10 +1045,12 @@ static void vmx_save_host_cpuid_fault(struct
> > > > > vcpu_vmx *vmx)
> > > > >   rdmsrl(MSR_MISC_FEATURES_ENABLES, host_val);
> > > > >   vmx->host_msr_misc_features_enables = host_val;
> > > > >  
> > > > > - /* clear cpuid fault bit to avoid it leak to guest */
> > > > > - if (host_val & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > > > > + guest_val = vmx->vcpu.arch.msr_misc_features_enables;
> > > > > +
> > > > > + /* we can use the hardware cpuid faulting to avoid emulation
> > > > > overhead */
> > > > > + if ((host_val ^ guest_val) &
> > > > > MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > > > >   wrmsrl(MSR_MISC_FEATURES_ENABLES,
> > > > > -host_val &
> > > > > ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > > > > +host_val ^
> > > > > MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > > > >   }
> > > > >  }
> > > > >  
> > > > > @@ -2057,6 +2059,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
> > > > > struct
> > > > > msr_data *msr_info)
> > > > >   else
> > > > >   vmx->pt_desc.guest.addr_a[index / 2] = data;
> > > > >   break;
> > > > > + case MSR_MISC_FEATURES_ENABLES:
> > > > > + if (!kvm_supported_msr_misc_features_enables(vcpu,
> > > > > data))
> > > > > + return 1;
> > > > > + if (boot_cpu_has(X86_FEATURE_CPUID_FAULT)) {
> > > > > + if (vmx->loaded_cpu_state)
> > > > 
> > > > No need for two separate if statements.  And assuming we're checking the
> > > > existing shadow value when loading guest/host state, the WRMSR should
> > > > only be done if the host's value is non-zero.
> > > 
> > > I'll combine these two if statements into one.
> > > 
> > > I cannot understand why the WRMSR should only be done if the host's value 
> > > is
> > > non-zero. I think there is no depedency with host's value, if using the
> > > hardware
> > > cpuid faulting. We just need to set the value to real hardware MSR.
> > 
> > What I was trying to say in patch 1/2 regarding save/restore, is that I
> > don't think it is worthwhile to voluntarily switch hardware's value.  In
> > other words, do the WRMSR if and only if it's absolutely necessary.  And
> > that means 

Re: [PATCH v2 2/2] kvm/vmx: Using hardware cpuid faulting to avoid emulation overhead

2019-03-19 Thread Xiaoyao Li
On Tue, 2019-03-19 at 07:28 -0700, Sean Christopherson wrote:
> On Tue, Mar 19, 2019 at 12:37:23PM +0800, Xiaoyao Li wrote:
> > On Mon, 2019-03-18 at 09:38 -0700, Sean Christopherson wrote:
> > > On Mon, Mar 18, 2019 at 07:43:24PM +0800, Xiaoyao Li wrote:
> > > > Current cpuid faulting of guest is purely emulated in kvm, which
> > > > exploits
> > > > CPUID vm exit to inject #GP to guest. However, if host hardware cpu has
> > > > X86_FEATURE_CPUID_FAULT, we can just use the hardware cpuid faulting for
> > > > guest to avoid the vm exit overhead.
> > > 
> > > Heh, I obviously didn't look at this patch before responding to patch 1/2.
> > > 
> > > > Note: cpuid faulting takes higher priority over CPUID instruction vm
> > > > exit (Intel SDM vol3.25.1.1).
> > > > 
> > > > Since cpuid faulting only exists on some Intel's cpu, just apply this
> > > > optimization to vmx.
> > > > 
> > > > Signed-off-by: Xiaoyao Li 
> > > > ---
> > > >  arch/x86/include/asm/kvm_host.h |  2 ++
> > > >  arch/x86/kvm/vmx/vmx.c  | 19 +++
> > > >  arch/x86/kvm/x86.c  | 15 ---
> > > >  3 files changed, 29 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/include/asm/kvm_host.h
> > > > b/arch/x86/include/asm/kvm_host.h
> > > > index ce79d7bfe1fd..14cad587b804 100644
> > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > @@ -1339,6 +1339,8 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long
> > > > msw);
> > > >  void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
> > > >  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
> > > >  
> > > > +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64
> > > > data);
> > > > +
> > > >  int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> > > >  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> > > >  
> > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > > index 2c59e0209e36..6b413e471dca 100644
> > > > --- a/arch/x86/kvm/vmx/vmx.c
> > > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > > @@ -1037,7 +1037,7 @@ static void pt_guest_exit(struct vcpu_vmx *vmx)
> > > >  
> > > >  static void vmx_save_host_cpuid_fault(struct vcpu_vmx *vmx)
> > > >  {
> > > > -   u64 host_val;
> > > > +   u64 host_val, guest_val;
> > > >  
> > > > if (!boot_cpu_has(X86_FEATURE_CPUID_FAULT))
> > > > return;
> > > > @@ -1045,10 +1045,12 @@ static void vmx_save_host_cpuid_fault(struct
> > > > vcpu_vmx *vmx)
> > > > rdmsrl(MSR_MISC_FEATURES_ENABLES, host_val);
> > > > vmx->host_msr_misc_features_enables = host_val;
> > > >  
> > > > -   /* clear cpuid fault bit to avoid it leak to guest */
> > > > -   if (host_val & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > > > +   guest_val = vmx->vcpu.arch.msr_misc_features_enables;
> > > > +
> > > > +   /* we can use the hardware cpuid faulting to avoid emulation
> > > > overhead */
> > > > +   if ((host_val ^ guest_val) &
> > > > MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > > > wrmsrl(MSR_MISC_FEATURES_ENABLES,
> > > > -  host_val &
> > > > ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > > > +  host_val ^
> > > > MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > > > }
> > > >  }
> > > >  
> > > > @@ -2057,6 +2059,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
> > > > struct
> > > > msr_data *msr_info)
> > > > else
> > > > vmx->pt_desc.guest.addr_a[index / 2] = data;
> > > > break;
> > > > +   case MSR_MISC_FEATURES_ENABLES:
> > > > +   if (!kvm_supported_msr_misc_features_enables(vcpu,
> > > > data))
> > > > +   return 1;
> > > > +   if (boot_cpu_has(X86_FEATURE_CPUID_FAULT)) {
> > > > +   if (vmx->loaded_cpu_state)
> > > 
> > > No need for two separate if statements.  And assuming we're checking the
> > > existing shadow value when loading guest/host state, the WRMSR should
> > > only be done if the host's value is non-zero.
> > 
> > I'll combine these two if statements into one.
> > 
> > I cannot understand why the WRMSR should only be done if the host's value is
> > non-zero. I think there is no depedency with host's value, if using the
> > hardware
> > cpuid faulting. We just need to set the value to real hardware MSR.
> 
> What I was trying to say in patch 1/2 regarding save/restore, is that I
> don't think it is worthwhile to voluntarily switch hardware's value.  In
> other words, do the WRMSR if and only if it's absolutely necessary.  And
> that means only installing the guest's value if the host's value is
> non-zero and host_val != guest_val.  If the host's value is zero, then
> the guest's value is irrelevant as CPUID faulting behavior will naturally
> be taken care of when intercepting CPUID.  And for obvious reasons the
> 

Re: [PATCH v2 2/2] kvm/vmx: Using hardware cpuid faulting to avoid emulation overhead

2019-03-19 Thread Sean Christopherson
On Tue, Mar 19, 2019 at 12:37:23PM +0800, Xiaoyao Li wrote:
> On Mon, 2019-03-18 at 09:38 -0700, Sean Christopherson wrote:
> > On Mon, Mar 18, 2019 at 07:43:24PM +0800, Xiaoyao Li wrote:
> > > Current cpuid faulting of guest is purely emulated in kvm, which exploits
> > > CPUID vm exit to inject #GP to guest. However, if host hardware cpu has
> > > X86_FEATURE_CPUID_FAULT, we can just use the hardware cpuid faulting for
> > > guest to avoid the vm exit overhead.
> > 
> > Heh, I obviously didn't look at this patch before responding to patch 1/2.
> > 
> > > Note: cpuid faulting takes higher priority over CPUID instruction vm
> > > exit (Intel SDM vol3.25.1.1).
> > > 
> > > Since cpuid faulting only exists on some Intel's cpu, just apply this
> > > optimization to vmx.
> > > 
> > > Signed-off-by: Xiaoyao Li 
> > > ---
> > >  arch/x86/include/asm/kvm_host.h |  2 ++
> > >  arch/x86/kvm/vmx/vmx.c  | 19 +++
> > >  arch/x86/kvm/x86.c  | 15 ---
> > >  3 files changed, 29 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/kvm_host.h
> > > b/arch/x86/include/asm/kvm_host.h
> > > index ce79d7bfe1fd..14cad587b804 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -1339,6 +1339,8 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long
> > > msw);
> > >  void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
> > >  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
> > >  
> > > +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64
> > > data);
> > > +
> > >  int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> > >  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> > >  
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 2c59e0209e36..6b413e471dca 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -1037,7 +1037,7 @@ static void pt_guest_exit(struct vcpu_vmx *vmx)
> > >  
> > >  static void vmx_save_host_cpuid_fault(struct vcpu_vmx *vmx)
> > >  {
> > > - u64 host_val;
> > > + u64 host_val, guest_val;
> > >  
> > >   if (!boot_cpu_has(X86_FEATURE_CPUID_FAULT))
> > >   return;
> > > @@ -1045,10 +1045,12 @@ static void vmx_save_host_cpuid_fault(struct
> > > vcpu_vmx *vmx)
> > >   rdmsrl(MSR_MISC_FEATURES_ENABLES, host_val);
> > >   vmx->host_msr_misc_features_enables = host_val;
> > >  
> > > - /* clear cpuid fault bit to avoid it leak to guest */
> > > - if (host_val & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > > + guest_val = vmx->vcpu.arch.msr_misc_features_enables;
> > > +
> > > + /* we can use the hardware cpuid faulting to avoid emulation overhead */
> > > + if ((host_val ^ guest_val) & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > >   wrmsrl(MSR_MISC_FEATURES_ENABLES,
> > > -host_val & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > > +host_val ^ MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > >   }
> > >  }
> > >  
> > > @@ -2057,6 +2059,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, 
> > > struct
> > > msr_data *msr_info)
> > >   else
> > >   vmx->pt_desc.guest.addr_a[index / 2] = data;
> > >   break;
> > > + case MSR_MISC_FEATURES_ENABLES:
> > > + if (!kvm_supported_msr_misc_features_enables(vcpu, data))
> > > + return 1;
> > > + if (boot_cpu_has(X86_FEATURE_CPUID_FAULT)) {
> > > + if (vmx->loaded_cpu_state)
> > 
> > No need for two separate if statements.  And assuming we're checking the
> > existing shadow value when loading guest/host state, the WRMSR should
> > only be done if the host's value is non-zero.
> 
> I'll combine these two if statements into one.
> 
> I cannot understand why the WRMSR should only be done if the host's value is
> non-zero. I think there is no depedency with host's value, if using the 
> hardware
> cpuid faulting. We just need to set the value to real hardware MSR.

What I was trying to say in patch 1/2 regarding save/restore, is that I
don't think it is worthwhile to voluntarily switch hardware's value.  In
other words, do the WRMSR if and only if it's absolutely necessary.  And
that means only installing the guest's value if the host's value is
non-zero and host_val != guest_val.  If the host's value is zero, then
the guest's value is irrelevant as CPUID faulting behavior will naturally
be taken care of when intercepting CPUID.  And for obvious reasons the
WRMSR can be skipped if host and guest want the same value.

As it pertains to this code, we should not modify any hardware value that
isn't saved/restored.  If we go with the above approach, then we're only
switching the hardware MSR when the host's value is non-zero.  Ergo this
code should not write the actual MSR if the host value is zero as the MSR
will not be restored to the host's value when putting the vCPU.

> > > +  

Re: [PATCH v2 2/2] kvm/vmx: Using hardware cpuid faulting to avoid emulation overhead

2019-03-18 Thread Xiaoyao Li
On Mon, 2019-03-18 at 09:38 -0700, Sean Christopherson wrote:
> On Mon, Mar 18, 2019 at 07:43:24PM +0800, Xiaoyao Li wrote:
> > Current cpuid faulting of guest is purely emulated in kvm, which exploits
> > CPUID vm exit to inject #GP to guest. However, if host hardware cpu has
> > X86_FEATURE_CPUID_FAULT, we can just use the hardware cpuid faulting for
> > guest to avoid the vm exit overhead.
> 
> Heh, I obviously didn't look at this patch before responding to patch 1/2.
> 
> > Note: cpuid faulting takes higher priority over CPUID instruction vm
> > exit (Intel SDM vol3.25.1.1).
> > 
> > Since cpuid faulting only exists on some Intel's cpu, just apply this
> > optimization to vmx.
> > 
> > Signed-off-by: Xiaoyao Li 
> > ---
> >  arch/x86/include/asm/kvm_host.h |  2 ++
> >  arch/x86/kvm/vmx/vmx.c  | 19 +++
> >  arch/x86/kvm/x86.c  | 15 ---
> >  3 files changed, 29 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h
> > b/arch/x86/include/asm/kvm_host.h
> > index ce79d7bfe1fd..14cad587b804 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1339,6 +1339,8 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long
> > msw);
> >  void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
> >  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
> >  
> > +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64
> > data);
> > +
> >  int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> >  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> >  
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 2c59e0209e36..6b413e471dca 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -1037,7 +1037,7 @@ static void pt_guest_exit(struct vcpu_vmx *vmx)
> >  
> >  static void vmx_save_host_cpuid_fault(struct vcpu_vmx *vmx)
> >  {
> > -   u64 host_val;
> > +   u64 host_val, guest_val;
> >  
> > if (!boot_cpu_has(X86_FEATURE_CPUID_FAULT))
> > return;
> > @@ -1045,10 +1045,12 @@ static void vmx_save_host_cpuid_fault(struct
> > vcpu_vmx *vmx)
> > rdmsrl(MSR_MISC_FEATURES_ENABLES, host_val);
> > vmx->host_msr_misc_features_enables = host_val;
> >  
> > -   /* clear cpuid fault bit to avoid it leak to guest */
> > -   if (host_val & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > +   guest_val = vmx->vcpu.arch.msr_misc_features_enables;
> > +
> > +   /* we can use the hardware cpuid faulting to avoid emulation overhead */
> > +   if ((host_val ^ guest_val) & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> > wrmsrl(MSR_MISC_FEATURES_ENABLES,
> > -  host_val & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > +  host_val ^ MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> > }
> >  }
> >  
> > @@ -2057,6 +2059,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct
> > msr_data *msr_info)
> > else
> > vmx->pt_desc.guest.addr_a[index / 2] = data;
> > break;
> > +   case MSR_MISC_FEATURES_ENABLES:
> > +   if (!kvm_supported_msr_misc_features_enables(vcpu, data))
> > +   return 1;
> > +   if (boot_cpu_has(X86_FEATURE_CPUID_FAULT)) {
> > +   if (vmx->loaded_cpu_state)
> 
> No need for two separate if statements.  And assuming we're checking the
> existing shadow value when loading guest/host state, the WRMSR should
> only be done if the host's value is non-zero.

I'll combine these two if statements into one.

I cannot understand why the WRMSR should only be done if the host's value is
non-zero. I think there is no depedency with host's value, if using the hardware
cpuid faulting. We just need to set the value to real hardware MSR.

> > +   wrmsrl(MSR_MISC_FEATURES_ENABLES, data);
> > +   }
> > +   vcpu->arch.msr_misc_features_enables = data;
> > +   break;
> > case MSR_TSC_AUX:
> > if (!msr_info->host_initiated &&
> > !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 434ec113cc79..33a8c95b2f2e 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -2449,6 +2449,17 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
> > >arch.st.steal, sizeof(struct kvm_steal_time));
> >  }
> >  
> > +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64
> > data)
> > +{
> > +   if (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT ||
> > +   (data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
> > +!supports_cpuid_fault(vcpu)))
> > +   return 0;
> > +   else
> > +   return 1;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_supported_msr_misc_features_enables);
> > +
> >  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  {
> > bool pr = false;
> > @@ 

Re: [PATCH v2 2/2] kvm/vmx: Using hardware cpuid faulting to avoid emulation overhead

2019-03-18 Thread Sean Christopherson
On Mon, Mar 18, 2019 at 07:43:24PM +0800, Xiaoyao Li wrote:
> Current cpuid faulting of guest is purely emulated in kvm, which exploits
> CPUID vm exit to inject #GP to guest. However, if host hardware cpu has
> X86_FEATURE_CPUID_FAULT, we can just use the hardware cpuid faulting for
> guest to avoid the vm exit overhead.

Heh, I obviously didn't look at this patch before responding to patch 1/2.

> Note: cpuid faulting takes higher priority over CPUID instruction vm
> exit (Intel SDM vol3.25.1.1).
> 
> Since cpuid faulting only exists on some Intel's cpu, just apply this
> optimization to vmx.
> 
> Signed-off-by: Xiaoyao Li 
> ---
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/vmx/vmx.c  | 19 +++
>  arch/x86/kvm/x86.c  | 15 ---
>  3 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ce79d7bfe1fd..14cad587b804 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1339,6 +1339,8 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw);
>  void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
>  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
>  
> +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64 data);
> +
>  int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
>  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
>  
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 2c59e0209e36..6b413e471dca 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1037,7 +1037,7 @@ static void pt_guest_exit(struct vcpu_vmx *vmx)
>  
>  static void vmx_save_host_cpuid_fault(struct vcpu_vmx *vmx)
>  {
> - u64 host_val;
> + u64 host_val, guest_val;
>  
>   if (!boot_cpu_has(X86_FEATURE_CPUID_FAULT))
>   return;
> @@ -1045,10 +1045,12 @@ static void vmx_save_host_cpuid_fault(struct vcpu_vmx 
> *vmx)
>   rdmsrl(MSR_MISC_FEATURES_ENABLES, host_val);
>   vmx->host_msr_misc_features_enables = host_val;
>  
> - /* clear cpuid fault bit to avoid it leak to guest */
> - if (host_val & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
> + guest_val = vmx->vcpu.arch.msr_misc_features_enables;
> +
> + /* we can use the hardware cpuid faulting to avoid emulation overhead */
> + if ((host_val ^ guest_val) & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
>   wrmsrl(MSR_MISC_FEATURES_ENABLES,
> -host_val & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
> +host_val ^ MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
>   }
>  }
>  
> @@ -2057,6 +2059,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>   else
>   vmx->pt_desc.guest.addr_a[index / 2] = data;
>   break;
> + case MSR_MISC_FEATURES_ENABLES:
> + if (!kvm_supported_msr_misc_features_enables(vcpu, data))
> + return 1;
> + if (boot_cpu_has(X86_FEATURE_CPUID_FAULT)) {
> + if (vmx->loaded_cpu_state)

No need for two separate if statements.  And assuming we're checking the
existing shadow value when loading guest/host state, the WRMSR should
only be done if the host's value is non-zero.

> + wrmsrl(MSR_MISC_FEATURES_ENABLES, data);
> + }
> + vcpu->arch.msr_misc_features_enables = data;
> + break;
>   case MSR_TSC_AUX:
>   if (!msr_info->host_initiated &&
>   !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 434ec113cc79..33a8c95b2f2e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2449,6 +2449,17 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
>   >arch.st.steal, sizeof(struct kvm_steal_time));
>  }
>  
> +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64 data)
> +{
> + if (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT ||
> + (data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
> +  !supports_cpuid_fault(vcpu)))
> + return 0;
> + else
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(kvm_supported_msr_misc_features_enables);
> +
>  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  {
>   bool pr = false;
> @@ -2669,9 +2680,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>   vcpu->arch.msr_platform_info = data;
>   break;
>   case MSR_MISC_FEATURES_ENABLES:
> - if (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT ||
> - (data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
> -  !supports_cpuid_fault(vcpu)))
> + if (!kvm_supported_msr_misc_features_enables(vcpu, data))
>   

[PATCH v2 2/2] kvm/vmx: Using hardware cpuid faulting to avoid emulation overhead

2019-03-18 Thread Xiaoyao Li
Current cpuid faulting of guest is purely emulated in kvm, which exploits
CPUID vm exit to inject #GP to guest. However, if host hardware cpu has
X86_FEATURE_CPUID_FAULT, we can just use the hardware cpuid faulting for
guest to avoid the vm exit overhead.

Note: cpuid faulting takes higher priority over CPUID instruction vm
exit (Intel SDM vol3.25.1.1).

Since cpuid faulting only exists on some Intel's cpu, just apply this
optimization to vmx.

Signed-off-by: Xiaoyao Li 
---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/vmx/vmx.c  | 19 +++
 arch/x86/kvm/x86.c  | 15 ---
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ce79d7bfe1fd..14cad587b804 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1339,6 +1339,8 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw);
 void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
 int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
 
+int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64 data);
+
 int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 2c59e0209e36..6b413e471dca 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1037,7 +1037,7 @@ static void pt_guest_exit(struct vcpu_vmx *vmx)
 
 static void vmx_save_host_cpuid_fault(struct vcpu_vmx *vmx)
 {
-   u64 host_val;
+   u64 host_val, guest_val;
 
if (!boot_cpu_has(X86_FEATURE_CPUID_FAULT))
return;
@@ -1045,10 +1045,12 @@ static void vmx_save_host_cpuid_fault(struct vcpu_vmx 
*vmx)
rdmsrl(MSR_MISC_FEATURES_ENABLES, host_val);
vmx->host_msr_misc_features_enables = host_val;
 
-   /* clear cpuid fault bit to avoid it leak to guest */
-   if (host_val & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
+   guest_val = vmx->vcpu.arch.msr_misc_features_enables;
+
+   /* we can use the hardware cpuid faulting to avoid emulation overhead */
+   if ((host_val ^ guest_val) & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) {
wrmsrl(MSR_MISC_FEATURES_ENABLES,
-  host_val & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
+  host_val ^ MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
}
 }
 
@@ -2057,6 +2059,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
else
vmx->pt_desc.guest.addr_a[index / 2] = data;
break;
+   case MSR_MISC_FEATURES_ENABLES:
+   if (!kvm_supported_msr_misc_features_enables(vcpu, data))
+   return 1;
+   if (boot_cpu_has(X86_FEATURE_CPUID_FAULT)) {
+   if (vmx->loaded_cpu_state)
+   wrmsrl(MSR_MISC_FEATURES_ENABLES, data);
+   }
+   vcpu->arch.msr_misc_features_enables = data;
+   break;
case MSR_TSC_AUX:
if (!msr_info->host_initiated &&
!guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 434ec113cc79..33a8c95b2f2e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2449,6 +2449,17 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
>arch.st.steal, sizeof(struct kvm_steal_time));
 }
 
+int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64 data)
+{
+   if (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT ||
+   (data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
+!supports_cpuid_fault(vcpu)))
+   return 0;
+   else
+   return 1;
+}
+EXPORT_SYMBOL_GPL(kvm_supported_msr_misc_features_enables);
+
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
bool pr = false;
@@ -2669,9 +2680,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
vcpu->arch.msr_platform_info = data;
break;
case MSR_MISC_FEATURES_ENABLES:
-   if (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT ||
-   (data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
-!supports_cpuid_fault(vcpu)))
+   if (!kvm_supported_msr_misc_features_enables(vcpu, data))
return 1;
vcpu->arch.msr_misc_features_enables = data;
break;
-- 
2.19.1