Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933344AbeAHXLe (ORCPT + 1 other); Mon, 8 Jan 2018 18:11:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42896 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932191AbeAHXLc (ORCPT ); Mon, 8 Jan 2018 18:11:32 -0500 Date: Mon, 8 Jan 2018 18:11:31 -0500 (EST) From: Paolo Bonzini To: Jim Mattson Cc: LKML , kvm list , Luwei Kang Message-ID: <1653432679.31735402.1515453091962.JavaMail.zimbra@redhat.com> In-Reply-To: References: <1515167000-51323-1-git-send-email-pbonzini@redhat.com> Subject: Re: [PATCH] KVM: VMX: use same MSR bitmaps for 32-/64-bit modes, fix MSR bitmaps for processor tracing MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [94.39.196.37, 10.4.196.12, 10.4.195.28] Thread-Topic: use same MSR bitmaps for 32-/64-bit modes, fix MSR bitmaps for processor tracing Thread-Index: MLPevDdQjuoBzo4FT3tmbSY0HaQtCQ== X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 08 Jan 2018 23:11:32 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: ----- Original Message ----- > From: "Jim Mattson" > To: "Paolo Bonzini" > Cc: "LKML" , "kvm list" , "Luwei Kang" > Sent: Monday, January 8, 2018 10:23:41 PM > Subject: Re: [PATCH] KVM: VMX: use same MSR bitmaps for 32-/64-bit modes, fix MSR bitmaps for processor tracing > > I would like to campaign for removing all of the precomputed MSR > permission bitmaps, and using a separate bitmap for each vCPU, a la > svm.c. As usual, patches are welcome. :) Paolo > On Fri, Jan 5, 2018 at 7:43 AM, Paolo Bonzini wrote: > > KVM has a small optimization where it doesn't save/restore > > MSR_KERNEL_GS_BASE if the guest is in 32-bit mode. However, > > this complicates the code noticeably by doubling the number of > > possible MSR bitmaps. In addition, pt_disable_intercept_for_msr > > was only updating the "basic" MSR bitmap, because x2apic and > > x2apic_apicv are memcpy'd just once in hardware_setup. > > > > Remove the long-mode bitmaps set, and touch all the three remaining > > bitmaps in vmx_disable_intercept_for_msr and vmx_enable_intercept_for_msr. > > > > Fixes: 3bd1f85e893daec4f3982d1d45b6bfc0683442c4 > > Cc: Luwei Kang > > Signed-off-by: Paolo Bonzini > > --- > > arch/x86/kvm/vmx.c | 126 > > ++++++++++++++++++++--------------------------------- > > 1 file changed, 48 insertions(+), 78 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > index 11321cc4129a..ea5b52dc2d78 100644 > > --- a/arch/x86/kvm/vmx.c > > +++ b/arch/x86/kvm/vmx.c > > @@ -935,12 +935,9 @@ static bool nested_vmx_is_page_fault_vmexit(struct > > vmcs12 *vmcs12, > > static DEFINE_PER_CPU(spinlock_t, blocked_vcpu_on_cpu_lock); > > > > enum { > > - VMX_MSR_BITMAP_LEGACY, > > - VMX_MSR_BITMAP_LONGMODE, > > - VMX_MSR_BITMAP_LEGACY_X2APIC_APICV, > > - VMX_MSR_BITMAP_LONGMODE_X2APIC_APICV, > > - VMX_MSR_BITMAP_LEGACY_X2APIC, > > - VMX_MSR_BITMAP_LONGMODE_X2APIC, > > + VMX_MSR_BITMAP_NOX2APIC, > > + VMX_MSR_BITMAP_X2APIC_APICV, > > + VMX_MSR_BITMAP_X2APIC, > > VMX_VMREAD_BITMAP, > > VMX_VMWRITE_BITMAP, > > VMX_BITMAP_NR > > @@ -948,12 +945,9 @@ enum { > > > > static unsigned long *vmx_bitmap[VMX_BITMAP_NR]; > > > > -#define vmx_msr_bitmap_legacy > > (vmx_bitmap[VMX_MSR_BITMAP_LEGACY]) > > -#define vmx_msr_bitmap_longmode > > (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE]) > > -#define vmx_msr_bitmap_legacy_x2apic_apicv > > (vmx_bitmap[VMX_MSR_BITMAP_LEGACY_X2APIC_APICV]) > > -#define vmx_msr_bitmap_longmode_x2apic_apicv > > (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE_X2APIC_APICV]) > > -#define vmx_msr_bitmap_legacy_x2apic > > (vmx_bitmap[VMX_MSR_BITMAP_LEGACY_X2APIC]) > > -#define vmx_msr_bitmap_longmode_x2apic > > (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE_X2APIC]) > > +#define vmx_msr_bitmap_nox2apic > > (vmx_bitmap[VMX_MSR_BITMAP_NOX2APIC]) > > +#define vmx_msr_bitmap_x2apic_apicv > > (vmx_bitmap[VMX_MSR_BITMAP_X2APIC_APICV]) > > +#define vmx_msr_bitmap_x2apic > > (vmx_bitmap[VMX_MSR_BITMAP_X2APIC]) > > #define vmx_vmread_bitmap > > (vmx_bitmap[VMX_VMREAD_BITMAP]) > > #define vmx_vmwrite_bitmap > > (vmx_bitmap[VMX_VMWRITE_BITMAP]) > > > > @@ -2203,8 +2197,7 @@ static void vmx_save_host_state(struct kvm_vcpu > > *vcpu) > > > > #ifdef CONFIG_X86_64 > > rdmsrl(MSR_KERNEL_GS_BASE, vmx->msr_host_kernel_gs_base); > > - if (is_long_mode(&vmx->vcpu)) > > - wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_guest_kernel_gs_base); > > + wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_guest_kernel_gs_base); > > #endif > > if (boot_cpu_has(X86_FEATURE_MPX)) > > rdmsrl(MSR_IA32_BNDCFGS, vmx->host_state.msr_host_bndcfgs); > > @@ -2222,8 +2215,7 @@ static void __vmx_load_host_state(struct vcpu_vmx > > *vmx) > > ++vmx->vcpu.stat.host_state_reload; > > vmx->host_state.loaded = 0; > > #ifdef CONFIG_X86_64 > > - if (is_long_mode(&vmx->vcpu)) > > - rdmsrl(MSR_KERNEL_GS_BASE, vmx->msr_guest_kernel_gs_base); > > + rdmsrl(MSR_KERNEL_GS_BASE, vmx->msr_guest_kernel_gs_base); > > #endif > > if (vmx->host_state.gs_ldt_reload_needed) { > > kvm_load_ldt(vmx->host_state.ldt_sel); > > @@ -2635,21 +2627,12 @@ static void vmx_set_msr_bitmap(struct kvm_vcpu > > *vcpu) > > (vmcs_read32(SECONDARY_VM_EXEC_CONTROL) & > > SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE)) { > > if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) { > > - if (is_long_mode(vcpu)) > > - msr_bitmap = > > vmx_msr_bitmap_longmode_x2apic_apicv; > > - else > > - msr_bitmap = > > vmx_msr_bitmap_legacy_x2apic_apicv; > > + msr_bitmap = vmx_msr_bitmap_x2apic_apicv; > > } else { > > - if (is_long_mode(vcpu)) > > - msr_bitmap = > > vmx_msr_bitmap_longmode_x2apic; > > - else > > - msr_bitmap = vmx_msr_bitmap_legacy_x2apic; > > + msr_bitmap = vmx_msr_bitmap_x2apic; > > } > > } else { > > - if (is_long_mode(vcpu)) > > - msr_bitmap = vmx_msr_bitmap_longmode; > > - else > > - msr_bitmap = vmx_msr_bitmap_legacy; > > + msr_bitmap = vmx_msr_bitmap_nox2apic; > > } > > > > vmcs_write64(MSR_BITMAP, __pa(msr_bitmap)); > > @@ -5211,22 +5194,24 @@ static void > > nested_vmx_disable_intercept_for_msr(unsigned long *msr_bitmap_l1, > > } > > } > > > > -static void vmx_disable_intercept_for_msr(u32 msr, bool longmode_only) > > +static void vmx_disable_intercept_for_msr(u32 msr) > > { > > - if (!longmode_only) > > - __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, > > - msr, MSR_TYPE_R | > > MSR_TYPE_W); > > - __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, > > - msr, MSR_TYPE_R | > > MSR_TYPE_W); > > + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_nox2apic, > > + msr, MSR_TYPE_R | MSR_TYPE_W); > > + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_x2apic, > > + msr, MSR_TYPE_R | MSR_TYPE_W); > > + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_x2apic_apicv, > > + msr, MSR_TYPE_R | MSR_TYPE_W); > > } > > > > -static void vmx_enable_intercept_for_msr(u32 msr, bool longmode_only) > > +static void vmx_enable_intercept_for_msr(u32 msr) > > { > > - if (!longmode_only) > > - __vmx_enable_intercept_for_msr(vmx_msr_bitmap_legacy, > > - msr, MSR_TYPE_R | > > MSR_TYPE_W); > > - __vmx_enable_intercept_for_msr(vmx_msr_bitmap_longmode, > > - msr, MSR_TYPE_R | > > MSR_TYPE_W); > > + __vmx_enable_intercept_for_msr(vmx_msr_bitmap_nox2apic, > > + msr, MSR_TYPE_R | MSR_TYPE_W); > > + __vmx_enable_intercept_for_msr(vmx_msr_bitmap_x2apic, > > + msr, MSR_TYPE_R | MSR_TYPE_W); > > + __vmx_enable_intercept_for_msr(vmx_msr_bitmap_x2apic_apicv, > > + msr, MSR_TYPE_R | MSR_TYPE_W); > > } > > > > static void pt_disable_intercept_for_msr(bool flag) > > @@ -5235,36 +5220,29 @@ static void pt_disable_intercept_for_msr(bool flag) > > unsigned int addr_num = kvm_get_pt_addr_cnt(); > > > > if (flag) { > > - vmx_disable_intercept_for_msr(MSR_IA32_RTIT_STATUS, false); > > - vmx_disable_intercept_for_msr(MSR_IA32_RTIT_OUTPUT_BASE, > > false); > > - vmx_disable_intercept_for_msr(MSR_IA32_RTIT_OUTPUT_MASK, > > false); > > - vmx_disable_intercept_for_msr(MSR_IA32_RTIT_CR3_MATCH, > > false); > > + vmx_disable_intercept_for_msr(MSR_IA32_RTIT_STATUS); > > + vmx_disable_intercept_for_msr(MSR_IA32_RTIT_OUTPUT_BASE); > > + vmx_disable_intercept_for_msr(MSR_IA32_RTIT_OUTPUT_MASK); > > + vmx_disable_intercept_for_msr(MSR_IA32_RTIT_CR3_MATCH); > > for (i = 0; i < addr_num; i++) > > - vmx_disable_intercept_for_msr(MSR_IA32_RTIT_ADDR0_A > > + i, > > - > > false); > > + vmx_disable_intercept_for_msr(MSR_IA32_RTIT_ADDR0_A > > + i); > > } else { > > - vmx_enable_intercept_for_msr(MSR_IA32_RTIT_STATUS, false); > > - vmx_enable_intercept_for_msr(MSR_IA32_RTIT_OUTPUT_BASE, > > false); > > - vmx_enable_intercept_for_msr(MSR_IA32_RTIT_OUTPUT_MASK, > > false); > > - vmx_enable_intercept_for_msr(MSR_IA32_RTIT_CR3_MATCH, > > false); > > + vmx_enable_intercept_for_msr(MSR_IA32_RTIT_STATUS); > > + vmx_enable_intercept_for_msr(MSR_IA32_RTIT_OUTPUT_BASE); > > + vmx_enable_intercept_for_msr(MSR_IA32_RTIT_OUTPUT_MASK); > > + vmx_enable_intercept_for_msr(MSR_IA32_RTIT_CR3_MATCH); > > for (i = 0; i < addr_num; i++) > > - vmx_enable_intercept_for_msr(MSR_IA32_RTIT_ADDR0_A > > + i, > > - > > false); > > + vmx_enable_intercept_for_msr(MSR_IA32_RTIT_ADDR0_A > > + i); > > } > > } > > > > static void vmx_disable_intercept_msr_x2apic(u32 msr, int type, bool > > apicv_only) > > { > > - __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic_apicv, > > + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_x2apic_apicv, > > msr, type); > > - > > __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic_apicv, > > - msr, type); > > - if (!apicv_only) { > > - > > __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic, > > - msr, type); > > - > > __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic, > > + if (!apicv_only) > > + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_x2apic, > > msr, type); > > - } > > } > > > > static bool vmx_get_enable_apicv(struct kvm_vcpu *vcpu) > > @@ -5733,7 +5711,7 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > > vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap)); > > } > > if (cpu_has_vmx_msr_bitmap()) > > - vmcs_write64(MSR_BITMAP, __pa(vmx_msr_bitmap_legacy)); > > + vmcs_write64(MSR_BITMAP, __pa(vmx_msr_bitmap_nox2apic)); > > > > vmcs_write64(VMCS_LINK_POINTER, -1ull); /* 22.3.1.5 */ > > > > @@ -7047,8 +7025,9 @@ static __init int hardware_setup(void) > > memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE); > > memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE); > > > > - memset(vmx_msr_bitmap_legacy, 0xff, PAGE_SIZE); > > - memset(vmx_msr_bitmap_longmode, 0xff, PAGE_SIZE); > > + memset(vmx_msr_bitmap_nox2apic, 0xff, PAGE_SIZE); > > + memset(vmx_msr_bitmap_x2apic, 0xff, PAGE_SIZE); > > + memset(vmx_msr_bitmap_x2apic_apicv, 0xff, PAGE_SIZE); > > > > if (setup_vmcs_config(&vmcs_config) < 0) { > > r = -EIO; > > @@ -7113,21 +7092,12 @@ static __init int hardware_setup(void) > > kvm_tsc_scaling_ratio_frac_bits = 48; > > } > > > > - vmx_disable_intercept_for_msr(MSR_FS_BASE, false); > > - vmx_disable_intercept_for_msr(MSR_GS_BASE, false); > > - vmx_disable_intercept_for_msr(MSR_KERNEL_GS_BASE, true); > > - vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false); > > - vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false); > > - vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false); > > - > > - memcpy(vmx_msr_bitmap_legacy_x2apic_apicv, > > - vmx_msr_bitmap_legacy, PAGE_SIZE); > > - memcpy(vmx_msr_bitmap_longmode_x2apic_apicv, > > - vmx_msr_bitmap_longmode, PAGE_SIZE); > > - memcpy(vmx_msr_bitmap_legacy_x2apic, > > - vmx_msr_bitmap_legacy, PAGE_SIZE); > > - memcpy(vmx_msr_bitmap_longmode_x2apic, > > - vmx_msr_bitmap_longmode, PAGE_SIZE); > > + vmx_disable_intercept_for_msr(MSR_FS_BASE); > > + vmx_disable_intercept_for_msr(MSR_GS_BASE); > > + vmx_disable_intercept_for_msr(MSR_KERNEL_GS_BASE); > > + vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS); > > + vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP); > > + vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP); > > > > set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */ > > > > -- > > 1.8.3.1 > > >