Re: [PATCH v3] kvm:vmx: more complete state update on APICv on/off

[Yang Zhang <yang.zhang.wz@gmail.com>]

On 2016/5/18 22:48, Roman Kagan wrote:
> The function to update APICv on/off state (in particular, to deactivate
> it when enabling Hyper-V SynIC), used to be incomplete: it didn't adjust
> APICv-related fields among secondary processor-based VM-execution
> controls.
>
> As a result, Windows 2012 guests would get stuck when SynIC-based
> auto-EOI interrupt intersected with e.g. an IPI in the guest.
>
> In addition, the MSR intercept bitmap wasn't updated to correspond to
> whether "virtualize x2APIC mode" was enabled.  This path used not to be
> triggered, since Windows didn't use x2APIC but rather their own
> synthetic APIC access MSRs; however it represented a security risk
> because the guest running in a SynIC-enabled VM could switch to x2APIC
> and thus obtain direct access to host APIC MSRs (thanks to Yang Zhang
> <yang.zhang.wz@gmail.com> for spotting this).
>
> The patch fixes those omissions.
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Steve Rutherford <srutherford@google.com>
> Cc: Yang Zhang <yang.zhang.wz@gmail.com>
> ---
> v2 -> v3:
>   - only switch to x2apic msr bitmap if virtualize x2apic mode is on in vmcs
>
> v1 -> v2:
>   - only update relevant bits in the secondary exec control
>   - update msr intercept bitmap (also make x2apic msr bitmap always
>     correspond to APICv)
>
>   arch/x86/kvm/vmx.c | 48 ++++++++++++++++++++++++++++++------------------
>   1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ee1c8a9..cef741a 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2418,7 +2418,9 @@ static void vmx_set_msr_bitmap(struct kvm_vcpu *vcpu)
>
>   	if (is_guest_mode(vcpu))
>   		msr_bitmap = vmx_msr_bitmap_nested;
> -	else if (vcpu->arch.apic_base & X2APIC_ENABLE) {
> +	else if (cpu_has_secondary_exec_ctrls() &&
> +		 (vmcs_read32(SECONDARY_VM_EXEC_CONTROL) &
> +		  SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE)) {
>   		if (is_long_mode(vcpu))
>   			msr_bitmap = vmx_msr_bitmap_longmode_x2apic;
>   		else
> @@ -4783,6 +4785,19 @@ static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>
>   	vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_ctrl(vmx));
> +	if (cpu_has_secondary_exec_ctrls()) {
> +		if (kvm_vcpu_apicv_active(vcpu))
> +			vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
> +				      SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +				      SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> +		else
> +			vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
> +					SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +					SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> +	}
> +
> +	if (cpu_has_vmx_msr_bitmap())
> +		vmx_set_msr_bitmap(vcpu);
>   }
>
>   static u32 vmx_exec_control(struct vcpu_vmx *vmx)
> @@ -6329,23 +6344,20 @@ static __init int hardware_setup(void)
>
>   	set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
>
> -	if (enable_apicv) {
> -		for (msr = 0x800; msr <= 0x8ff; msr++)
> -			vmx_disable_intercept_msr_read_x2apic(msr);
> -
> -		/* 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_enable_intercept_msr_read_x2apic(0x802);
> -		/* 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);
> -	}
> +	for (msr = 0x800; msr <= 0x8ff; msr++)
> +		vmx_disable_intercept_msr_read_x2apic(msr);
> +
> +	/* 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_enable_intercept_msr_read_x2apic(0x802);
> +	/* 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);

In current KVM, it will enable virtualize x2apic mode only when 
enable_apicv is enabled. So this change seems unnecessary.

Except this minor comment, the other part is :

Reviewed-by: Yang Zhang <yang.zhang.wz@gmail.com>

-- 
best regards
yang