Re: [kvm PATCH] KVM: nVMX - enable VMX preemption timer migration

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

A few comments on the shortlog:

  - Use [PATCH] instead of [kvm PATCH], the "kvm" part is redundant with
    the subsystem scope.  I almost skipped over this patch because my eyes
    have been trained to treat [kvm... PATCH] as patches for kvm-unit-tests.

  - Use a colon instead of a dash after nVMX.

  - The patch should be tagged v2.  Again, I almost glossed over this
    because I thought it was a resend of v1.

  - I wouldn't phrase this as enabling, e.g. nothing prevented migrating
    the preemption timer before this patch, it just wasn't migrated
    correctly.

E.g.

  [PATCH v2] KVM: nVMX: Fix VMX preemption timer migration


On Wed, Apr 22, 2020 at 01:50:30PM -0700, Makarand Sonare wrote:
> From: Peter Shier <pshier@google.com>
> 
> Add new field to hold preemption timer remaining until expiration
> appended to struct kvm_vmx_nested_state_data. This is to prevent
> the second (and later) VM-Enter after migration from restarting the timer

This isn't correct.  The bug being fixed is that the _first_ VM-Enter after
migration would use the incorrect value, i.e. full value instead of
partially decayed timer.  My comment in v1 was that the changelog should
document why it adds a new field to fix the bug as opposed to simply
snapshotting the decayed timer.

> with wrong value. KVM_SET_NESTED_STATE restarts timer using migrated
> state regardless of whether L1 sets VM_EXIT_SAVE_VMX_PREEMPTION_TIMER.

Any plans to enhance the vmx_set_nested_state_test.c to verify this works
as intended?

> struct kvm_vmx_nested_state_hdr {
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index cbc9ea2de28f9..a5207df73f015 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2014,15 +2014,16 @@ static enum hrtimer_restart vmx_preemption_timer_fn(struct hrtimer *timer)
>  		container_of(timer, struct vcpu_vmx, nested.preemption_timer);
>  
>  	vmx->nested.preemption_timer_expired = true;
> +	vmx->nested.preemption_timer_remaining = 0;

Won't this get overwritten by sync_vmcs02_to_vmcs12()?  Unless there is a
corner cases I'm missing, I think it'd be better to omit this so that it's
clear that sync_vmcs02_to_vmcs12() is responsible for snapshotting the
remaining time.

>  	kvm_make_request(KVM_REQ_EVENT, &vmx->vcpu);
>  	kvm_vcpu_kick(&vmx->vcpu);
>  
>  	return HRTIMER_NORESTART;
>  }

...

> @@ -5790,6 +5809,9 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
>  
>  	BUILD_BUG_ON(sizeof(user_vmx_nested_state->vmcs12) < VMCS12_SIZE);
>  	BUILD_BUG_ON(sizeof(user_vmx_nested_state->shadow_vmcs12) < VMCS12_SIZE);
> +	BUILD_BUG_ON(sizeof(user_vmx_nested_state->preemption_timer_remaining)
> +		    != sizeof(vmx->nested.preemption_timer_remaining));
> +
>  
>  	/*
>  	 * Copy over the full allocated size of vmcs12 rather than just the size
> @@ -5805,6 +5827,13 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
>  			return -EFAULT;
>  	}
>  
> +	if (kvm_state.flags & KVM_STATE_NESTED_PREEMPTION_TIMER) {
> +		if (copy_to_user(&user_vmx_nested_state->preemption_timer_remaining,
> +				 &vmx->nested.preemption_timer_remaining,
> +				 sizeof(vmx->nested.preemption_timer_remaining)))a

Build tested only, but {get,put}_user() compiles just fine, as requested.

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index a2cd413f2901..b2ec62a24f0c 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5970,9 +5970,6 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,

        BUILD_BUG_ON(sizeof(user_vmx_nested_state->vmcs12) < VMCS12_SIZE);
        BUILD_BUG_ON(sizeof(user_vmx_nested_state->shadow_vmcs12) < VMCS12_SIZE);
-       BUILD_BUG_ON(sizeof(user_vmx_nested_state->preemption_timer_remaining)
-                   != sizeof(vmx->nested.preemption_timer_remaining));
-

        /*
         * Copy over the full allocated size of vmcs12 rather than just the size
@@ -5989,9 +5986,8 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
        }

        if (kvm_state.flags & KVM_STATE_NESTED_PREEMPTION_TIMER) {
-               if (copy_to_user(&user_vmx_nested_state->preemption_timer_remaining,
-                                &vmx->nested.preemption_timer_remaining,
-                                sizeof(vmx->nested.preemption_timer_remaining)))
+               if (put_user(vmx->nested.preemption_timer_remaining,
+                            &user_vmx_nested_state->preemption_timer_remaining))
                        return -EFAULT;
        }

@@ -6164,9 +6160,8 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
                    offsetofend(struct  kvm_vmx_nested_state_data, preemption_timer_remaining))
                        goto error_guest_mode;

-               if (copy_from_user(&vmx->nested.preemption_timer_remaining,
-                                  &user_vmx_nested_state->preemption_timer_remaining,
-                                  sizeof(user_vmx_nested_state->preemption_timer_remaining))) {
+               if (get_user(vmx->nested.preemption_timer_remaining,
+                            &user_vmx_nested_state->preemption_timer_remaining)) {
                        ret = -EFAULT;
                        goto error_guest_mode;
                }

> +			return -EFAULT;
> +	}
> +
>  out:
>  	return kvm_state.size;
>  }
> @@ -5876,7 +5905,8 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>  	 */
>  	if (is_smm(vcpu) ?
>  		(kvm_state->flags &
> -		 (KVM_STATE_NESTED_GUEST_MODE | KVM_STATE_NESTED_RUN_PENDING))
> +		 (KVM_STATE_NESTED_GUEST_MODE | KVM_STATE_NESTED_RUN_PENDING |
> +		  KVM_STATE_NESTED_PREEMPTION_TIMER))
>  		: kvm_state->hdr.vmx.smm.flags)
>  		return -EINVAL;
>  
> @@ -5966,6 +5996,21 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>  			goto error_guest_mode;
>  	}
>  
> +	if (kvm_state->flags & KVM_STATE_NESTED_PREEMPTION_TIMER) {
> +
> +		if (kvm_state->size <
> +		    offsetof(struct  kvm_nested_state, hdr.vmx) +
> +		    offsetofend(struct  kvm_vmx_nested_state_data, preemption_timer_remaining))

Too many spaces after 'struct'.

> +			goto error_guest_mode;
> +
> +		if (copy_from_user(&vmx->nested.preemption_timer_remaining,
> +				   &user_vmx_nested_state->preemption_timer_remaining,
> +				   sizeof(user_vmx_nested_state->preemption_timer_remaining))) {
> +			ret = -EFAULT;
> +			goto error_guest_mode;
> +		}
> +	}
> +