From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Zhang Subject: Re: [PATCH v3] kvm:vmx: more complete state update on APICv on/off Date: Thu, 19 May 2016 09:38:45 +0800 Message-ID: <573D1925.3070108@gmail.com> References: <1463582900-22620-1-git-send-email-rkagan@virtuozzo.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: Paolo Bonzini , "Denis V. Lunev" , Steve Rutherford To: Roman Kagan , kvm@vger.kernel.org Return-path: Received: from mail-oi0-f65.google.com ([209.85.218.65]:35272 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752691AbcESBiv (ORCPT ); Wed, 18 May 2016 21:38:51 -0400 Received: by mail-oi0-f65.google.com with SMTP id w198so13471805oiw.2 for ; Wed, 18 May 2016 18:38:50 -0700 (PDT) In-Reply-To: <1463582900-22620-1-git-send-email-rkagan@virtuozzo.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > for spotting this). > > The patch fixes those omissions. > > Signed-off-by: Roman Kagan > Cc: Steve Rutherford > Cc: Yang Zhang > --- > 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 -- best regards yang