Re: [kvm PATCH 2/2] KVM: nVMX: Don't clobber preemption timer in the VMCS12 before L2 ran

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Wed, Apr 22, 2020 at 10:05:45AM -0700, Jim Mattson wrote:
> On Tue, Apr 21, 2020 at 7:02 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > index 409a39af121f..7dd6440425ab 100644
> > > --- a/arch/x86/kvm/vmx/nested.c
> > > +++ b/arch/x86/kvm/vmx/nested.c
> > > @@ -3951,7 +3951,8 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> > >         else
> > >                 vmcs12->guest_activity_state = GUEST_ACTIVITY_ACTIVE;
> > >
> > > -       if (nested_cpu_has_preemption_timer(vmcs12)) {
> > > +       if (nested_cpu_has_preemption_timer(vmcs12) &&
> > > +           !vmx->nested.nested_run_pending) {
> > >                 vmx->nested.preemption_timer_remaining =
> > >                         vmx_get_preemption_timer_value(vcpu);
> > >                 if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)
> >
> > Actually, why is this a separate patch?  The code it's fixing was introduced
> > in patch one of this series.
> 
> That's my fault. I questioned the legitimacy of this patch. When
> nested_run_pending is set in sync_vmcs02_to_vmcs12, we are in the
> middle of executing a VMLAUNCH or VMRESUME and userspace has requested
> a dump of the nested state. At this point, we have already called
> nested_vmx_enter_non_root_mode, and we have already started the timer.
> Even though we are going to repeat the vmcs02 setup when restoring the
> nested state, we do not actually rollback and restart the instruction.
> Setting up the vmcs02 on the target is just something we have to do to
> continue where we left off. Since we're continuing a partially
> executed instruction after the restore rather than rolling back, I
> think it's perfectly reasonable to go ahead and count the time elapsed
> prior to KVM_GET_NESTED_STATE against L2's VMX-preemption timer.

Counting the time lapsed seems reasonable, and is allowed from an
architectural standpoint.  It probably ends up being more accurate for the
KVM (as L1) use case where the preemption timer is being used to emulate
the TSC deadline timer, i.e. is less delayed.

> I don't have a strong objection to this patch. It just seems to add
> gratuitous complexity. If the consensus is to take it, the two parts
> should be squashed together.

I too don't have a strong opinion either way.