* [PATCH v2] kvm:vmx: more complete state update on APICv on/off
@ 2016-05-17 10:36 Roman Kagan
2016-05-18 5:24 ` Yang Zhang
0 siblings, 1 reply; 3+ messages in thread
From: Roman Kagan @ 2016-05-17 10:36 UTC (permalink / raw)
To: kvm; +Cc: Paolo Bonzini, Denis V. Lunev, Roman Kagan, Steve Rutherford
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 to be
untriggerable, since Windows didn't use x2APIC but rather their own
synthetic APIC access MSRs.]
The patch fixes those omissions.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: Steve Rutherford <srutherford@google.com>
---
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 | 47 +++++++++++++++++++++++++++++------------------
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ee1c8a9..5f9701e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2418,7 +2418,8 @@ 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 (vcpu->arch.apic_base & X2APIC_ENABLE &&
+ kvm_vcpu_apicv_active(vcpu)) {
if (is_long_mode(vcpu))
msr_bitmap = vmx_msr_bitmap_longmode_x2apic;
else
@@ -4783,6 +4784,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 +6343,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);
if (enable_ept) {
kvm_mmu_set_mask_ptes(0ull,
--
2.5.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] kvm:vmx: more complete state update on APICv on/off
2016-05-17 10:36 [PATCH v2] kvm:vmx: more complete state update on APICv on/off Roman Kagan
@ 2016-05-18 5:24 ` Yang Zhang
2016-05-18 8:37 ` Roman Kagan
0 siblings, 1 reply; 3+ messages in thread
From: Yang Zhang @ 2016-05-18 5:24 UTC (permalink / raw)
To: Roman Kagan, kvm; +Cc: Paolo Bonzini, Denis V. Lunev, Steve Rutherford
On 2016/5/17 18:36, Roman Kagan wrote:
During reviewing this patch, I think there is an secure issue in
upstream KVM since the 5c919412fe61c:
1. The enable_apicv is setting to 1 if the underling hardware has APICv
capability.
2. set the x2apic_read/write msr to disable the interception for most
x2apic read and TPR/EOI/SELF-IPI write.
3. vmx_set_virtual_x2apic_mode is called when guest enables the
x2apic,but kvm_vcpu_apicv_active() check may fail if synIc is
activated.This means the virtualize_x2apic_mode will not set even guest
enabling the x2apic.
4. In the following vmx_set_msr_bitmap, it will use the x2apic msr
bitmap without check whether virtualize_x2apic_mode is enabled and apicv
is enabled.
5. Then the access to the msr that doesn't intercept by hypervisor will
access the hardware directly.
> 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 to be
> untriggerable, since Windows didn't use x2APIC but rather their own
> synthetic APIC access MSRs.]
>
> The patch fixes those omissions.
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Steve Rutherford <srutherford@google.com>
> ---
> 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 | 47 +++++++++++++++++++++++++++++------------------
> 1 file changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ee1c8a9..5f9701e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2418,7 +2418,8 @@ 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 (vcpu->arch.apic_base & X2APIC_ENABLE &&
> + kvm_vcpu_apicv_active(vcpu)) {
Need to check the virtualize_x2apic_mode is enabled or not.
> if (is_long_mode(vcpu))
> msr_bitmap = vmx_msr_bitmap_longmode_x2apic;
> else
> @@ -4783,6 +4784,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 +6343,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);
>
> if (enable_ept) {
> kvm_mmu_set_mask_ptes(0ull,
>
--
best regards
yang
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] kvm:vmx: more complete state update on APICv on/off
2016-05-18 5:24 ` Yang Zhang
@ 2016-05-18 8:37 ` Roman Kagan
0 siblings, 0 replies; 3+ messages in thread
From: Roman Kagan @ 2016-05-18 8:37 UTC (permalink / raw)
To: Yang Zhang; +Cc: kvm, Paolo Bonzini, Denis V. Lunev, Steve Rutherford
On Wed, May 18, 2016 at 01:24:50PM +0800, Yang Zhang wrote:
> On 2016/5/17 18:36, Roman Kagan wrote:
>
> During reviewing this patch, I think there is an secure issue in upstream
> KVM since the 5c919412fe61c:
> 1. The enable_apicv is setting to 1 if the underling hardware has APICv
> capability.
> 2. set the x2apic_read/write msr to disable the interception for most x2apic
> read and TPR/EOI/SELF-IPI write.
> 3. vmx_set_virtual_x2apic_mode is called when guest enables the x2apic,but
> kvm_vcpu_apicv_active() check may fail if synIc is activated.This means the
> virtualize_x2apic_mode will not set even guest enabling the x2apic.
> 4. In the following vmx_set_msr_bitmap, it will use the x2apic msr bitmap
> without check whether virtualize_x2apic_mode is enabled and apicv is
> enabled.
> 5. Then the access to the msr that doesn't intercept by hypervisor will
> access the hardware directly.
Looks like a valid concern, thanks.
> > @@ -2418,7 +2418,8 @@ 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 (vcpu->arch.apic_base & X2APIC_ENABLE &&
> > + kvm_vcpu_apicv_active(vcpu)) {
>
> Need to check the virtualize_x2apic_mode is enabled or not.
Indeed. Have lost it while porting over from our branch; will resubmit.
Thanks!
Roman.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-05-18 8:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-17 10:36 [PATCH v2] kvm:vmx: more complete state update on APICv on/off Roman Kagan
2016-05-18 5:24 ` Yang Zhang
2016-05-18 8:37 ` Roman Kagan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.