On Tue, Apr 21, 2020 at 7:02 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> On Tue, Apr 21, 2020 at 06:57:59PM -0700, Sean Christopherson wrote:
> > On Fri, Apr 17, 2020 at 11:34:52AM -0700, Makarand Sonare wrote:
> > > Don't clobber the VMX-preemption timer value in the VMCS12 during
> > > migration on the source while handling an L1 VMLAUNCH/VMRESUME but
> > > before L2 ran. In that case the VMCS12 preemption timer value
> > > should not be touched as it will be restarted on the target
> > > from its original value. This emulates migration occurring while L1
> > > awaits completion of its VMLAUNCH/VMRESUME instruction.
> > >
> > > Signed-off-by: Makarand Sonare <makarandsonare@xxxxxxxxxx>
> > > Signed-off-by: Peter Shier <pshier@xxxxxxxxxx>
> >
> > The SOB tags are reversed, i.e. Peter's should be first to show that he
> > wrote the patch and then transfered it to you for upstreaming.
> >
> > > Change-Id: I376d151585d4f1449319f7512151f11bbf08c5bf
> > > ---
> > > arch/x86/kvm/vmx/nested.c | 12 +++++++-----
> > > 1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > index 5365d7e5921ea..66155e9114114 100644
> > > --- a/arch/x86/kvm/vmx/nested.c
> > > +++ b/arch/x86/kvm/vmx/nested.c
> > > @@ -3897,11 +3897,13 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> > > vmcs12->guest_activity_state = GUEST_ACTIVITY_ACTIVE;
> > >
> > > if (nested_cpu_has_preemption_timer(vmcs12)) {
> > > - vmx->nested.preemption_timer_remaining =
> > > - vmx_get_preemption_timer_value(vcpu);
> > > - if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)
> > > - vmcs12->vmx_preemption_timer_value =
> > > - vmx->nested.preemption_timer_remaining;
> > > + if (!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)
> > > + vmcs12->vmx_preemption_timer_value =
> > > + vmx->nested.preemption_timer_remaining;
> > > + }
> >
> > This indentation is messed up, the closing brace is for !nested_run_pending,
> > but it's aligned with (vm_exit_controls & ..._PREEMPTION_TIMER).
> >
> > Even better than fixing the indentation would be to include !nested_run_pending
> > in the top-level if statement, which reduces the nesting level and produces
> > a much cleaner diff, e.g.
> >
> > 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.
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.
