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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191018195206.GG26319@linux.intel.com>
Date:   Fri, 18 Oct 2019 12:52:06 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Xiaoyao Li <xiaoyao.li@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] KVM: VMX: Some minor refactor of MSR bitmap

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