Attached patch is modified according your comments to use 4 msr bitmaps. Please help to review it.
Also, is there any comments for the other part of this series? If yes, welcome. :) Gleb Natapov wrote on 2013-01-14: > On Mon, Jan 14, 2013 at 11:25:45AM +0000, Zhang, Yang Z wrote: >> Gleb Natapov wrote on 2013-01-14: >>> On Mon, Jan 14, 2013 at 11:10:26AM +0000, Zhang, Yang Z wrote: >>>> Gleb Natapov wrote on 2013-01-14: >>>>> On Mon, Jan 14, 2013 at 11:01:02AM +0000, Zhang, Yang Z wrote: >>>>>> Gleb Natapov wrote on 2013-01-14: >>>>>>> On Mon, Jan 14, 2013 at 03:13:34PM +0800, Yang Zhang wrote: >>>>>>>> From: Yang Zhang <[email protected]> >>>>>>>> >>>>>>>> basically to benefit from apicv, we need to enable virtualized >>>>>>>> x2apic mode. Currently, we only enable it when guest is really >>>>>>>> using x2apic. >>>>>>>> >>>>>>>> Also, clear MSR bitmap for corresponding x2apic MSRs when guest >>> enabled >>>>>>> x2apic: >>>>>>>> 0x800 - 0x8ff: no read intercept for apicv register virtualization, >>>>>>>> except APIC ID and TMCCT. >>>>>>>> APIC ID and TMCCT: need software's assistance to get right value. >>>>>>> Actually since msr bitmap is shared between all vcpus this will break >>>>>>> guests that do not enable x2apic. >>>>>> I don't think this case will exist. It will break the real OS too. >>>>>> >>>>> Which case? One VM uses x2apic another one does not? Bitmap is shared >>>>> between all vcpus of all VMs. >>>> Sorry. I misread your comments. >>>> >>> I miswrote it. Forgot to include "of all VMs" there. >>> >>>> Yes, it is really a problem. Maybe we need to use per VM msr bitmap instead >>> global bitmap. >>> Are you sure cpus cannot be in different modes during boot and smp >>> initialization? Where spec says that? >> I don't think hardware has this limitation. I mean use per vcpu's msr >> bitmap instead global bitmap. >> > Even if HW has such limitation we cannot have per VM bitmap since > malicious guest can abuse it. > > Per cpu msr bitmap means that each vcpu will waste one more page of > memory. We already have different global msr bitmap for long mode > and legacy mode, may be make it 4: x2apic X long X legacy. > >>>> >>>>>>>> Signed-off-by: Kevin Tian <[email protected]> >>>>>>>> Signed-off-by: Yang Zhang <[email protected]> >>>>>>>> --- >>>>>>>> arch/x86/include/asm/kvm_host.h | 1 + >>> arch/x86/include/asm/vmx.h >>>>>>>> | 1 + arch/x86/kvm/lapic.c | 15 +++- >>>>>>>> arch/x86/kvm/svm.c | 6 ++ arch/x86/kvm/vmx.c >>>>>>>> | 162 +++++++++++++++++++++++++++++++++++++-- 5 files >>>>> changed, 173 >>>>>>>> insertions(+), 12 deletions(-) >>>>>>>> diff --git a/arch/x86/include/asm/kvm_host.h >>>>>>>> b/arch/x86/include/asm/kvm_host.h index c431b33..35aa8e6 100644 >>>>>>>> --- a/arch/x86/include/asm/kvm_host.h +++ >>>>>>>> b/arch/x86/include/asm/kvm_host.h @@ -697,6 +697,7 @@ struct >>>>>>>> kvm_x86_ops { >>>>>>>> void (*enable_nmi_window)(struct kvm_vcpu *vcpu); void >>>>>>>> (*enable_irq_window)(struct kvm_vcpu *vcpu); void >>>>>>>> (*update_cr8_intercept)(struct kvm_vcpu *vcpu, int tpr, int >>>>>>>> irr); + void (*set_virtual_x2apic_mode)(struct kvm_vcpu *vcpu, >>>>>>>> bool set); int (*set_tss_addr)(struct kvm *kvm, unsigned int >>>>>>>> addr); int (*get_tdp_level)(void); u64 >>>>>>>> (*get_mt_mask)(struct >>>>>>>> kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio); >>>>>>>> diff --git a/arch/x86/include/asm/vmx.h >>>>>>>> b/arch/x86/include/asm/vmx.h index 44c3f7e..0a54df0 100644 --- >>>>>>>> a/arch/x86/include/asm/vmx.h +++ b/arch/x86/include/asm/vmx.h @@ >>>>>>>> -139,6 +139,7 @@ >>>>>>>> #define SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES 0x00000001 >>>>>>>> #define SECONDARY_EXEC_ENABLE_EPT 0x00000002 >>>>>>>> #define SECONDARY_EXEC_RDTSCP 0x00000008 +#define >>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE 0x00000010 #define >>>>>>>> SECONDARY_EXEC_ENABLE_VPID 0x00000020 #define >>>>>>>> SECONDARY_EXEC_WBINVD_EXITING 0x00000040 #define >>>>>>>> SECONDARY_EXEC_UNRESTRICTED_GUEST 0x00000080 >>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >>>>>>>> index 0664c13..2ef5e2b 100644 >>>>>>>> --- a/arch/x86/kvm/lapic.c >>>>>>>> +++ b/arch/x86/kvm/lapic.c >>>>>>>> @@ -1323,12 +1323,17 @@ void kvm_lapic_set_base(struct kvm_vcpu >>>>> *vcpu, >>>>>>> u64 value) >>>>>>>> if (!kvm_vcpu_is_bsp(apic->vcpu)) >>>>>>>> value &= ~MSR_IA32_APICBASE_BSP; >>>>>>>> - vcpu->arch.apic_base = value; >>>>>>>> - if (apic_x2apic_mode(apic)) { >>>>>>>> - u32 id = kvm_apic_id(apic); >>>>>>>> - u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf)); >>>>>>>> - kvm_apic_set_ldr(apic, ldr); >>>>>>>> + if ((vcpu->arch.apic_base ^ value) & X2APIC_ENABLE) { >>>>>>>> + if (value & X2APIC_ENABLE) { >>>>>>>> + u32 id = kvm_apic_id(apic); >>>>>>>> + u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf)); >>>>>>>> + kvm_apic_set_ldr(apic, ldr); >>>>>>>> + kvm_x86_ops->set_virtual_x2apic_mode(vcpu, >>>>>>>> true); >>>>>>>> + } else >>>>>>>> + kvm_x86_ops->set_virtual_x2apic_mode(vcpu, >>>>>>>> false); >>>>>>>> } >>>>>>>> + >>>>>>>> + vcpu->arch.apic_base = value; >>>>>>>> apic->base_address = apic->vcpu->arch.apic_base & >>>>>>>> MSR_IA32_APICBASE_BASE; >>>>>>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >>>>>>>> index d29d3cd..38407e9 100644 >>>>>>>> --- a/arch/x86/kvm/svm.c >>>>>>>> +++ b/arch/x86/kvm/svm.c >>>>>>>> @@ -3571,6 +3571,11 @@ static void update_cr8_intercept(struct >>>>> kvm_vcpu >>>>>>> *vcpu, int tpr, int irr) >>>>>>>> set_cr_intercept(svm, INTERCEPT_CR8_WRITE); >>>>>>>> } >>>>>>>> +static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, >>>>>>>> bool set) +{ + return; +} + >>>>>>>> static int svm_nmi_allowed(struct kvm_vcpu *vcpu) { struct >>>>>>>> vcpu_svm *svm = to_svm(vcpu); @@ -4290,6 +4295,7 @@ static >>>>>>>> struct kvm_x86_ops svm_x86_ops = { .enable_nmi_window = >>>>>>>> enable_nmi_window, .enable_irq_window = enable_irq_window, >>>>>>>> .update_cr8_intercept = update_cr8_intercept, >>>>>>>> + .set_virtual_x2apic_mode = svm_set_virtual_x2apic_mode, >>>>>>>> >>>>>>>> .set_tss_addr = svm_set_tss_addr, >>>>>>>> .get_tdp_level = get_npt_level, >>>>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>>>>>>> index 0403634..847022e 100644 >>>>>>>> --- a/arch/x86/kvm/vmx.c >>>>>>>> +++ b/arch/x86/kvm/vmx.c >>>>>>>> @@ -767,6 +767,12 @@ static inline bool >>>>>>> cpu_has_vmx_virtualize_apic_accesses(void) >>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; >>>>>>>> } >>>>>>>> +static inline bool cpu_has_vmx_virtualize_x2apic_mode(void) >>>>>>>> +{ >>>>>>>> + return vmcs_config.cpu_based_2nd_exec_ctrl & >>>>>>>> + SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; >>>>>>>> +} >>>>>>>> + >>>>>>>> static inline bool cpu_has_vmx_apic_register_virt(void) >>>>>>>> { >>>>>>>> return vmcs_config.cpu_based_2nd_exec_ctrl & >>>>>>>> @@ -2543,6 +2549,7 @@ static __init int setup_vmcs_config(struct >>>>>>> vmcs_config *vmcs_conf) >>>>>>>> if (_cpu_based_exec_control & >>>>>>>> CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) { min2 = 0; >>>>>>>> opt2 = >>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES | >>>>>>>> + SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE | >>>>>>>> SECONDARY_EXEC_WBINVD_EXITING | >>>>>>>> SECONDARY_EXEC_ENABLE_VPID | >>>>>>>> SECONDARY_EXEC_ENABLE_EPT | @@ -3724,7 +3731,45 >>>>>>>> @@ static >>>>>>>> void free_vpid(struct vcpu_vmx *vmx) > spin_unlock(&vmx_vpid_lock); } >>>>>>>> -static void __vmx_disable_intercept_for_msr(unsigned long >>>>>>>> *msr_bitmap, u32 msr) +#define MSR_TYPE_R 1 +#define MSR_TYPE_W >>>>>>>> 2 >>>>>>>> +static void __vmx_disable_intercept_for_msr(unsigned long >>>>>>>> *msr_bitmap, + u32 msr, int >>>>>>>> type) +{ + int f = >>>>>>>> sizeof(unsigned long); + + if (!cpu_has_vmx_msr_bitmap()) >>>>>>>> + return; + + /* + * See Intel PRM Vol. 3, 20.6.9 >>>>>>>> (MSR-Bitmap >>>>>>>> Address). Early manuals + >>>>>>>> >>>>>>>> * have the write-low and read-high bitmap offsets the wrong way >>>>>>>> round. + * We can control MSRs 0x00000000-0x00001fff and >>>>>>>> 0xc0000000-0xc0001fff. + */ + if (msr <= 0x1fff) { + >>>>>>>> if (type & >>>>>>>> MSR_TYPE_R) + /* read-low */ + >>>>>>>> __clear_bit(msr, msr_bitmap + >>>>>>>> 0x000 / f); + + if (type & MSR_TYPE_W) + >>>>>>>> /* write-low */ + >>>>>>>> __clear_bit(msr, msr_bitmap + 0x800 / f); + + } else if ((msr >>>>>>>> >= >>>>>>>> 0xc0000000) && (msr <= 0xc0001fff)) { + msr &= 0x1fff; >>>>>>>> + if >>>>>>>> (type & MSR_TYPE_R) + /* read-high */ + >>>>>>>> __clear_bit(msr, >>>>>>>> msr_bitmap + 0x400 / f); + + if (type & MSR_TYPE_W) + >>>>>>>> /* >>>>>>>> write-high */ + __clear_bit(msr, msr_bitmap + 0xc00 / f); + + >>>>>>>> } >>>>>>>> +} + +static void __vmx_enable_intercept_for_msr(unsigned long >>>>>>>> *msr_bitmap, + u32 msr, int type) >>>>>>>> { >>>>>>>> int f = sizeof(unsigned long); >>>>>>>> @@ -3737,20 +3782,75 @@ static void >>>>>>> __vmx_disable_intercept_for_msr(unsigned long *msr_bitmap, u32 > msr) >>>>>>>> * We can control MSRs 0x00000000-0x00001fff and >>>>>>>> 0xc0000000-0xc0001fff. */ if (msr <= 0x1fff) { >>>>>>>> - __clear_bit(msr, msr_bitmap + 0x000 / f); /* read-low */ >>>>>>>> - __clear_bit(msr, msr_bitmap + 0x800 / f); /* write-low >>>>>>>> */ >>>>>>>> + if (type & MSR_TYPE_R) >>>>>>>> + /* read-low */ >>>>>>>> + __set_bit(msr, msr_bitmap + 0x000 / f); >>>>>>>> + >>>>>>>> + if (type & MSR_TYPE_W) >>>>>>>> + /* write-low */ >>>>>>>> + __set_bit(msr, msr_bitmap + 0x800 / f); >>>>>>>> + >>>>>>>> } else if ((msr >= 0xc0000000) && (msr <= 0xc0001fff)) { >>>>>>>> msr &= 0x1fff; >>>>>>>> - __clear_bit(msr, msr_bitmap + 0x400 / f); /* read-high >>>>>>>> */ >>>>>>>> - __clear_bit(msr, msr_bitmap + 0xc00 / f); /* write-high >>>>>>>> */ >>>>>>>> + if (type & MSR_TYPE_R) >>>>>>>> + /* read-high */ >>>>>>>> + __set_bit(msr, msr_bitmap + 0x400 / f); >>>>>>>> + >>>>>>>> + if (type & MSR_TYPE_W) >>>>>>>> + /* write-high */ >>>>>>>> + __set_bit(msr, msr_bitmap + 0xc00 / f); >>>>>>>> + >>>>>>>> } } + static void vmx_disable_intercept_for_msr(u32 msr, bool >>>>>>>> longmode_only) { if (!longmode_only) >>>>>>>> - __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, msr); >>>>>>>> - __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, msr); >>>>>>>> + __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); +} + +static void >>>>>>>> vmx_intercept_for_msr_read(u32 msr, bool longmode_only, + >>>>>>>> bool >>>>>>>> set) +{ + if (!longmode_only) { + if (set) + >>>>>>>> __vmx_enable_intercept_for_msr(vmx_msr_bitmap_legacy, + >>>>>>>> msr, >>>>>>>> MSR_TYPE_R); + else + >>>>>>>> __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, + >>>>>>>> msr, MSR_TYPE_R); + + } + if >>>>>>>> (set) + >>>>>>>> __vmx_enable_intercept_for_msr(vmx_msr_bitmap_longmode, >>>>>>>> + msr, MSR_TYPE_R); + else + >>>>>>>> __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, >>>>>>>> + msr, MSR_TYPE_R); +} + +static void >>>>>>>> vmx_intercept_for_msr_write(u32 msr, bool longmode_only, + >>>>>>>> bool set) +{ + if (!longmode_only) { + >>>>>>>> if (set) + >>>>>>>> __vmx_enable_intercept_for_msr(vmx_msr_bitmap_legacy, + >>>>>>>> msr, MSR_TYPE_W); + >>>>>>>> else + >>>>>>>> __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, + >>>>>>>> msr, MSR_TYPE_W); + + } + if >>>>>>>> (set) + >>>>>>>> __vmx_enable_intercept_for_msr(vmx_msr_bitmap_longmode, >>>>>>>> + msr, MSR_TYPE_W); + else + >>>>>>>> __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, >>>>>>>> + msr, MSR_TYPE_W); >>>>>>>> } >>>>>>>> >>>>>>>> /* >>>>>>>> @@ -3848,6 +3948,7 @@ static u32 > vmx_secondary_exec_control(struct >>>>>>> vcpu_vmx *vmx) >>>>>>>> exec_control &= ~SECONDARY_EXEC_PAUSE_LOOP_EXITING; >>>>>>>> if >>>>>>>> (!enable_apicv_reg) exec_control &= >>>>>>>> ~SECONDARY_EXEC_APIC_REGISTER_VIRT; + exec_control &= >>>>>>>> ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; return >>>>> exec_control; } >>>>>>>> @@ -6103,6 +6204,52 @@ static void update_cr8_intercept(struct >>>>> kvm_vcpu >>>>>>> *vcpu, int tpr, int irr) >>>>>>>> vmcs_write32(TPR_THRESHOLD, irr); >>>>>>>> } >>>>>>>> +static void vmx_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, >>>>>>>> bool set) +{ + u32 exec_control, sec_exec_control; + int msr; >>>>>>>> + struct vcpu_vmx *vmx = to_vmx(vcpu); + + /* There is not >>>>>>>> point >>>>>>>> to enable virtualize x2apic without enable + * apicv*/ + if >>>>>>>> (!cpu_has_vmx_virtualize_x2apic_mode() || !enable_apicv_reg) + >>>>>>>> return; + + if (set) { + exec_control = >>>>>>>> vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); + /* virtualize >>>>>>>> x2apic >>>>>>>> mode relies on tpr shadow */ + if (!(exec_control & >>>>>>>> CPU_BASED_TPR_SHADOW)) + return; + } + + >>>>>>>> sec_exec_control = >>>>>>>> vmcs_read32(SECONDARY_VM_EXEC_CONTROL); + + if (set) { >>>>>>>> + sec_exec_control &= >>>>>>>> ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; >>>>>>>> + sec_exec_control |= >>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; + } >>>>>>>> else { + sec_exec_control &= >>>>>>>> ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; + if >>>>>>>> (vm_need_virtualize_apic_accesses(vmx->vcpu.kvm)) + >>>>>>>> sec_exec_control |= + SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; >>>>>>>> + } + vmcs_write32(SECONDARY_VM_EXEC_CONTROL, >>>>>>>> sec_exec_control); >>>>>>>> + + for (msr = 0x800; msr <= 0x8ff; msr++) + >>>>>>>> vmx_intercept_for_msr_read(msr, false, !set); + + if >>>>>>>> (set) { + >>>>>>>> /* According SDM, in x2apic mode, the whole id reg is used. + >>>>>>>> >>>>>>>> * But in KVM, it only use the highest eight bits. Need to + >>>>>>>> * >>>>>>>> intercept it*/ + vmx_intercept_for_msr_read(0x802, false, true); >>>>>>>> + /* TMCCT */ + vmx_intercept_for_msr_read(0x839, false, true); >>>>>>>> + } + /* TPR */ + vmx_intercept_for_msr_write(0x808, >>>>>>>> false, >>>>>>>> !set); +} + >>>>>>>> static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx) { >>>>>>>> u32 exit_intr_info; @@ -7366,6 +7513,7 @@ static struct >>>>>>>> kvm_x86_ops vmx_x86_ops = { .enable_nmi_window = >>>>>>>> enable_nmi_window, .enable_irq_window = enable_irq_window, >>>>>>>> .update_cr8_intercept = update_cr8_intercept, >>>>>>>> + .set_virtual_x2apic_mode = vmx_set_virtual_x2apic_mode, >>>>>>>> >>>>>>>> .set_tss_addr = vmx_set_tss_addr, >>>>>>>> .get_tdp_level = get_ept_level, >>>>>>>> -- >>>>>>>> 1.7.1 >>>>>>> >>>>>>> -- >>>>>>> Gleb. >>>>>> >>>>>> >>>>>> Best regards, >>>>>> Yang >>>>> >>>>> -- >>>>> Gleb. >>>> >>>> >>>> Best regards, >>>> Yang >>>> >>> >>> -- >>> Gleb. >> >> >> Best regards, >> Yang >> > > -- > Gleb. Best regards, Yang
x2apic_revise.patch
Description: x2apic_revise.patch
