[PATCH v2 0/3] KVM: VMX: Some refactor of VMX vmcs and msr bitmap

[PATCH v2 0/3] KVM: VMX: Some refactor of VMX vmcs and msr bitmap

[Xiaoyao Li]

Remove the vcpu creation refactor and FPU allocation cleanup from v1, since
I need more time to invest Sean's suggestion.

This series add one patch to move the vmcs reset from vcpu_reset, based on
Krish's suggestion.

Xiaoyao Li (3):
  KVM: VMX: Move vmcs related resetting out of vmx_vcpu_reset()
  KVM: VMX: Rename {vmx,nested_vmx}_vcpu_setup() and minor cleanup
  KVM: VMX: Some minor refactor of MSR bitmap

 arch/x86/kvm/vmx/nested.c |   2 +-
 arch/x86/kvm/vmx/nested.h |   2 +-
 arch/x86/kvm/vmx/vmx.c    | 173 ++++++++++++++++++++------------------
 3 files changed, 91 insertions(+), 86 deletions(-)

-- 
2.19.1

[PATCH v2 1/3] KVM: VMX: Move vmcs related resetting out of vmx_vcpu_reset()

[Xiaoyao Li]

Move vmcs related codes into a new function vmx_vmcs_reset() from
vmx_vcpu_reset(). So that it's more clearer which data is related with
vmcs and can be held in vmcs.

Suggested-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 65 ++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e660e28e9ae0..ef567df344bf 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4271,33 +4271,11 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
 	}
 }
 
-static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
+static void vmx_vmcs_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	struct msr_data apic_base_msr;
 	u64 cr0;
 
-	vmx->rmode.vm86_active = 0;
-	vmx->spec_ctrl = 0;
-
-	vmx->msr_ia32_umwait_control = 0;
-
-	vcpu->arch.microcode_version = 0x100000000ULL;
-	vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val();
-	vmx->hv_deadline_tsc = -1;
-	kvm_set_cr8(vcpu, 0);
-
-	if (!init_event) {
-		apic_base_msr.data = APIC_DEFAULT_PHYS_BASE |
-				     MSR_IA32_APICBASE_ENABLE;
-		if (kvm_vcpu_is_reset_bsp(vcpu))
-			apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
-		apic_base_msr.host_initiated = true;
-		kvm_set_apic_base(vcpu, &apic_base_msr);
-	}
-
-	vmx_segment_cache_clear(vmx);
-
 	seg_setup(VCPU_SREG_CS);
 	vmcs_write16(GUEST_CS_SELECTOR, 0xf000);
 	vmcs_writel(GUEST_CS_BASE, 0xffff0000ul);
@@ -4340,8 +4318,6 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	if (kvm_mpx_supported())
 		vmcs_write64(GUEST_BNDCFGS, 0);
 
-	setup_msrs(vmx);
-
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);  /* 22.2.1 */
 
 	if (cpu_has_vmx_tpr_shadow() && !init_event) {
@@ -4357,19 +4333,52 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	if (vmx->vpid != 0)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
 
+	vpid_sync_context(vmx->vpid);
+
 	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
-	vmx->vcpu.arch.cr0 = cr0;
+	vcpu->arch.cr0 = cr0;
 	vmx_set_cr0(vcpu, cr0); /* enter rmode */
 	vmx_set_cr4(vcpu, 0);
-	vmx_set_efer(vcpu, 0);
 
 	update_exception_bitmap(vcpu);
 
-	vpid_sync_context(vmx->vpid);
 	if (init_event)
 		vmx_clear_hlt(vcpu);
 }
 
+static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct msr_data apic_base_msr;
+
+	vmx->rmode.vm86_active = 0;
+	vmx->spec_ctrl = 0;
+
+	vmx->msr_ia32_umwait_control = 0;
+
+	vcpu->arch.microcode_version = 0x100000000ULL;
+	kvm_rdx_write(vcpu, get_rdx_init_val());
+	vmx->hv_deadline_tsc = -1;
+	kvm_set_cr8(vcpu, 0);
+
+	if (!init_event) {
+		apic_base_msr.data = APIC_DEFAULT_PHYS_BASE |
+				     MSR_IA32_APICBASE_ENABLE;
+		if (kvm_vcpu_is_reset_bsp(vcpu))
+			apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
+		apic_base_msr.host_initiated = true;
+		kvm_set_apic_base(vcpu, &apic_base_msr);
+	}
+
+	vmx_segment_cache_clear(vmx);
+
+	setup_msrs(vmx);
+
+	vmx_set_efer(vcpu, 0);
+
+	vmx_vmcs_reset(vcpu, init_event);
+}
+
 static void enable_irq_window(struct kvm_vcpu *vcpu)
 {
 	exec_controls_setbit(to_vmx(vcpu), CPU_BASED_VIRTUAL_INTR_PENDING);
-- 
2.19.1

Re: [PATCH v2 1/3] KVM: VMX: Move vmcs related resetting out of vmx_vcpu_reset()

[Sean Christopherson]

On Fri, Oct 18, 2019 at 05:37:21PM +0800, Xiaoyao Li wrote:
> Move vmcs related codes into a new function vmx_vmcs_reset() from
> vmx_vcpu_reset(). So that it's more clearer which data is related with
> vmcs and can be held in vmcs.
> 
> Suggested-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 65 ++++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index e660e28e9ae0..ef567df344bf 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4271,33 +4271,11 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
>  	}
>  }
>  
> -static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> +static void vmx_vmcs_reset(struct kvm_vcpu *vcpu, bool init_event)

I'd strongly prefer to keep the existing code.  For me, "vmcs_reset" means
zeroing out the VMCS, i.e. reset the VMCS to a virgin state.  "vcpu_reset"
means exactly that, stuff vCPU state to emulate RESET/INIT.

And the split is arbitrary and funky, e.g. EFER is integrated into the
VMCS on all recent CPUs, but here it's handled in vcpu_reset.  

>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> -	struct msr_data apic_base_msr;
>  	u64 cr0;
>  
> -	vmx->rmode.vm86_active = 0;
> -	vmx->spec_ctrl = 0;
> -
> -	vmx->msr_ia32_umwait_control = 0;
> -
> -	vcpu->arch.microcode_version = 0x100000000ULL;
> -	vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val();
> -	vmx->hv_deadline_tsc = -1;
> -	kvm_set_cr8(vcpu, 0);
> -
> -	if (!init_event) {
> -		apic_base_msr.data = APIC_DEFAULT_PHYS_BASE |
> -				     MSR_IA32_APICBASE_ENABLE;
> -		if (kvm_vcpu_is_reset_bsp(vcpu))
> -			apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
> -		apic_base_msr.host_initiated = true;
> -		kvm_set_apic_base(vcpu, &apic_base_msr);
> -	}
> -
> -	vmx_segment_cache_clear(vmx);
> -
>  	seg_setup(VCPU_SREG_CS);
>  	vmcs_write16(GUEST_CS_SELECTOR, 0xf000);
>  	vmcs_writel(GUEST_CS_BASE, 0xffff0000ul);
> @@ -4340,8 +4318,6 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	if (kvm_mpx_supported())
>  		vmcs_write64(GUEST_BNDCFGS, 0);
>  
> -	setup_msrs(vmx);
> -
>  	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);  /* 22.2.1 */
>  
>  	if (cpu_has_vmx_tpr_shadow() && !init_event) {
> @@ -4357,19 +4333,52 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	if (vmx->vpid != 0)
>  		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>  
> +	vpid_sync_context(vmx->vpid);
> +
>  	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
> -	vmx->vcpu.arch.cr0 = cr0;
> +	vcpu->arch.cr0 = cr0;
>  	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>  	vmx_set_cr4(vcpu, 0);
> -	vmx_set_efer(vcpu, 0);
>  
>  	update_exception_bitmap(vcpu);
>  
> -	vpid_sync_context(vmx->vpid);
>  	if (init_event)
>  		vmx_clear_hlt(vcpu);
>  }
>  
> +static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> +{
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	struct msr_data apic_base_msr;
> +
> +	vmx->rmode.vm86_active = 0;
> +	vmx->spec_ctrl = 0;
> +
> +	vmx->msr_ia32_umwait_control = 0;
> +
> +	vcpu->arch.microcode_version = 0x100000000ULL;
> +	kvm_rdx_write(vcpu, get_rdx_init_val());
> +	vmx->hv_deadline_tsc = -1;
> +	kvm_set_cr8(vcpu, 0);
> +
> +	if (!init_event) {
> +		apic_base_msr.data = APIC_DEFAULT_PHYS_BASE |
> +				     MSR_IA32_APICBASE_ENABLE;
> +		if (kvm_vcpu_is_reset_bsp(vcpu))
> +			apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
> +		apic_base_msr.host_initiated = true;
> +		kvm_set_apic_base(vcpu, &apic_base_msr);
> +	}
> +
> +	vmx_segment_cache_clear(vmx);
> +
> +	setup_msrs(vmx);
> +
> +	vmx_set_efer(vcpu, 0);

Setting EFER before CR0/CR4 is a functional change, and likely wrong, e.g.
vmx_set_cr0() queries EFER_LME to trigger exit_lmode() if INIT/RESET is
received while the vCPU is in long mode.

> +	vmx_vmcs_reset(vcpu, init_event);
> +}
> +
>  static void enable_irq_window(struct kvm_vcpu *vcpu)
>  {
>  	exec_controls_setbit(to_vmx(vcpu), CPU_BASED_VIRTUAL_INTR_PENDING);
> -- 
> 2.19.1
> 

Re: [PATCH v2 1/3] KVM: VMX: Move vmcs related resetting out of vmx_vcpu_reset()

[Xiaoyao Li]

On 10/19/2019 12:57 AM, Sean Christopherson wrote:
> On Fri, Oct 18, 2019 at 05:37:21PM +0800, Xiaoyao Li wrote:
>> Move vmcs related codes into a new function vmx_vmcs_reset() from
>> vmx_vcpu_reset(). So that it's more clearer which data is related with
>> vmcs and can be held in vmcs.
>>
>> Suggested-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 65 ++++++++++++++++++++++++------------------
>>   1 file changed, 37 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index e660e28e9ae0..ef567df344bf 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -4271,33 +4271,11 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
>>   	}
>>   }
>>   
>> -static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>> +static void vmx_vmcs_reset(struct kvm_vcpu *vcpu, bool init_event)
> 
> I'd strongly prefer to keep the existing code.  For me, "vmcs_reset" means
> zeroing out the VMCS, i.e. reset the VMCS to a virgin state.  "vcpu_reset"
> means exactly that, stuff vCPU state to emulate RESET/INIT.
> 
> And the split is arbitrary and funky, e.g. EFER is integrated into the
> VMCS on all recent CPUs, but here it's handled in vcpu_reset.
>

I left EFER in vcpu_reset() because it doesn't directly lead to a 
vmcs_write in vmx_set_efer().

OK. I'll drop this patch.

[PATCH v2 2/3] KVM: VMX: Rename {vmx,nested_vmx}_vcpu_setup() and minor cleanup

[Xiaoyao Li]

Rename {vmx,nested_vmx}_vcpu_setup() to {vmx,nested_vmx}_vmcs_setup,
to match what they really do.

Aslo remove the vmcs unrelated codes to vmx_vcpu_create().

The initialization of vmx->hv_deadline_tsc can be removed here, because
it will be called in vmx_vcpu_reset() as the flow:

kvm_arch_vcpu_setup()
  -> kvm_vcpu_reset()
       -> vmx_vcpu_reset()

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes in v2:
  - move out the vmcs unrelated codes
---
 arch/x86/kvm/vmx/nested.c |  2 +-
 arch/x86/kvm/vmx/nested.h |  2 +-
 arch/x86/kvm/vmx/vmx.c    | 45 +++++++++++++++++----------------------
 3 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 5e231da00310..7935422d311f 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5768,7 +5768,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 	return ret;
 }
 
-void nested_vmx_vcpu_setup(void)
+void nested_vmx_vmcs_setup(void)
 {
 	if (enable_shadow_vmcs) {
 		vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap));
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 187d39bf0bf1..2be1ba7482c9 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -11,7 +11,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
 				bool apicv);
 void nested_vmx_hardware_unsetup(void);
 __init int nested_vmx_hardware_setup(int (*exit_handlers[])(struct kvm_vcpu *));
-void nested_vmx_vcpu_setup(void);
+void nested_vmx_vmcs_setup(void);
 void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu);
 int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry);
 bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index ef567df344bf..b083316a598d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4161,15 +4161,10 @@ static void ept_set_mmio_spte_mask(void)
 
 #define VMX_XSS_EXIT_BITMAP 0
 
-/*
- * Sets up the vmcs for emulated real mode.
- */
-static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
+static void vmx_vmcs_setup(struct vcpu_vmx *vmx)
 {
-	int i;
-
 	if (nested)
-		nested_vmx_vcpu_setup();
+		nested_vmx_vmcs_setup();
 
 	if (cpu_has_vmx_msr_bitmap())
 		vmcs_write64(MSR_BITMAP, __pa(vmx->vmcs01.msr_bitmap));
@@ -4178,7 +4173,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
 
 	/* Control */
 	pin_controls_set(vmx, vmx_pin_based_exec_ctrl(vmx));
-	vmx->hv_deadline_tsc = -1;
 
 	exec_controls_set(vmx, vmx_exec_control(vmx));
 
@@ -4227,21 +4221,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
 	if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT)
 		vmcs_write64(GUEST_IA32_PAT, vmx->vcpu.arch.pat);
 
-	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) {
-		u32 index = vmx_msr_index[i];
-		u32 data_low, data_high;
-		int j = vmx->nmsrs;
-
-		if (rdmsr_safe(index, &data_low, &data_high) < 0)
-			continue;
-		if (wrmsr_safe(index, data_low, data_high) < 0)
-			continue;
-		vmx->guest_msrs[j].index = i;
-		vmx->guest_msrs[j].data = 0;
-		vmx->guest_msrs[j].mask = -1ull;
-		++vmx->nmsrs;
-	}
-
 	vm_exit_controls_set(vmx, vmx_vmexit_ctrl());
 
 	/* 22.2.1, 20.8.1 */
@@ -6710,7 +6689,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 	int err;
 	struct vcpu_vmx *vmx;
 	unsigned long *msr_bitmap;
-	int cpu;
+	int i, cpu;
 
 	BUILD_BUG_ON_MSG(offsetof(struct vcpu_vmx, vcpu) != 0,
 		"struct kvm_vcpu must be at offset 0 for arch usercopy region");
@@ -6786,9 +6765,25 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 	cpu = get_cpu();
 	vmx_vcpu_load(&vmx->vcpu, cpu);
 	vmx->vcpu.cpu = cpu;
-	vmx_vcpu_setup(vmx);
+	vmx_vmcs_setup(vmx);
 	vmx_vcpu_put(&vmx->vcpu);
 	put_cpu();
+
+	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) {
+		u32 index = vmx_msr_index[i];
+		u32 data_low, data_high;
+		int j = vmx->nmsrs;
+
+		if (rdmsr_safe(index, &data_low, &data_high) < 0)
+			continue;
+		if (wrmsr_safe(index, data_low, data_high) < 0)
+			continue;
+		vmx->guest_msrs[j].index = i;
+		vmx->guest_msrs[j].data = 0;
+		vmx->guest_msrs[j].mask = -1ull;
+		++vmx->nmsrs;
+	}
+
 	if (cpu_need_virtualize_apic_accesses(&vmx->vcpu)) {
 		err = alloc_apic_access_page(kvm);
 		if (err)
-- 
2.19.1

Re: [PATCH v2 2/3] KVM: VMX: Rename {vmx,nested_vmx}_vcpu_setup() and minor cleanup

[Sean Christopherson]

On Fri, Oct 18, 2019 at 05:37:22PM +0800, Xiaoyao Li wrote:
> Rename {vmx,nested_vmx}_vcpu_setup() to {vmx,nested_vmx}_vmcs_setup,
> to match what they really do.
> 
> Aslo remove the vmcs unrelated codes to vmx_vcpu_create().

Do this in a separate patch, just in case there is a dependencies we're
missing.

> The initialization of vmx->hv_deadline_tsc can be removed here, because
> it will be called in vmx_vcpu_reset() as the flow:
> 
> kvm_arch_vcpu_setup()
>   -> kvm_vcpu_reset()
>        -> vmx_vcpu_reset()

Definitely needs to be in a separate patch.

> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> Changes in v2:
>   - move out the vmcs unrelated codes
> ---
>  arch/x86/kvm/vmx/nested.c |  2 +-
>  arch/x86/kvm/vmx/nested.h |  2 +-
>  arch/x86/kvm/vmx/vmx.c    | 45 +++++++++++++++++----------------------
>  3 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 5e231da00310..7935422d311f 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5768,7 +5768,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>  	return ret;
>  }
>  
> -void nested_vmx_vcpu_setup(void)
> +void nested_vmx_vmcs_setup(void)

"vmcs_setup" sounds like we're allocating and loading a VMCS.  Maybe
{nested_,}vmx_set_initial_vmcs_state() a la vmx_set_constant_host_state()?

>  {
>  	if (enable_shadow_vmcs) {
>  		vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap));
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index 187d39bf0bf1..2be1ba7482c9 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -11,7 +11,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
>  				bool apicv);
>  void nested_vmx_hardware_unsetup(void);
>  __init int nested_vmx_hardware_setup(int (*exit_handlers[])(struct kvm_vcpu *));
> -void nested_vmx_vcpu_setup(void);
> +void nested_vmx_vmcs_setup(void);
>  void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu);
>  int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry);
>  bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index ef567df344bf..b083316a598d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4161,15 +4161,10 @@ static void ept_set_mmio_spte_mask(void)
>  
>  #define VMX_XSS_EXIT_BITMAP 0
>  
> -/*
> - * Sets up the vmcs for emulated real mode.
> - */
> -static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
> +static void vmx_vmcs_setup(struct vcpu_vmx *vmx)
>  {
> -	int i;
> -
>  	if (nested)
> -		nested_vmx_vcpu_setup();
> +		nested_vmx_vmcs_setup();
>  
>  	if (cpu_has_vmx_msr_bitmap())
>  		vmcs_write64(MSR_BITMAP, __pa(vmx->vmcs01.msr_bitmap));
> @@ -4178,7 +4173,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
>  
>  	/* Control */
>  	pin_controls_set(vmx, vmx_pin_based_exec_ctrl(vmx));
> -	vmx->hv_deadline_tsc = -1;
>  
>  	exec_controls_set(vmx, vmx_exec_control(vmx));
>  
> @@ -4227,21 +4221,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
>  	if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT)
>  		vmcs_write64(GUEST_IA32_PAT, vmx->vcpu.arch.pat);
>  
> -	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) {
> -		u32 index = vmx_msr_index[i];
> -		u32 data_low, data_high;
> -		int j = vmx->nmsrs;
> -
> -		if (rdmsr_safe(index, &data_low, &data_high) < 0)
> -			continue;
> -		if (wrmsr_safe(index, data_low, data_high) < 0)
> -			continue;
> -		vmx->guest_msrs[j].index = i;
> -		vmx->guest_msrs[j].data = 0;
> -		vmx->guest_msrs[j].mask = -1ull;
> -		++vmx->nmsrs;
> -	}
> -
>  	vm_exit_controls_set(vmx, vmx_vmexit_ctrl());
>  
>  	/* 22.2.1, 20.8.1 */
> @@ -6710,7 +6689,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>  	int err;
>  	struct vcpu_vmx *vmx;
>  	unsigned long *msr_bitmap;
> -	int cpu;
> +	int i, cpu;
>  
>  	BUILD_BUG_ON_MSG(offsetof(struct vcpu_vmx, vcpu) != 0,
>  		"struct kvm_vcpu must be at offset 0 for arch usercopy region");
> @@ -6786,9 +6765,25 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>  	cpu = get_cpu();
>  	vmx_vcpu_load(&vmx->vcpu, cpu);
>  	vmx->vcpu.cpu = cpu;
> -	vmx_vcpu_setup(vmx);
> +	vmx_vmcs_setup(vmx);
>  	vmx_vcpu_put(&vmx->vcpu);
>  	put_cpu();
> +
> +	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) {
> +		u32 index = vmx_msr_index[i];
> +		u32 data_low, data_high;
> +		int j = vmx->nmsrs;
> +
> +		if (rdmsr_safe(index, &data_low, &data_high) < 0)
> +			continue;
> +		if (wrmsr_safe(index, data_low, data_high) < 0)
> +			continue;
> +		vmx->guest_msrs[j].index = i;
> +		vmx->guest_msrs[j].data = 0;
> +		vmx->guest_msrs[j].mask = -1ull;
> +		++vmx->nmsrs;
> +	}

I'd put this immediately after guest_msrs is allocated.  Yeah, we'll waste
a few cycles if allocating vmcs01 fails, but that should be a very rare
event.

> +
>  	if (cpu_need_virtualize_apic_accesses(&vmx->vcpu)) {
>  		err = alloc_apic_access_page(kvm);
>  		if (err)
> -- 
> 2.19.1
> 

Re: [PATCH v2 2/3] KVM: VMX: Rename {vmx,nested_vmx}_vcpu_setup() and minor cleanup

[Xiaoyao Li]

On 10/19/2019 1:09 AM, Sean Christopherson wrote:
> On Fri, Oct 18, 2019 at 05:37:22PM +0800, Xiaoyao Li wrote:
>> Rename {vmx,nested_vmx}_vcpu_setup() to {vmx,nested_vmx}_vmcs_setup,
>> to match what they really do.
>>
>> Aslo remove the vmcs unrelated codes to vmx_vcpu_create().
> 
> Do this in a separate patch, just in case there is a dependencies we're
> missing.
> 
>> The initialization of vmx->hv_deadline_tsc can be removed here, because
>> it will be called in vmx_vcpu_reset() as the flow:
>>
>> kvm_arch_vcpu_setup()
>>    -> kvm_vcpu_reset()
>>         -> vmx_vcpu_reset()
> 
> Definitely needs to be in a separate patch.
> 

OK, I'll split it into 3 patches.

>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> Changes in v2:
>>    - move out the vmcs unrelated codes
>> ---
>>   arch/x86/kvm/vmx/nested.c |  2 +-
>>   arch/x86/kvm/vmx/nested.h |  2 +-
>>   arch/x86/kvm/vmx/vmx.c    | 45 +++++++++++++++++----------------------
>>   3 files changed, 22 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index 5e231da00310..7935422d311f 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -5768,7 +5768,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>>   	return ret;
>>   }
>>   
>> -void nested_vmx_vcpu_setup(void)
>> +void nested_vmx_vmcs_setup(void)
> 
> "vmcs_setup" sounds like we're allocating and loading a VMCS.  Maybe
> {nested_,}vmx_set_initial_vmcs_state() a la vmx_set_constant_host_state()?
> 
>>   {
>>   	if (enable_shadow_vmcs) {
>>   		vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap));
>> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
>> index 187d39bf0bf1..2be1ba7482c9 100644
>> --- a/arch/x86/kvm/vmx/nested.h
>> +++ b/arch/x86/kvm/vmx/nested.h
>> @@ -11,7 +11,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
>>   				bool apicv);
>>   void nested_vmx_hardware_unsetup(void);
>>   __init int nested_vmx_hardware_setup(int (*exit_handlers[])(struct kvm_vcpu *));
>> -void nested_vmx_vcpu_setup(void);
>> +void nested_vmx_vmcs_setup(void);
>>   void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu);
>>   int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry);
>>   bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason);
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index ef567df344bf..b083316a598d 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -4161,15 +4161,10 @@ static void ept_set_mmio_spte_mask(void)
>>   
>>   #define VMX_XSS_EXIT_BITMAP 0
>>   
>> -/*
>> - * Sets up the vmcs for emulated real mode.
>> - */
>> -static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
>> +static void vmx_vmcs_setup(struct vcpu_vmx *vmx)
>>   {
>> -	int i;
>> -
>>   	if (nested)
>> -		nested_vmx_vcpu_setup();
>> +		nested_vmx_vmcs_setup();
>>   
>>   	if (cpu_has_vmx_msr_bitmap())
>>   		vmcs_write64(MSR_BITMAP, __pa(vmx->vmcs01.msr_bitmap));
>> @@ -4178,7 +4173,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
>>   
>>   	/* Control */
>>   	pin_controls_set(vmx, vmx_pin_based_exec_ctrl(vmx));
>> -	vmx->hv_deadline_tsc = -1;
>>   
>>   	exec_controls_set(vmx, vmx_exec_control(vmx));
>>   
>> @@ -4227,21 +4221,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
>>   	if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT)
>>   		vmcs_write64(GUEST_IA32_PAT, vmx->vcpu.arch.pat);
>>   
>> -	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) {
>> -		u32 index = vmx_msr_index[i];
>> -		u32 data_low, data_high;
>> -		int j = vmx->nmsrs;
>> -
>> -		if (rdmsr_safe(index, &data_low, &data_high) < 0)
>> -			continue;
>> -		if (wrmsr_safe(index, data_low, data_high) < 0)
>> -			continue;
>> -		vmx->guest_msrs[j].index = i;
>> -		vmx->guest_msrs[j].data = 0;
>> -		vmx->guest_msrs[j].mask = -1ull;
>> -		++vmx->nmsrs;
>> -	}
>> -
>>   	vm_exit_controls_set(vmx, vmx_vmexit_ctrl());
>>   
>>   	/* 22.2.1, 20.8.1 */
>> @@ -6710,7 +6689,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>   	int err;
>>   	struct vcpu_vmx *vmx;
>>   	unsigned long *msr_bitmap;
>> -	int cpu;
>> +	int i, cpu;
>>   
>>   	BUILD_BUG_ON_MSG(offsetof(struct vcpu_vmx, vcpu) != 0,
>>   		"struct kvm_vcpu must be at offset 0 for arch usercopy region");
>> @@ -6786,9 +6765,25 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>   	cpu = get_cpu();
>>   	vmx_vcpu_load(&vmx->vcpu, cpu);
>>   	vmx->vcpu.cpu = cpu;
>> -	vmx_vcpu_setup(vmx);
>> +	vmx_vmcs_setup(vmx);
>>   	vmx_vcpu_put(&vmx->vcpu);
>>   	put_cpu();
>> +
>> +	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i) {
>> +		u32 index = vmx_msr_index[i];
>> +		u32 data_low, data_high;
>> +		int j = vmx->nmsrs;
>> +
>> +		if (rdmsr_safe(index, &data_low, &data_high) < 0)
>> +			continue;
>> +		if (wrmsr_safe(index, data_low, data_high) < 0)
>> +			continue;
>> +		vmx->guest_msrs[j].index = i;
>> +		vmx->guest_msrs[j].data = 0;
>> +		vmx->guest_msrs[j].mask = -1ull;
>> +		++vmx->nmsrs;
>> +	}
> 
> I'd put this immediately after guest_msrs is allocated.  Yeah, we'll waste
> a few cycles if allocating vmcs01 fails, but that should be a very rare
> event.
> 

OK.

>> +
>>   	if (cpu_need_virtualize_apic_accesses(&vmx->vcpu)) {
>>   		err = alloc_apic_access_page(kvm);
>>   		if (err)
>> -- 
>> 2.19.1
>>

[PATCH v2 3/3] KVM: VMX: Some minor refactor of MSR bitmap

[Xiaoyao Li]

Move the MSR bitmap capability check from vmx_disable_intercept_for_msr()
and vmx_enable_intercept_for_msr(), so that we can do the check far
early before we really want to touch the bitmap.

Also, we can move the common MSR not-intercept setup to where msr bitmap
is actually used.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes in v2:
  - Remove the check of cpu_has_vmx_msr_bitmap() from
    vmx_{disable,enable}_intercept_for_msr (Krish)
---
 arch/x86/kvm/vmx/vmx.c | 65 +++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b083316a598d..017689d0144e 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -343,8 +343,8 @@ module_param_cb(vmentry_l1d_flush, &vmentry_l1d_flush_ops, NULL, 0644);
 
 static bool guest_state_valid(struct kvm_vcpu *vcpu);
 static u32 vmx_segment_access_rights(struct kvm_segment *var);
-static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bitmap,
-							  u32 msr, int type);
+static __always_inline void vmx_set_intercept_for_msr(unsigned long *msr_bitmap,
+		u32 msr, int type, bool value);
 
 void vmx_vmexit(void);
 
@@ -2000,9 +2000,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		 * in the merging. We update the vmcs01 here for L1 as well
 		 * since it will end up touching the MSR anyway now.
 		 */
-		vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap,
-					      MSR_IA32_SPEC_CTRL,
-					      MSR_TYPE_RW);
+		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap,
+					  MSR_IA32_SPEC_CTRL,
+					  MSR_TYPE_RW, false);
 		break;
 	case MSR_IA32_PRED_CMD:
 		if (!msr_info->host_initiated &&
@@ -2028,8 +2028,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		 * vmcs02.msr_bitmap here since it gets completely overwritten
 		 * in the merging.
 		 */
-		vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_IA32_PRED_CMD,
-					      MSR_TYPE_W);
+		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap,
+					  MSR_IA32_PRED_CMD,
+					  MSR_TYPE_W, false);
 		break;
 	case MSR_IA32_CR_PAT:
 		if (!kvm_pat_valid(data))
@@ -3599,9 +3600,6 @@ static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bit
 {
 	int f = sizeof(unsigned long);
 
-	if (!cpu_has_vmx_msr_bitmap())
-		return;
-
 	if (static_branch_unlikely(&enable_evmcs))
 		evmcs_touch_msr_bitmap();
 
@@ -3637,9 +3635,6 @@ static __always_inline void vmx_enable_intercept_for_msr(unsigned long *msr_bitm
 {
 	int f = sizeof(unsigned long);
 
-	if (!cpu_has_vmx_msr_bitmap())
-		return;
-
 	if (static_branch_unlikely(&enable_evmcs))
 		evmcs_touch_msr_bitmap();
 
@@ -3673,6 +3668,9 @@ static __always_inline void vmx_enable_intercept_for_msr(unsigned long *msr_bitm
 static __always_inline void vmx_set_intercept_for_msr(unsigned long *msr_bitmap,
 			     			      u32 msr, int type, bool value)
 {
+	if (!cpu_has_vmx_msr_bitmap())
+		return;
+
 	if (value)
 		vmx_enable_intercept_for_msr(msr_bitmap, msr, type);
 	else
@@ -4163,11 +4161,30 @@ static void ept_set_mmio_spte_mask(void)
 
 static void vmx_vmcs_setup(struct vcpu_vmx *vmx)
 {
+	unsigned long *msr_bitmap;
+
 	if (nested)
 		nested_vmx_vmcs_setup();
 
-	if (cpu_has_vmx_msr_bitmap())
+	if (cpu_has_vmx_msr_bitmap()) {
+		msr_bitmap = vmx->vmcs01.msr_bitmap;
+		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
+		vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
+		vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
+		vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
+		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
+		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
+		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
+		if (kvm_cstate_in_guest(vmx->vcpu.kvm)) {
+			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
+			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
+			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
+			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
+		}
+
 		vmcs_write64(MSR_BITMAP, __pa(vmx->vmcs01.msr_bitmap));
+	}
+	vmx->msr_bitmap_mode = 0;
 
 	vmcs_write64(VMCS_LINK_POINTER, -1ull); /* 22.3.1.5 */
 
@@ -6074,7 +6091,8 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
 	}
 	secondary_exec_controls_set(vmx, sec_exec_control);
 
-	vmx_update_msr_bitmap(vcpu);
+	if (cpu_has_vmx_msr_bitmap())
+		vmx_update_msr_bitmap(vcpu);
 }
 
 static void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu, hpa_t hpa)
@@ -6688,7 +6706,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 {
 	int err;
 	struct vcpu_vmx *vmx;
-	unsigned long *msr_bitmap;
 	int i, cpu;
 
 	BUILD_BUG_ON_MSG(offsetof(struct vcpu_vmx, vcpu) != 0,
@@ -6745,22 +6762,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 	if (err < 0)
 		goto free_msrs;
 
-	msr_bitmap = vmx->vmcs01.msr_bitmap;
-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
-	if (kvm_cstate_in_guest(kvm)) {
-		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
-		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
-		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
-		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
-	}
-	vmx->msr_bitmap_mode = 0;
-
 	vmx->loaded_vmcs = &vmx->vmcs01;
 	cpu = get_cpu();
 	vmx_vcpu_load(&vmx->vcpu, cpu);
-- 
2.19.1

Re: [PATCH v2 3/3] KVM: VMX: Some minor refactor of MSR bitmap

[Sean Christopherson]

On Fri, Oct 18, 2019 at 05:37:23PM +0800, Xiaoyao Li wrote:
> Move the MSR bitmap capability check from vmx_disable_intercept_for_msr()
> and vmx_enable_intercept_for_msr(), so that we can do the check far
> early before we really want to touch the bitmap.
> 
> Also, we can move the common MSR not-intercept setup to where msr bitmap
> is actually used.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> Changes in v2:
>   - Remove the check of cpu_has_vmx_msr_bitmap() from
>     vmx_{disable,enable}_intercept_for_msr (Krish)
> ---
>  arch/x86/kvm/vmx/vmx.c | 65 +++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index b083316a598d..017689d0144e 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -343,8 +343,8 @@ module_param_cb(vmentry_l1d_flush, &vmentry_l1d_flush_ops, NULL, 0644);
>  
>  static bool guest_state_valid(struct kvm_vcpu *vcpu);
>  static u32 vmx_segment_access_rights(struct kvm_segment *var);
> -static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bitmap,
> -							  u32 msr, int type);
> +static __always_inline void vmx_set_intercept_for_msr(unsigned long *msr_bitmap,
> +		u32 msr, int type, bool value);
>  
>  void vmx_vmexit(void);
>  
> @@ -2000,9 +2000,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		 * in the merging. We update the vmcs01 here for L1 as well
>  		 * since it will end up touching the MSR anyway now.
>  		 */
> -		vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap,
> -					      MSR_IA32_SPEC_CTRL,
> -					      MSR_TYPE_RW);
> +		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap,
> +					  MSR_IA32_SPEC_CTRL,
> +					  MSR_TYPE_RW, false);

IMO this is a net negative.  The explicit "disable" is significantly more
intuitive than "set" with a %false param, e.g. at a quick glance it would
be easy to think this code is "setting", i.e. "enabling" interception.

>  		break;
>  	case MSR_IA32_PRED_CMD:
>  		if (!msr_info->host_initiated &&
> @@ -2028,8 +2028,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		 * vmcs02.msr_bitmap here since it gets completely overwritten
>  		 * in the merging.
>  		 */
> -		vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_IA32_PRED_CMD,
> -					      MSR_TYPE_W);
> +		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap,
> +					  MSR_IA32_PRED_CMD,
> +					  MSR_TYPE_W, false);
>  		break;
>  	case MSR_IA32_CR_PAT:
>  		if (!kvm_pat_valid(data))
> @@ -3599,9 +3600,6 @@ static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bit
>  {
>  	int f = sizeof(unsigned long);
>  
> -	if (!cpu_has_vmx_msr_bitmap())
> -		return;

As above, I'd rather keep these here.  Functionally it changes nothing on
CPUs with an MSR bitmap.  For old CPUs, it saves all of two uops in paths
that aren't performance critical.

> -
>  	if (static_branch_unlikely(&enable_evmcs))
>  		evmcs_touch_msr_bitmap();
>  
> @@ -3637,9 +3635,6 @@ static __always_inline void vmx_enable_intercept_for_msr(unsigned long *msr_bitm
>  {
>  	int f = sizeof(unsigned long);
>  
> -	if (!cpu_has_vmx_msr_bitmap())
> -		return;
> -
>  	if (static_branch_unlikely(&enable_evmcs))
>  		evmcs_touch_msr_bitmap();
>  
> @@ -3673,6 +3668,9 @@ static __always_inline void vmx_enable_intercept_for_msr(unsigned long *msr_bitm
>  static __always_inline void vmx_set_intercept_for_msr(unsigned long *msr_bitmap,
>  			     			      u32 msr, int type, bool value)
>  {
> +	if (!cpu_has_vmx_msr_bitmap())
> +		return;
> +
>  	if (value)
>  		vmx_enable_intercept_for_msr(msr_bitmap, msr, type);
>  	else
> @@ -4163,11 +4161,30 @@ static void ept_set_mmio_spte_mask(void)
>  
>  static void vmx_vmcs_setup(struct vcpu_vmx *vmx)
>  {
> +	unsigned long *msr_bitmap;
> +
>  	if (nested)
>  		nested_vmx_vmcs_setup();
>  
> -	if (cpu_has_vmx_msr_bitmap())
> +	if (cpu_has_vmx_msr_bitmap()) {
> +		msr_bitmap = vmx->vmcs01.msr_bitmap;
> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
> +		if (kvm_cstate_in_guest(vmx->vcpu.kvm)) {
> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
> +		}
> +
>  		vmcs_write64(MSR_BITMAP, __pa(vmx->vmcs01.msr_bitmap));
> +	}
> +	vmx->msr_bitmap_mode = 0;

Zeroing msr_bitmap_mode can be skipped as well.

>  	vmcs_write64(VMCS_LINK_POINTER, -1ull); /* 22.3.1.5 */
>  
> @@ -6074,7 +6091,8 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>  	}
>  	secondary_exec_controls_set(vmx, sec_exec_control);
>  
> -	vmx_update_msr_bitmap(vcpu);
> +	if (cpu_has_vmx_msr_bitmap())
> +		vmx_update_msr_bitmap(vcpu);
>  }
>  
>  static void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu, hpa_t hpa)
> @@ -6688,7 +6706,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>  {
>  	int err;
>  	struct vcpu_vmx *vmx;
> -	unsigned long *msr_bitmap;
>  	int i, cpu;
>  
>  	BUILD_BUG_ON_MSG(offsetof(struct vcpu_vmx, vcpu) != 0,
> @@ -6745,22 +6762,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>  	if (err < 0)
>  		goto free_msrs;
>  
> -	msr_bitmap = vmx->vmcs01.msr_bitmap;
> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
> -	if (kvm_cstate_in_guest(kvm)) {
> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
> -	}
> -	vmx->msr_bitmap_mode = 0;

Keep this code here to be consistent with the previous change that moved
the guest_msrs intialization *out* of the VMCS specific function.  Both
are collateral pages that are not directly part of the VMCS.

I'd be tempted to use a goto to skip the code, the line length is bad
enough as it is, e.g.:

	if (!cpu_has_vmx_msr_bitmap())
		goto skip_msr_bitmap;

	vmx->msr_bitmap_mode = 0;
skip_msr_bitmap:

> -
>  	vmx->loaded_vmcs = &vmx->vmcs01;
>  	cpu = get_cpu();
>  	vmx_vcpu_load(&vmx->vcpu, cpu);
> -- 
> 2.19.1
> 

Re: [PATCH v2 3/3] KVM: VMX: Some minor refactor of MSR bitmap

[Xiaoyao Li]

On 10/19/2019 1:27 AM, Sean Christopherson wrote:
> On Fri, Oct 18, 2019 at 05:37:23PM +0800, Xiaoyao Li wrote:
>> Move the MSR bitmap capability check from vmx_disable_intercept_for_msr()
>> and vmx_enable_intercept_for_msr(), so that we can do the check far
>> early before we really want to touch the bitmap.
>>
>> Also, we can move the common MSR not-intercept setup to where msr bitmap
>> is actually used.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> Changes in v2:
>>    - Remove the check of cpu_has_vmx_msr_bitmap() from
>>      vmx_{disable,enable}_intercept_for_msr (Krish)
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 65 +++++++++++++++++++++---------------------
>>   1 file changed, 33 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index b083316a598d..017689d0144e 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -343,8 +343,8 @@ module_param_cb(vmentry_l1d_flush, &vmentry_l1d_flush_ops, NULL, 0644);
>>   
>>   static bool guest_state_valid(struct kvm_vcpu *vcpu);
>>   static u32 vmx_segment_access_rights(struct kvm_segment *var);
>> -static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bitmap,
>> -							  u32 msr, int type);
>> +static __always_inline void vmx_set_intercept_for_msr(unsigned long *msr_bitmap,
>> +		u32 msr, int type, bool value);
>>   
>>   void vmx_vmexit(void);
>>   
>> @@ -2000,9 +2000,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   		 * in the merging. We update the vmcs01 here for L1 as well
>>   		 * since it will end up touching the MSR anyway now.
>>   		 */
>> -		vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap,
>> -					      MSR_IA32_SPEC_CTRL,
>> -					      MSR_TYPE_RW);
>> +		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap,
>> +					  MSR_IA32_SPEC_CTRL,
>> +					  MSR_TYPE_RW, false);
> 
> IMO this is a net negative.  The explicit "disable" is significantly more
> intuitive than "set" with a %false param, e.g. at a quick glance it would
> be easy to think this code is "setting", i.e. "enabling" interception.
> 
>>   		break;
>>   	case MSR_IA32_PRED_CMD:
>>   		if (!msr_info->host_initiated &&
>> @@ -2028,8 +2028,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   		 * vmcs02.msr_bitmap here since it gets completely overwritten
>>   		 * in the merging.
>>   		 */
>> -		vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_IA32_PRED_CMD,
>> -					      MSR_TYPE_W);
>> +		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap,
>> +					  MSR_IA32_PRED_CMD,
>> +					  MSR_TYPE_W, false);
>>   		break;
>>   	case MSR_IA32_CR_PAT:
>>   		if (!kvm_pat_valid(data))
>> @@ -3599,9 +3600,6 @@ static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bit
>>   {
>>   	int f = sizeof(unsigned long);
>>   
>> -	if (!cpu_has_vmx_msr_bitmap())
>> -		return;
> 
> As above, I'd rather keep these here.  Functionally it changes nothing on
> CPUs with an MSR bitmap.  For old CPUs, it saves all of two uops in paths
> that aren't performance critical.
> 
>> -
>>   	if (static_branch_unlikely(&enable_evmcs))
>>   		evmcs_touch_msr_bitmap();
>>   
>> @@ -3637,9 +3635,6 @@ static __always_inline void vmx_enable_intercept_for_msr(unsigned long *msr_bitm
>>   {
>>   	int f = sizeof(unsigned long);
>>   
>> -	if (!cpu_has_vmx_msr_bitmap())
>> -		return;
>> -
>>   	if (static_branch_unlikely(&enable_evmcs))
>>   		evmcs_touch_msr_bitmap();
>>   
>> @@ -3673,6 +3668,9 @@ static __always_inline void vmx_enable_intercept_for_msr(unsigned long *msr_bitm
>>   static __always_inline void vmx_set_intercept_for_msr(unsigned long *msr_bitmap,
>>   			     			      u32 msr, int type, bool value)
>>   {
>> +	if (!cpu_has_vmx_msr_bitmap())
>> +		return;
>> +
>>   	if (value)
>>   		vmx_enable_intercept_for_msr(msr_bitmap, msr, type);
>>   	else
>> @@ -4163,11 +4161,30 @@ static void ept_set_mmio_spte_mask(void)
>>   
>>   static void vmx_vmcs_setup(struct vcpu_vmx *vmx)
>>   {
>> +	unsigned long *msr_bitmap;
>> +
>>   	if (nested)
>>   		nested_vmx_vmcs_setup();
>>   
>> -	if (cpu_has_vmx_msr_bitmap())
>> +	if (cpu_has_vmx_msr_bitmap()) {
>> +		msr_bitmap = vmx->vmcs01.msr_bitmap;
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
>> +		if (kvm_cstate_in_guest(vmx->vcpu.kvm)) {
>> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
>> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
>> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
>> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
>> +		}
>> +
>>   		vmcs_write64(MSR_BITMAP, __pa(vmx->vmcs01.msr_bitmap));
>> +	}
>> +	vmx->msr_bitmap_mode = 0;
> 
> Zeroing msr_bitmap_mode can be skipped as well.
> 
>>   	vmcs_write64(VMCS_LINK_POINTER, -1ull); /* 22.3.1.5 */
>>   
>> @@ -6074,7 +6091,8 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>>   	}
>>   	secondary_exec_controls_set(vmx, sec_exec_control);
>>   
>> -	vmx_update_msr_bitmap(vcpu);
>> +	if (cpu_has_vmx_msr_bitmap())
>> +		vmx_update_msr_bitmap(vcpu);
>>   }
>>   
>>   static void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu, hpa_t hpa)
>> @@ -6688,7 +6706,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>   {
>>   	int err;
>>   	struct vcpu_vmx *vmx;
>> -	unsigned long *msr_bitmap;
>>   	int i, cpu;
>>   
>>   	BUILD_BUG_ON_MSG(offsetof(struct vcpu_vmx, vcpu) != 0,
>> @@ -6745,22 +6762,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>   	if (err < 0)
>>   		goto free_msrs;
>>   
>> -	msr_bitmap = vmx->vmcs01.msr_bitmap;
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
>> -	if (kvm_cstate_in_guest(kvm)) {
>> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
>> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
>> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
>> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
>> -	}
>> -	vmx->msr_bitmap_mode = 0;
> 
> Keep this code here to be consistent with the previous change that moved
> the guest_msrs intialization *out* of the VMCS specific function.  Both
> are collateral pages that are not directly part of the VMCS.
>

OK. Then this patch is unnecessary too.

> I'd be tempted to use a goto to skip the code, the line length is bad
> enough as it is, e.g.:
> 
> 	if (!cpu_has_vmx_msr_bitmap())
> 		goto skip_msr_bitmap;
> 
> 	vmx->msr_bitmap_mode = 0;
> skip_msr_bitmap:
> 
>> -
>>   	vmx->loaded_vmcs = &vmx->vmcs01;
>>   	cpu = get_cpu();
>>   	vmx_vcpu_load(&vmx->vcpu, cpu);
>> -- 
>> 2.19.1
>>

Re: [PATCH v2 3/3] KVM: VMX: Some minor refactor of MSR bitmap

[Sean Christopherson]

On Sat, Oct 19, 2019 at 02:39:57AM +0800, Xiaoyao Li wrote:
> On 10/19/2019 1:27 AM, Sean Christopherson wrote:
> >>@@ -6745,22 +6762,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
> >>  	if (err < 0)
> >>  		goto free_msrs;
> >>-	msr_bitmap = vmx->vmcs01.msr_bitmap;
> >>-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
> >>-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
> >>-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
> >>-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
> >>-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
> >>-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
> >>-	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
> >>-	if (kvm_cstate_in_guest(kvm)) {
> >>-		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
> >>-		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
> >>-		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
> >>-		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
> >>-	}
> >>-	vmx->msr_bitmap_mode = 0;
> >
> >Keep this code here to be consistent with the previous change that moved
> >the guest_msrs intialization *out* of the VMCS specific function.  Both
> >are collateral pages that are not directly part of the VMCS.
> >
> 
> OK. Then this patch is unnecessary too.

I'd keep the change to skip this code if the CPU doesn't have msr bitmaps.
Performance wise it may be largely irrelevant, but it's a good change from
a readability perspective.

> 
> >I'd be tempted to use a goto to skip the code, the line length is bad
> >enough as it is, e.g.:
> >
> >	if (!cpu_has_vmx_msr_bitmap())
> >		goto skip_msr_bitmap;
> >
> >	vmx->msr_bitmap_mode = 0;
> >skip_msr_bitmap:
> >
> >>-
> >>  	vmx->loaded_vmcs = &vmx->vmcs01;
> >>  	cpu = get_cpu();
> >>  	vmx_vcpu_load(&vmx->vcpu, cpu);
> >>-- 
> >>2.19.1
> >>

Re: [PATCH v2 3/3] KVM: VMX: Some minor refactor of MSR bitmap

[Xiaoyao Li]

On 10/19/2019 1:27 AM, Sean Christopherson wrote:
> On Fri, Oct 18, 2019 at 05:37:23PM +0800, Xiaoyao Li wrote:
>> Move the MSR bitmap capability check from vmx_disable_intercept_for_msr()
>> and vmx_enable_intercept_for_msr(), so that we can do the check far
>> early before we really want to touch the bitmap.
>>
>> Also, we can move the common MSR not-intercept setup to where msr bitmap
>> is actually used.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> Changes in v2:
>>    - Remove the check of cpu_has_vmx_msr_bitmap() from
>>      vmx_{disable,enable}_intercept_for_msr (Krish)
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 65 +++++++++++++++++++++---------------------
>>   1 file changed, 33 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index b083316a598d..017689d0144e 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -343,8 +343,8 @@ module_param_cb(vmentry_l1d_flush, &vmentry_l1d_flush_ops, NULL, 0644);
>>   
>>   static bool guest_state_valid(struct kvm_vcpu *vcpu);
>>   static u32 vmx_segment_access_rights(struct kvm_segment *var);
>> -static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bitmap,
>> -							  u32 msr, int type);
>> +static __always_inline void vmx_set_intercept_for_msr(unsigned long *msr_bitmap,
>> +		u32 msr, int type, bool value);
>>   
>>   void vmx_vmexit(void);
>>   
>> @@ -2000,9 +2000,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   		 * in the merging. We update the vmcs01 here for L1 as well
>>   		 * since it will end up touching the MSR anyway now.
>>   		 */
>> -		vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap,
>> -					      MSR_IA32_SPEC_CTRL,
>> -					      MSR_TYPE_RW);
>> +		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap,
>> +					  MSR_IA32_SPEC_CTRL,
>> +					  MSR_TYPE_RW, false);
> 
> IMO this is a net negative.  The explicit "disable" is significantly more
> intuitive than "set" with a %false param, e.g. at a quick glance it would
> be easy to think this code is "setting", i.e. "enabling" interception.
>

How about renaming it to vmx_switch_intercept_for_msr()?
or just add the cpu_has_vmx_msr_bitmap() check outside because the check 
is removed in vmx_disable_intercept_for_msr()

>>   		break;
>>   	case MSR_IA32_PRED_CMD:
>>   		if (!msr_info->host_initiated &&
>> @@ -2028,8 +2028,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   		 * vmcs02.msr_bitmap here since it gets completely overwritten
>>   		 * in the merging.
>>   		 */
>> -		vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_IA32_PRED_CMD,
>> -					      MSR_TYPE_W);
>> +		vmx_set_intercept_for_msr(vmx->vmcs01.msr_bitmap,
>> +					  MSR_IA32_PRED_CMD,
>> +					  MSR_TYPE_W, false);
>>   		break;
>>   	case MSR_IA32_CR_PAT:
>>   		if (!kvm_pat_valid(data))
>> @@ -3599,9 +3600,6 @@ static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bit
>>   {
>>   	int f = sizeof(unsigned long);
>>   
>> -	if (!cpu_has_vmx_msr_bitmap())
>> -		return;
> 
> As above, I'd rather keep these here.  Functionally it changes nothing on
> CPUs with an MSR bitmap.  For old CPUs, it saves all of two uops in paths
> that aren't performance critical.
> 
>> -
>>   	if (static_branch_unlikely(&enable_evmcs))
>>   		evmcs_touch_msr_bitmap();
>>   
>> @@ -3637,9 +3635,6 @@ static __always_inline void vmx_enable_intercept_for_msr(unsigned long *msr_bitm
>>   {
>>   	int f = sizeof(unsigned long);
>>   
>> -	if (!cpu_has_vmx_msr_bitmap())
>> -		return;
>> -
>>   	if (static_branch_unlikely(&enable_evmcs))
>>   		evmcs_touch_msr_bitmap();
>>   
>> @@ -3673,6 +3668,9 @@ static __always_inline void vmx_enable_intercept_for_msr(unsigned long *msr_bitm
>>   static __always_inline void vmx_set_intercept_for_msr(unsigned long *msr_bitmap,
>>   			     			      u32 msr, int type, bool value)
>>   {
>> +	if (!cpu_has_vmx_msr_bitmap())
>> +		return;
>> +
>>   	if (value)
>>   		vmx_enable_intercept_for_msr(msr_bitmap, msr, type);
>>   	else
>> @@ -4163,11 +4161,30 @@ static void ept_set_mmio_spte_mask(void)
>>   
>>   static void vmx_vmcs_setup(struct vcpu_vmx *vmx)
>>   {
>> +	unsigned long *msr_bitmap;
>> +
>>   	if (nested)
>>   		nested_vmx_vmcs_setup();
>>   
>> -	if (cpu_has_vmx_msr_bitmap())
>> +	if (cpu_has_vmx_msr_bitmap()) {
>> +		msr_bitmap = vmx->vmcs01.msr_bitmap;
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
>> +		vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
>> +		if (kvm_cstate_in_guest(vmx->vcpu.kvm)) {
>> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
>> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
>> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
>> +			vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
>> +		}
>> +
>>   		vmcs_write64(MSR_BITMAP, __pa(vmx->vmcs01.msr_bitmap));
>> +	}
>> +	vmx->msr_bitmap_mode = 0;
> 
> Zeroing msr_bitmap_mode can be skipped as well.
> 
>>   	vmcs_write64(VMCS_LINK_POINTER, -1ull); /* 22.3.1.5 */
>>   
>> @@ -6074,7 +6091,8 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>>   	}
>>   	secondary_exec_controls_set(vmx, sec_exec_control);
>>   
>> -	vmx_update_msr_bitmap(vcpu);
>> +	if (cpu_has_vmx_msr_bitmap())
>> +		vmx_update_msr_bitmap(vcpu);
>>   }
>>   
>>   static void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu, hpa_t hpa)
>> @@ -6688,7 +6706,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>   {
>>   	int err;
>>   	struct vcpu_vmx *vmx;
>> -	unsigned long *msr_bitmap;
>>   	int i, cpu;
>>   
>>   	BUILD_BUG_ON_MSG(offsetof(struct vcpu_vmx, vcpu) != 0,
>> @@ -6745,22 +6762,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>   	if (err < 0)
>>   		goto free_msrs;
>>   
>> -	msr_bitmap = vmx->vmcs01.msr_bitmap;
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_TSC, MSR_TYPE_R);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_GS_BASE, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
>> -	vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
>> -	if (kvm_cstate_in_guest(kvm)) {
>> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C1_RES, MSR_TYPE_R);
>> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C3_RESIDENCY, MSR_TYPE_R);
>> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C6_RESIDENCY, MSR_TYPE_R);
>> -		vmx_disable_intercept_for_msr(msr_bitmap, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
>> -	}
>> -	vmx->msr_bitmap_mode = 0;
> 
> Keep this code here to be consistent with the previous change that moved
> the guest_msrs intialization *out* of the VMCS specific function.  Both
> are collateral pages that are not directly part of the VMCS.
> 
> I'd be tempted to use a goto to skip the code, the line length is bad
> enough as it is, e.g.:
> 
> 	if (!cpu_has_vmx_msr_bitmap())
> 		goto skip_msr_bitmap;
> 
> 	vmx->msr_bitmap_mode = 0;
> skip_msr_bitmap:
> 
>> -
>>   	vmx->loaded_vmcs = &vmx->vmcs01;
>>   	cpu = get_cpu();
>>   	vmx_vcpu_load(&vmx->vcpu, cpu);
>> -- 
>> 2.19.1
>>