[PATCH v4 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

[PATCH v4 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<          (null)>]           (null)
PGD 0
Oops: 0010 [#1] SMP
Call Trace:
 ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
 handle_preemption_timer+0xe/0x20 [kvm_intel]
 vmx_handle_exit+0x169/0x15a0 [kvm_intel]
 ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
 kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
 ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
 ? vcpu_load+0x1c/0x60 [kvm]
 ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
 kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
 do_vfs_ioctl+0x96/0x6a0
 ? __fget_light+0x2a/0x90
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x68/0x180
 entry_SYSCALL64_slow_path+0x25/0x25
Code:  Bad RIP value.
RIP  [<          (null)>]           (null)
 RSP <ffff8800b5263c48>
CR2: 0000000000000000
---[ end trace 9c70c48b1a2bc66e ]---

This can be reproduced readily by preemption timer enabled on L0 and disabled
on L1.

Preemption timer for nested VMX is emulated by hrtimer which is started on L2
entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
nested_vmx_exit_handled is always return true for preemption timer vmexit, then
the L1 preemption timer vmexit is captured and be treated as a L2 preemption
timer vmexit, incurr a nested vmexit dereference NULL pointer.

This patch fix it by depending on check_nested_events to capture L2 preemption
timer(emulated hrtimer) expire and nested vmexit.

Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v2 -> v3:
 * update patch subject
v1 -> v2:
 * fix typo in patch description

 arch/x86/kvm/vmx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 85e2f0a..29c16a8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
 	case EXIT_REASON_PCOMMIT:
 		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
+	case EXIT_REASON_PREEMPTION_TIMER:
+		return false;
 	default:
 		return true;
 	}
-- 
1.9.1

[PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

We will go to vcpu_run() loop after L0 emulates VMRESUME which maybe 
incur kvm_sched_out and kvm_sched_in operations since cond_resched() 
will be called once need resched. Preemption timer will be reprogrammed 
if vCPU is scheduled to a different pCPU. Then the preemption timer 
bit of vmcs02 will be set if L0 enable preemption timer to run L1 even 
if L1 doesn't enable preemption timer to run L2.

This patch fix it by don't reprogram preemption timer of vmcs02 if L1's 
vCPU is scheduled on diffent pCPU when we are in the way to vmresume 
nested guest, and fallback to hrtimer based emulated method.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v3 -> v4:
 * fallback to hrtimer based emulated method when in the way to vmresume nested guest 

 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0cc6cf8..05137c0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2743,8 +2743,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 			mark_tsc_unstable("KVM discovered backwards TSC");
 
 		if (kvm_lapic_hv_timer_in_use(vcpu) &&
+			(is_guest_mode(vcpu) ||
 				kvm_x86_ops->set_hv_timer(vcpu,
-					kvm_get_lapic_tscdeadline_msr(vcpu)))
+					kvm_get_lapic_tscdeadline_msr(vcpu))))
 			kvm_lapic_switch_to_sw_timer(vcpu);
 		if (check_tsc_unstable()) {
 			u64 offset = kvm_compute_tsc_offset(vcpu,
-- 
1.9.1

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Paolo Bonzini]

On 07/07/2016 14:18, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> We will go to vcpu_run() loop after L0 emulates VMRESUME which maybe 
> incur kvm_sched_out and kvm_sched_in operations since cond_resched() 
> will be called once need resched. Preemption timer will be reprogrammed 
> if vCPU is scheduled to a different pCPU. Then the preemption timer 
> bit of vmcs02 will be set if L0 enable preemption timer to run L1 even 
> if L1 doesn't enable preemption timer to run L2.
> 
> This patch fix it by don't reprogram preemption timer of vmcs02 if L1's 
> vCPU is scheduled on diffent pCPU when we are in the way to vmresume 
> nested guest, and fallback to hrtimer based emulated method.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v3 -> v4:
>  * fallback to hrtimer based emulated method when in the way to vmresume nested guest 
> 
>  arch/x86/kvm/x86.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0cc6cf8..05137c0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2743,8 +2743,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  			mark_tsc_unstable("KVM discovered backwards TSC");
>  
>  		if (kvm_lapic_hv_timer_in_use(vcpu) &&
> +			(is_guest_mode(vcpu) ||
>  				kvm_x86_ops->set_hv_timer(vcpu,
> -					kvm_get_lapic_tscdeadline_msr(vcpu)))
> +					kvm_get_lapic_tscdeadline_msr(vcpu))))
>  			kvm_lapic_switch_to_sw_timer(vcpu);
>  		if (check_tsc_unstable()) {
>  			u64 offset = kvm_compute_tsc_offset(vcpu,
> 

Thanks, this is good as a fallback.  I'll try to fix it by getting the
pin-based execution controls right but if I fail this patch is okay.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH v4 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

[Paolo Bonzini]

On 07/07/2016 14:18, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> BUG: unable to handle kernel NULL pointer dereference at           (null)
> IP: [<          (null)>]           (null)
> PGD 0
> Oops: 0010 [#1] SMP
> Call Trace:
>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
>  handle_preemption_timer+0xe/0x20 [kvm_intel]
>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>  ? vcpu_load+0x1c/0x60 [kvm]
>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
>  do_vfs_ioctl+0x96/0x6a0
>  ? __fget_light+0x2a/0x90
>  SyS_ioctl+0x79/0x90
>  do_syscall_64+0x68/0x180
>  entry_SYSCALL64_slow_path+0x25/0x25
> Code:  Bad RIP value.
> RIP  [<          (null)>]           (null)
>  RSP <ffff8800b5263c48>
> CR2: 0000000000000000
> ---[ end trace 9c70c48b1a2bc66e ]---
> 
> This can be reproduced readily by preemption timer enabled on L0 and disabled
> on L1.
> 
> Preemption timer for nested VMX is emulated by hrtimer which is started on L2
> entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
> nested_vmx_exit_handled is always return true for preemption timer vmexit, then
> the L1 preemption timer vmexit is captured and be treated as a L2 preemption
> timer vmexit, incurr a nested vmexit dereference NULL pointer.
> 
> This patch fix it by depending on check_nested_events to capture L2 preemption
> timer(emulated hrtimer) expire and nested vmexit.
> 
> Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v2 -> v3:
>  * update patch subject
> v1 -> v2:
>  * fix typo in patch description
> 
>  arch/x86/kvm/vmx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 85e2f0a..29c16a8 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
>  	case EXIT_REASON_PCOMMIT:
>  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
> +	case EXIT_REASON_PREEMPTION_TIMER:
> +		return false;
>  	default:
>  		return true;
>  	}
> 

Thanks, applied to kvm/queue.

Paolo

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Wanpeng Li]

2016-07-07 20:29 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 07/07/2016 14:18, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> We will go to vcpu_run() loop after L0 emulates VMRESUME which maybe
>> incur kvm_sched_out and kvm_sched_in operations since cond_resched()
>> will be called once need resched. Preemption timer will be reprogrammed
>> if vCPU is scheduled to a different pCPU. Then the preemption timer
>> bit of vmcs02 will be set if L0 enable preemption timer to run L1 even
>> if L1 doesn't enable preemption timer to run L2.
>>
>> This patch fix it by don't reprogram preemption timer of vmcs02 if L1's
>> vCPU is scheduled on diffent pCPU when we are in the way to vmresume
>> nested guest, and fallback to hrtimer based emulated method.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>> v3 -> v4:
>>  * fallback to hrtimer based emulated method when in the way to vmresume nested guest
>>
>>  arch/x86/kvm/x86.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 0cc6cf8..05137c0 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2743,8 +2743,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>                       mark_tsc_unstable("KVM discovered backwards TSC");
>>
>>               if (kvm_lapic_hv_timer_in_use(vcpu) &&
>> +                     (is_guest_mode(vcpu) ||
>>                               kvm_x86_ops->set_hv_timer(vcpu,
>> -                                     kvm_get_lapic_tscdeadline_msr(vcpu)))
>> +                                     kvm_get_lapic_tscdeadline_msr(vcpu))))
>>                       kvm_lapic_switch_to_sw_timer(vcpu);
>>               if (check_tsc_unstable()) {
>>                       u64 offset = kvm_compute_tsc_offset(vcpu,
>>
>
> Thanks, this is good as a fallback.  I'll try to fix it by getting the
> pin-based execution controls right but if I fail this patch is okay.

I believe we still need this patch even if you implement "L1 TSC
deadline timer to trigger while L2 is running" eventually, the codes
you posted before:

  exec_control = vmcs12->pin_based_vm_exec_control;
+exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
  exec_control |= vmcs_config.pin_based_exec_ctrl;
- exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
+ if (vmx->hv_deadline_tsc == -1)
+     exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;

So there is still case the preemption timer bit of vmcs02 is not set,
however,  the scenario I mentioned above in kvm_arch_vcpu_load() will
set it unnecessary.

Regards,
Wanpeng Li

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Paolo Bonzini]

On 07/07/2016 15:23, Wanpeng Li wrote:
>>> >>
>>> >>               if (kvm_lapic_hv_timer_in_use(vcpu) &&
>>> >> +                     (is_guest_mode(vcpu) ||
>>> >>                               kvm_x86_ops->set_hv_timer(vcpu,
>>> >> -                                     kvm_get_lapic_tscdeadline_msr(vcpu)))
>>> >> +                                     kvm_get_lapic_tscdeadline_msr(vcpu))))
>>> >>                       kvm_lapic_switch_to_sw_timer(vcpu);
>>> >>               if (check_tsc_unstable()) {
>>> >>                       u64 offset = kvm_compute_tsc_offset(vcpu,
>>> >>
>> >
>> > Thanks, this is good as a fallback.  I'll try to fix it by getting the
>> > pin-based execution controls right but if I fail this patch is okay.
> I believe we still need this patch even if you implement "L1 TSC
> deadline timer to trigger while L2 is running" eventually, the codes
> you posted before:
> 
>   exec_control = vmcs12->pin_based_vm_exec_control;
> +exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>   exec_control |= vmcs_config.pin_based_exec_ctrl;
> - exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
> + if (vmx->hv_deadline_tsc == -1)
> +     exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
> 
> So there is still case the preemption timer bit of vmcs02 is not set,
> however,  the scenario I mentioned above in kvm_arch_vcpu_load() will
> set it unnecessary.

kvm_x86_ops->set_hv_timer _will_ set the preemption timer bit of vmcs02
if vmcs02 is the loaded one.

This can happen if L2 has access to L1's local APIC registers (i.e. L1
passes the local APIC instead of emulating it, as is the case in a
partitioning hypervisor).  While L2 runs, it writes to the TSC deadline
MSR of L1.  This causes a call to kvm_x86_ops->set_hv_timer while the
active VMCS is a vmcs02.

Paolo

Re: [PATCH v4 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

[yunhong jiang]

On Thu, 7 Jul 2016 14:29:45 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 07/07/2016 14:18, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpeng.li@hotmail.com>
> > 
> > BUG: unable to handle kernel NULL pointer dereference at
> > (null) IP: [<          (null)>]           (null)
> > PGD 0
> > Oops: 0010 [#1] SMP
> > Call Trace:
> >  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
> >  handle_preemption_timer+0xe/0x20 [kvm_intel]
> >  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
> >  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
> >  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
> >  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
> >  ? vcpu_load+0x1c/0x60 [kvm]
> >  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
> >  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
> >  do_vfs_ioctl+0x96/0x6a0
> >  ? __fget_light+0x2a/0x90
> >  SyS_ioctl+0x79/0x90
> >  do_syscall_64+0x68/0x180
> >  entry_SYSCALL64_slow_path+0x25/0x25
> > Code:  Bad RIP value.
> > RIP  [<          (null)>]           (null)
> >  RSP <ffff8800b5263c48>
> > CR2: 0000000000000000
> > ---[ end trace 9c70c48b1a2bc66e ]---
> > 
> > This can be reproduced readily by preemption timer enabled on L0
> > and disabled on L1.
> > 
> > Preemption timer for nested VMX is emulated by hrtimer which is
> > started on L2 entry, stopped on L2 exit and evaluated via the
> > check_nested_events hook. However, nested_vmx_exit_handled is
> > always return true for preemption timer vmexit, then the L1
> > preemption timer vmexit is captured and be treated as a L2
> > preemption timer vmexit, incurr a nested vmexit dereference NULL
> > pointer.
> > 
> > This patch fix it by depending on check_nested_events to capture L2
> > preemption timer(emulated hrtimer) expire and nested vmexit.
> > 
> > Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> > Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > Cc: Haozhong Zhang <haozhong.zhang@intel.com>
> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> > ---
> > v2 -> v3:
> >  * update patch subject
> > v1 -> v2:
> >  * fix typo in patch description
> > 
> >  arch/x86/kvm/vmx.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 85e2f0a..29c16a8 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct
> > kvm_vcpu *vcpu) return nested_cpu_has2(vmcs12,
> > SECONDARY_EXEC_XSAVES); case EXIT_REASON_PCOMMIT:
> >  		return nested_cpu_has2(vmcs12,
> > SECONDARY_EXEC_PCOMMIT);
> > +	case EXIT_REASON_PREEMPTION_TIMER:
> > +		return false;
> >  	default:
> >  		return true;
> >  	}
> > 
> 
> Thanks, applied to kvm/queue.
> 
> Paolo

Just back from vocation and see this issue. Really sorry that I didn't consider
the nested VM situation when working on the original patch.

Paolo/Wanpeng/Haozhong, thanks for the patch.

--jyh

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Wanpeng Li]

2016-07-07 22:11 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 07/07/2016 15:23, Wanpeng Li wrote:
>>>> >>
>>>> >>               if (kvm_lapic_hv_timer_in_use(vcpu) &&
>>>> >> +                     (is_guest_mode(vcpu) ||
>>>> >>                               kvm_x86_ops->set_hv_timer(vcpu,
>>>> >> -                                     kvm_get_lapic_tscdeadline_msr(vcpu)))
>>>> >> +                                     kvm_get_lapic_tscdeadline_msr(vcpu))))
>>>> >>                       kvm_lapic_switch_to_sw_timer(vcpu);
>>>> >>               if (check_tsc_unstable()) {
>>>> >>                       u64 offset = kvm_compute_tsc_offset(vcpu,
>>>> >>
>>> >
>>> > Thanks, this is good as a fallback.  I'll try to fix it by getting the
>>> > pin-based execution controls right but if I fail this patch is okay.
>> I believe we still need this patch even if you implement "L1 TSC
>> deadline timer to trigger while L2 is running" eventually, the codes
>> you posted before:
>>
>>   exec_control = vmcs12->pin_based_vm_exec_control;
>> +exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>   exec_control |= vmcs_config.pin_based_exec_ctrl;
>> - exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>> + if (vmx->hv_deadline_tsc == -1)
>> +     exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>
>> So there is still case the preemption timer bit of vmcs02 is not set,
>> however,  the scenario I mentioned above in kvm_arch_vcpu_load() will
>> set it unnecessary.
>
> kvm_x86_ops->set_hv_timer _will_ set the preemption timer bit of vmcs02
> if vmcs02 is the loaded one.
>
> This can happen if L2 has access to L1's local APIC registers (i.e. L1
> passes the local APIC instead of emulating it, as is the case in a
> partitioning hypervisor).  While L2 runs, it writes to the TSC deadline
> MSR of L1.  This causes a call to kvm_x86_ops->set_hv_timer while the
> active VMCS is a vmcs02.

Yes, in the scenario you pointed out the call to
kvm_x86_ops->set_hv_timer while the active VMCS is vmcs02 is correct,
however, in the scenario I mentioned in the patch description is not
correct even if enable "L1 TSC deadline timer to trigger while L2 is
running".

Regards,
Wanpeng Li

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Paolo Bonzini]

On 08/07/2016 02:38, Wanpeng Li wrote:
> 2016-07-07 22:11 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>
>>
>> On 07/07/2016 15:23, Wanpeng Li wrote:
>>>>>>>
>>>>>>>               if (kvm_lapic_hv_timer_in_use(vcpu) &&
>>>>>>> +                     (is_guest_mode(vcpu) ||
>>>>>>>                               kvm_x86_ops->set_hv_timer(vcpu,
>>>>>>> -                                     kvm_get_lapic_tscdeadline_msr(vcpu)))
>>>>>>> +                                     kvm_get_lapic_tscdeadline_msr(vcpu))))
>>>>>>>                       kvm_lapic_switch_to_sw_timer(vcpu);
>>>>>>>               if (check_tsc_unstable()) {
>>>>>>>                       u64 offset = kvm_compute_tsc_offset(vcpu,
>>>>>>>
>>>>>
>>>>> Thanks, this is good as a fallback.  I'll try to fix it by getting the
>>>>> pin-based execution controls right but if I fail this patch is okay.
>>> I believe we still need this patch even if you implement "L1 TSC
>>> deadline timer to trigger while L2 is running" eventually, the codes
>>> you posted before:
>>>
>>>   exec_control = vmcs12->pin_based_vm_exec_control;
>>> +exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>>   exec_control |= vmcs_config.pin_based_exec_ctrl;
>>> - exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>> + if (vmx->hv_deadline_tsc == -1)
>>> +     exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>>
>>> So there is still case the preemption timer bit of vmcs02 is not set,
>>> however,  the scenario I mentioned above in kvm_arch_vcpu_load() will
>>> set it unnecessary.
>>
>> kvm_x86_ops->set_hv_timer _will_ set the preemption timer bit of vmcs02
>> if vmcs02 is the loaded one.
>>
>> This can happen if L2 has access to L1's local APIC registers (i.e. L1
>> passes the local APIC instead of emulating it, as is the case in a
>> partitioning hypervisor).  While L2 runs, it writes to the TSC deadline
>> MSR of L1.  This causes a call to kvm_x86_ops->set_hv_timer while the
>> active VMCS is a vmcs02.
> 
> Yes, in the scenario you pointed out the call to
> kvm_x86_ops->set_hv_timer while the active VMCS is vmcs02 is correct,
> however, in the scenario I mentioned in the patch description is not
> correct even if enable "L1 TSC deadline timer to trigger while L2 is
> running".

It doesn't help that you have not explained how to reproduce the
bug---this is what the cover letter and commit messages are for, too.

Your patch 1 is enough for me to boot L2 Windows 2008 inside L1 KVM 4.1.
 So I have an updated patch to handle the TSC deadline timer while L2 is
running, but I have no idea how to test its correctness.  I'll send the
patch shortly.

Paolo

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Wanpeng Li]

2016-07-08 18:18 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 08/07/2016 02:38, Wanpeng Li wrote:
>> 2016-07-07 22:11 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>
>>>
>>> On 07/07/2016 15:23, Wanpeng Li wrote:
>>>>>>>>
>>>>>>>>               if (kvm_lapic_hv_timer_in_use(vcpu) &&
>>>>>>>> +                     (is_guest_mode(vcpu) ||
>>>>>>>>                               kvm_x86_ops->set_hv_timer(vcpu,
>>>>>>>> -                                     kvm_get_lapic_tscdeadline_msr(vcpu)))
>>>>>>>> +                                     kvm_get_lapic_tscdeadline_msr(vcpu))))
>>>>>>>>                       kvm_lapic_switch_to_sw_timer(vcpu);
>>>>>>>>               if (check_tsc_unstable()) {
>>>>>>>>                       u64 offset = kvm_compute_tsc_offset(vcpu,
>>>>>>>>
>>>>>>
>>>>>> Thanks, this is good as a fallback.  I'll try to fix it by getting the
>>>>>> pin-based execution controls right but if I fail this patch is okay.
>>>> I believe we still need this patch even if you implement "L1 TSC
>>>> deadline timer to trigger while L2 is running" eventually, the codes
>>>> you posted before:
>>>>
>>>>   exec_control = vmcs12->pin_based_vm_exec_control;
>>>> +exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>>>   exec_control |= vmcs_config.pin_based_exec_ctrl;
>>>> - exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>>> + if (vmx->hv_deadline_tsc == -1)
>>>> +     exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>>>
>>>> So there is still case the preemption timer bit of vmcs02 is not set,
>>>> however,  the scenario I mentioned above in kvm_arch_vcpu_load() will
>>>> set it unnecessary.
>>>
>>> kvm_x86_ops->set_hv_timer _will_ set the preemption timer bit of vmcs02
>>> if vmcs02 is the loaded one.
>>>
>>> This can happen if L2 has access to L1's local APIC registers (i.e. L1
>>> passes the local APIC instead of emulating it, as is the case in a
>>> partitioning hypervisor).  While L2 runs, it writes to the TSC deadline
>>> MSR of L1.  This causes a call to kvm_x86_ops->set_hv_timer while the
>>> active VMCS is a vmcs02.
>>
>> Yes, in the scenario you pointed out the call to
>> kvm_x86_ops->set_hv_timer while the active VMCS is vmcs02 is correct,
>> however, in the scenario I mentioned in the patch description is not
>> correct even if enable "L1 TSC deadline timer to trigger while L2 is
>> running".
>
> It doesn't help that you have not explained how to reproduce the
> bug---this is what the cover letter and commit messages are for, too.

I believe you pointed out this before:

| The patch looks correct, but I'm not sure how you get a preemption
| timer vmexit while vmcs02 is active:
|
| exec_control = vmcs12->pin_based_vm_exec_control;
| exec_control |= vmcs_config.pin_based_exec_ctrl;
| exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;

We can't get preemption timer vmexit which vmcs02 is loaded since
preemtion timer bit in vmcs02 is not set, then how can we get the
incorrect preemption timer vmexit in nested guest which patch 1 fixed?
Because the scenario I mentioned in patch 2 set vmcs02.

w/o patch1 + w/o enable "L1 TSC deadline timer to trigger while L2 is
running"  => we will get the bug calltrace mentioned in patch1 since
incorrect vmcs02 bit is set due to the bug mentioned in patch 2. So
apply patch2 can fix it.

However, after enable "L1 TSC deadline timer to trigger while L2 is
running", we should have patch 1 to correctly capture nested vmexit
for preemption timer.

My setup is L0 and L1 both are latest kvm queue branch w/ Yunhong's
preemption timer enable patches and my previous "fix missed
cancellation of TSC deadline timer" patches. I always enable
preemption timer in L0, but try to enable or disable preemption timer
in L1.

Regards,
Wanpeng Li

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Wanpeng Li]

2016-07-08 21:58 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2016-07-08 18:18 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>
>>
>> On 08/07/2016 02:38, Wanpeng Li wrote:
>>> 2016-07-07 22:11 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>>
>>>>
>>>> On 07/07/2016 15:23, Wanpeng Li wrote:
>>>>>>>>>
>>>>>>>>>               if (kvm_lapic_hv_timer_in_use(vcpu) &&
>>>>>>>>> +                     (is_guest_mode(vcpu) ||
>>>>>>>>>                               kvm_x86_ops->set_hv_timer(vcpu,
>>>>>>>>> -                                     kvm_get_lapic_tscdeadline_msr(vcpu)))
>>>>>>>>> +                                     kvm_get_lapic_tscdeadline_msr(vcpu))))
>>>>>>>>>                       kvm_lapic_switch_to_sw_timer(vcpu);
>>>>>>>>>               if (check_tsc_unstable()) {
>>>>>>>>>                       u64 offset = kvm_compute_tsc_offset(vcpu,
>>>>>>>>>
>>>>>>>
>>>>>>> Thanks, this is good as a fallback.  I'll try to fix it by getting the
>>>>>>> pin-based execution controls right but if I fail this patch is okay.
>>>>> I believe we still need this patch even if you implement "L1 TSC
>>>>> deadline timer to trigger while L2 is running" eventually, the codes
>>>>> you posted before:
>>>>>
>>>>>   exec_control = vmcs12->pin_based_vm_exec_control;
>>>>> +exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>>>>   exec_control |= vmcs_config.pin_based_exec_ctrl;
>>>>> - exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>>>> + if (vmx->hv_deadline_tsc == -1)
>>>>> +     exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>>>>
>>>>> So there is still case the preemption timer bit of vmcs02 is not set,
>>>>> however,  the scenario I mentioned above in kvm_arch_vcpu_load() will
>>>>> set it unnecessary.
>>>>
>>>> kvm_x86_ops->set_hv_timer _will_ set the preemption timer bit of vmcs02
>>>> if vmcs02 is the loaded one.
>>>>
>>>> This can happen if L2 has access to L1's local APIC registers (i.e. L1
>>>> passes the local APIC instead of emulating it, as is the case in a
>>>> partitioning hypervisor).  While L2 runs, it writes to the TSC deadline
>>>> MSR of L1.  This causes a call to kvm_x86_ops->set_hv_timer while the
>>>> active VMCS is a vmcs02.
>>>
>>> Yes, in the scenario you pointed out the call to
>>> kvm_x86_ops->set_hv_timer while the active VMCS is vmcs02 is correct,
>>> however, in the scenario I mentioned in the patch description is not
>>> correct even if enable "L1 TSC deadline timer to trigger while L2 is
>>> running".
>>
>> It doesn't help that you have not explained how to reproduce the
>> bug---this is what the cover letter and commit messages are for, too.
>
> I believe you pointed out this before:
>
> | The patch looks correct, but I'm not sure how you get a preemption
> | timer vmexit while vmcs02 is active:
> |
> | exec_control = vmcs12->pin_based_vm_exec_control;
> | exec_control |= vmcs_config.pin_based_exec_ctrl;
> | exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>
> We can't get preemption timer vmexit which vmcs02 is loaded since
> preemtion timer bit in vmcs02 is not set, then how can we get the
> incorrect preemption timer vmexit in nested guest which patch 1 fixed?
> Because the scenario I mentioned in patch 2 set vmcs02.
>
> w/o patch1 + w/o enable "L1 TSC deadline timer to trigger while L2 is
> running"  => we will get the bug calltrace mentioned in patch1 since
> incorrect vmcs02 bit is set due to the bug mentioned in patch 2. So
> apply patch2 can fix it.
>
> However, after enable "L1 TSC deadline timer to trigger while L2 is
> running", we should have patch 1 to correctly capture nested vmexit
> for preemption timer.
>
> My setup is L0 and L1 both are latest kvm queue branch w/ Yunhong's
> preemption timer enable patches and my previous "fix missed
> cancellation of TSC deadline timer" patches. I always enable
> preemption timer in L0, but try to enable or disable preemption timer
> in L1.

Btw, my L1 is a full dynticks guest in order that hrtimer in L1 will
be heavily used.

Regards,
Wanpeng Li

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Paolo Bonzini]

On 08/07/2016 16:08, Wanpeng Li wrote:
> 2016-07-08 21:58 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>> We can't get preemption timer vmexit which vmcs02 is loaded since
>> preemtion timer bit in vmcs02 is not set, then how can we get the
>> incorrect preemption timer vmexit in nested guest which patch 1 fixed?
>> Because the scenario I mentioned in patch 2 set vmcs02.
>>
>> w/o patch1 + w/o enable "L1 TSC deadline timer to trigger while L2 is
>> running"  => we will get the bug calltrace mentioned in patch1 since
>> incorrect vmcs02 bit is set due to the bug mentioned in patch 2. So
>> apply patch2 can fix it.
>>
>> However, after enable "L1 TSC deadline timer to trigger while L2 is
>> running", we should have patch 1 to correctly capture nested vmexit
>> for preemption timer.
>>
>> My setup is L0 and L1 both are latest kvm queue branch w/ Yunhong's
>> preemption timer enable patches and my previous "fix missed
>> cancellation of TSC deadline timer" patches. I always enable
>> preemption timer in L0, but try to enable or disable preemption timer
>> in L1.
> 
> Btw, my L1 is a full dynticks guest in order that hrtimer in L1 will
> be heavily used.

Thanks---I'd be grateful if you tested my patch series.

Paolo

Re: [PATCH v4 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

[Wanpeng Li]

2016-07-08 23:47 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 08/07/2016 16:08, Wanpeng Li wrote:
>> 2016-07-08 21:58 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>>> We can't get preemption timer vmexit which vmcs02 is loaded since
>>> preemtion timer bit in vmcs02 is not set, then how can we get the
>>> incorrect preemption timer vmexit in nested guest which patch 1 fixed?
>>> Because the scenario I mentioned in patch 2 set vmcs02.
>>>
>>> w/o patch1 + w/o enable "L1 TSC deadline timer to trigger while L2 is
>>> running"  => we will get the bug calltrace mentioned in patch1 since
>>> incorrect vmcs02 bit is set due to the bug mentioned in patch 2. So
>>> apply patch2 can fix it.
>>>
>>> However, after enable "L1 TSC deadline timer to trigger while L2 is
>>> running", we should have patch 1 to correctly capture nested vmexit
>>> for preemption timer.
>>>
>>> My setup is L0 and L1 both are latest kvm queue branch w/ Yunhong's
>>> preemption timer enable patches and my previous "fix missed
>>> cancellation of TSC deadline timer" patches. I always enable
>>> preemption timer in L0, but try to enable or disable preemption timer
>>> in L1.
>>
>> Btw, my L1 is a full dynticks guest in order that hrtimer in L1 will
>> be heavily used.
>
> Thanks---I'd be grateful if you tested my patch series.

I will do it and also reconfirm my patch 2 next monday.

Regards,
Wapeng Li