Re: [PATCH] KVM: VMX: Enable MSR-BASED TPR shadow even if w/o APICv

[Mika PenttilÃ]

On 09/14/2016 10:58 AM, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> 
> I observed that kvmvapic(to optimize flexpriority=N or AMD) is used 
> to boost TPR access when testing kvm-unit-test/eventinj.flat tpr case
> on my haswell desktop (w/ flexpriority, w/o APICv). Commit (8d14695f9542 
> x86, apicv: add virtual x2apic support) disable virtual x2apic mode 
> completely if w/o APICv, and the author also told me that windows guest
> can't enter into x2apic mode when he developed the APICv feature several 
> years ago. However, it is not truth currently, Interrupt Remapping and 
> vIOMMU is added to qemu and the developers from Intel test windows 8 can 
> work in x2apic mode w/ Interrupt Remapping enabled recently. 
> 
> This patch enables TPR shadow for virtual x2apic mode to boost 
> windows guest in x2apic mode even if w/o APICv.
> 
> Can pass the kvm-unit-test.
> 

While at it, is the vmx flexpriotity stuff still valid code?
AFAICS it gets enabled iff TPR shadow is on. flexpriority
is on when :

(flexpriority_enabled && lapic_in_kernel && cpu_has_vmx_tpr_shadow && cpu_has_vmx_virtualize_apic_accesses)

But apic accesses to TPR mmio are not then trapped and TPR changes not reported because
the âuse TPR shadowâ VM-execution control is 1.

Thanks,
Mika


> Suggested-by: Wincy Van <fanwenyi0529@xxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: Wincy Van <fanwenyi0529@xxxxxxxxx>
> Cc: Yang Zhang <yang.zhang.wz@xxxxxxxxx>
> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> ---
>  arch/x86/kvm/vmx.c | 41 ++++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 5cede40..e703129 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6336,7 +6336,7 @@ static void wakeup_handler(void)
>  
>  static __init int hardware_setup(void)
>  {
> -	int r = -ENOMEM, i, msr;
> +	int r = -ENOMEM, i;
>  
>  	rdmsrl_safe(MSR_EFER, &host_efer);
>  
> @@ -6464,18 +6464,6 @@ static __init int hardware_setup(void)
>  
>  	set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
>  
> -	for (msr = 0x800; msr <= 0x8ff; msr++)
> -		vmx_disable_intercept_msr_read_x2apic(msr);
> -
> -	/* TMCCT */
> -	vmx_enable_intercept_msr_read_x2apic(0x839);
> -	/* TPR */
> -	vmx_disable_intercept_msr_write_x2apic(0x808);
> -	/* EOI */
> -	vmx_disable_intercept_msr_write_x2apic(0x80b);
> -	/* SELF-IPI */
> -	vmx_disable_intercept_msr_write_x2apic(0x83f);
> -
>  	if (enable_ept) {
>  		kvm_mmu_set_mask_ptes(VMX_EPT_READABLE_MASK,
>  			(enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
> @@ -8435,12 +8423,7 @@ static void vmx_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
>  		return;
>  	}
>  
> -	/*
> -	 * There is not point to enable virtualize x2apic without enable
> -	 * apicv
> -	 */
> -	if (!cpu_has_vmx_virtualize_x2apic_mode() ||
> -				!kvm_vcpu_apicv_active(vcpu))
> +	if (!cpu_has_vmx_virtualize_x2apic_mode())
>  		return;
>  
>  	if (!cpu_need_tpr_shadow(vcpu))
> @@ -8449,8 +8432,28 @@ static void vmx_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
>  	sec_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
>  
>  	if (set) {
> +		int msr;
> +
>  		sec_exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
>  		sec_exec_control |= SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> +
> +		if (kvm_vcpu_apicv_active(vcpu)) {
> +			for (msr = 0x800; msr <= 0x8ff; msr++)
> +				vmx_disable_intercept_msr_read_x2apic(msr);
> +
> +			/* TMCCT */
> +			vmx_enable_intercept_msr_read_x2apic(0x839);
> +			/* TPR */
> +			vmx_disable_intercept_msr_write_x2apic(0x808);
> +			/* EOI */
> +			vmx_disable_intercept_msr_write_x2apic(0x80b);
> +			/* SELF-IPI */
> +			vmx_disable_intercept_msr_write_x2apic(0x83f);
> +		} else if (vmx_exec_control(to_vmx(vcpu)) & CPU_BASED_TPR_SHADOW) {
> +			/* TPR */
> +			vmx_disable_intercept_msr_read_x2apic(0x808);
> +			vmx_disable_intercept_msr_write_x2apic(0x808);
> +		}
>  	} else {
>  		sec_exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
>  		sec_exec_control |= SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
>