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
> >>