[PATCH] kvm:vmx: update secondary controls when disabling APICv

[PATCH] kvm:vmx: update secondary controls when disabling APICv

[Roman Kagan]

There are APICv-related fields among secondary processor-based
VM-execution controls, too.  Those used to be left at their initial
enabled state when APICv was disabled (upon activating Hyper-V SynIC).
This would result in Windows guests getting stuck when SynIC-based
auto-EOI interrupt intersected with e.g. an IPI in the guest.

So update the secondary execution controls too, if present, when
deactivating APICv while enabling Hyper-V SynIC.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 arch/x86/kvm/vmx.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ee1c8a9..7d98517 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4778,13 +4778,6 @@ static u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
 	return pin_based_exec_ctrl;
 }
 
-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));
-}
-
 static u32 vmx_exec_control(struct vcpu_vmx *vmx)
 {
 	u32 exec_control = vmcs_config.cpu_based_exec_ctrl;
@@ -4843,6 +4836,16 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
 	return exec_control;
 }
 
+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())
+		vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
+				vmx_secondary_exec_control(vmx));
+}
+
 static void ept_set_mmio_spte_mask(void)
 {
 	/*
-- 
2.5.5

Re: [PATCH] kvm:vmx: update secondary controls when disabling APICv

[Steve Rutherford]

After updating the secondary execution controls, the MSR bitmaps need
to be updated to enable APIC MSR intercepts (i.e. start treating APIC
MSRs as if the guest's APICs are back in xAPIC mode by calling
vmx_set_msr_bitmap, given that updating the execution controls should
disable "virtualize x2APIC mode").

The function vmx_set_msr_bitmap needs to look at the state of APICv to
know that "virtualize x2APIC mode" was disabled.

Also, calling vmx_secondary_exec_control clobbers /all/ dynamic bits
in the secondary controls (including the shadow VMCS bits). Those bits
needs to be respected (ideally this patch should just whack all of the
APICv related secondary execution controls).

I can put together a patch if you'd like.

On Mon, May 16, 2016 at 8:38 AM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> There are APICv-related fields among secondary processor-based
> VM-execution controls, too.  Those used to be left at their initial
> enabled state when APICv was disabled (upon activating Hyper-V SynIC).
> This would result in Windows guests getting stuck when SynIC-based
> auto-EOI interrupt intersected with e.g. an IPI in the guest.
>
> So update the secondary execution controls too, if present, when
> deactivating APICv while enabling Hyper-V SynIC.
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  arch/x86/kvm/vmx.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ee1c8a9..7d98517 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4778,13 +4778,6 @@ static u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
>         return pin_based_exec_ctrl;
>  }
>
> -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));
> -}
> -
>  static u32 vmx_exec_control(struct vcpu_vmx *vmx)
>  {
>         u32 exec_control = vmcs_config.cpu_based_exec_ctrl;
> @@ -4843,6 +4836,16 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>         return exec_control;
>  }
>
> +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())
> +               vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
> +                               vmx_secondary_exec_control(vmx));
> +}
> +
>  static void ept_set_mmio_spte_mask(void)
>  {
>         /*
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm:vmx: update secondary controls when disabling APICv

[Denis V. Lunev]

On 05/16/2016 11:15 PM, Steve Rutherford wrote:
> After updating the secondary execution controls, the MSR bitmaps need
> to be updated to enable APIC MSR intercepts (i.e. start treating APIC
> MSRs as if the guest's APICs are back in xAPIC mode by calling
> vmx_set_msr_bitmap, given that updating the execution controls should
> disable "virtualize x2APIC mode").
>
> The function vmx_set_msr_bitmap needs to look at the state of APICv to
> know that "virtualize x2APIC mode" was disabled.
>
> Also, calling vmx_secondary_exec_control clobbers /all/ dynamic bits
> in the secondary controls (including the shadow VMCS bits). Those bits
> needs to be respected (ideally this patch should just whack all of the
> APICv related secondary execution controls).
>
> I can put together a patch if you'd like.
for the perfect world I will agree. For a real life -Windows does not 
support x2Apic.
OK. We will think on this a bit.

May be they will in Win2016.

Re: [PATCH] kvm:vmx: update secondary controls when disabling APICv

[Roman Kagan]

On Mon, May 16, 2016 at 01:15:39PM -0700, Steve Rutherford wrote:
> After updating the secondary execution controls, the MSR bitmaps need
> to be updated to enable APIC MSR intercepts (i.e. start treating APIC
> MSRs as if the guest's APICs are back in xAPIC mode by calling
> vmx_set_msr_bitmap, given that updating the execution controls should
> disable "virtualize x2APIC mode").

Indeed.  I didn't notice that in my tests because Windows doesn't use
x2apic (they use their own apic access msrs).

> The function vmx_set_msr_bitmap needs to look at the state of APICv to
> know that "virtualize x2APIC mode" was disabled.

OK

> Also, calling vmx_secondary_exec_control clobbers /all/ dynamic bits
> in the secondary controls (including the shadow VMCS bits). Those bits
> needs to be respected (ideally this patch should just whack all of the
> APICv related secondary execution controls).

I probably misunderstood the intent of vmx_*exec_control functions.  OK
I'll clear the relevant bits in vmcs directly instead.  I wonder if I
should also do so for PIN_BASED_POSTED_INTR?

> I can put together a patch if you'd like.

I'll try myself but I won't mind if you beat me to it :)

Roman.