RE: [PATCH v8 3/3] x86, apicv: add virtual x2apic support

Zhang, Yang Z
2013-01-07T06:58:23Z

Gleb Natapov wrote on 2013-01-07:
> On Mon, Jan 07, 2013 at 10:02:37AM +0800, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>
>> basically to benefit from apicv, we need clear MSR bitmap for
>> corresponding x2apic MSRs when guest enabled x2apic:
>> 0x800 - 0x8ff: no read intercept for apicv register virtualization
>> TPR,EOI,SELF-IPI: no write intercept for virtual interrupt delivery
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 + arch/x86/include/asm/vmx.h
>> | 1 + arch/x86/kvm/lapic.c | 2 + arch/x86/kvm/svm.c
>> | 6 +++ arch/x86/kvm/vmx.c | 80
>> +++++++++++++++++++++++++++++++++++--- 5 files changed, 83
>> insertions(+), 7 deletions(-)
>> diff --git a/arch/x86/include/asm/kvm_host.h
>> b/arch/x86/include/asm/kvm_host.h index 135603f..af9a8c3 100644 ---
>> a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -704,6 +704,7 @@ struct kvm_x86_ops {
>> void (*update_exitmap_end)(struct kvm_vcpu *vcpu); void
>> (*load_eoi_exitmap)(struct kvm_vcpu *vcpu); void
>> (*restore_rvi)(struct kvm_vcpu *vcpu); + void
>> (*enable_virtual_x2apic_mode)(struct kvm_vcpu *vcpu); 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 d1ab331..694586c 100644
>> --- a/arch/x86/include/asm/vmx.h
>> +++ b/arch/x86/include/asm/vmx.h
>> @@ -140,6 +140,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 e1baf37..dba5300 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1349,6 +1349,8 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64
> value)
>> u32 id = kvm_apic_id(apic);
>> u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));
>> kvm_apic_set_ldr(apic, ldr);
>> + kvm_x86_ops->enable_virtual_x2apic_mode(vcpu);
> And where do you disable it?
Yes, need to disable it when guest rolls back to xapic mode. Will add it in next patch.

>> +
>> }
>> 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 a8a8a4e..3e34e19 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -3601,6 +3601,11 @@ static void svm_restore_rvi(struct kvm_vcpu *vcpu)
>> return;
>> }
>> +static void svm_enable_virtual_x2apic_mode(struct kvm_vcpu *vcpu)
>> +{
>> + return;
>> +}
>> +
>> static int svm_nmi_allowed(struct kvm_vcpu *vcpu) { struct vcpu_svm
>> *svm = to_svm(vcpu); @@ -4326,6 +4331,7 @@ static struct kvm_x86_ops
>> svm_x86_ops = { .update_exitmap_end = svm_update_exitmap_end,
>> .load_eoi_exitmap = svm_load_eoi_exitmap, .restore_rvi =
>> svm_restore_rvi,
>> + .enable_virtual_x2apic_mode = svm_enable_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 0c85c7e..466b05d 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2551,6 +2551,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 | @@ -3739,7 +3740,10 @@ 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);
>> @@ -3752,20 +3756,52 @@ 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 */
>> + __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;
>> - __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 */
>> + __clear_bit(msr, msr_bitmap + 0x400 / f);
>> +
>> + if (type & MSR_TYPE_W)
>> + /* write-high */
>> + __clear_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_disable_intercept_for_msr_read(u32 msr, bool longmode_only) +{ + if
>> (!longmode_only)
>> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, + msr,
>> MSR_TYPE_R); + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode,
>> + msr, MSR_TYPE_R); +} + +static void
>> vmx_disable_intercept_for_msr_write(u32 msr, bool longmode_only) +{
>> + if (!longmode_only)
>> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, + msr,
>> MSR_TYPE_W); + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode,
>> + msr, MSR_TYPE_W);
>> }
>>
>> /*
>> @@ -3864,6 +3900,7 @@ static u32 vmx_secondary_exec_control(struct
> vcpu_vmx *vmx)
>> if (!enable_apicv_reg_vid) exec_control &=
>> ~(SECONDARY_EXEC_APIC_REGISTER_VIRT |
>> SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY); + exec_control &=
>> ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; return exec_control; }
>> @@ -6274,6 +6311,34 @@ static void vmx_load_eoi_exitmap(struct kvm_vcpu
> *vcpu)
>> spin_unlock(&vmx->eoi_bitmap_lock);
>> }
>> +static void vmx_enable_virtual_x2apic_mode(struct kvm_vcpu *vcpu)
>> +{
>> + u32 exec_control;
>> + int msr;
>> +
>> + if (!enable_apicv_reg_vid)
>> + return;
>> +
>> + exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
>> + if (!(exec_control & CPU_BASED_TPR_SHADOW))
>> + return;
>> + exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
>> + exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
>> + exec_control |= SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
>> +
>> + vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
>> +
>> + for (msr = 0x800; msr <= 0x8ff; msr++)
>> + vmx_disable_intercept_for_msr_read(msr, false);
>> +
>> + /* TPR */
>> + vmx_disable_intercept_for_msr_write(0x808, false);
>> + /* EOI */
>> + vmx_disable_intercept_for_msr_write(0x80b, false);
>> + /* SELF-IPI */
>> + vmx_disable_intercept_for_msr_write(0x83f, false);
>> +}
>> +
>> static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx) { u32
>> exit_intr_info; @@ -7544,6 +7609,7 @@ static struct kvm_x86_ops
>> vmx_x86_ops = { .update_exitmap_end = vmx_update_exitmap_end,
>> .load_eoi_exitmap = vmx_load_eoi_exitmap, .restore_rvi =
>> vmx_restore_rvi,
>> + .enable_virtual_x2apic_mode = vmx_enable_virtual_x2apic_mode,
>>
>> .set_tss_addr = vmx_set_tss_addr,
>> .get_tdp_level = get_ept_level,
>> --
>> 1.7.1
>
> --
> Gleb.

Best regards,
Yang