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@xxxxxxxxxx>
>
> 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;
> + }
> + }
> +
