| Message ID | 20210107093854.882483-3-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: few random fixes | expand |
On 07/01/21 10:38, Maxim Levitsky wrote: > The code to store it on the migration exists, but no code was restoring it. > > One of the side effects of fixing this is that L1->L2 injected events > are no longer lost when migration happens with nested run pending. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/x86/kvm/svm/nested.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index ee4f2082ad1bd..cc3130ab612e5 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -1200,6 +1200,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, > * in the registers, the save area of the nested state instead > * contains saved L1 state. > */ > + > + svm->nested.nested_run_pending = > + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > + > copy_vmcb_control_area(&hsave->control, &svm->vmcb->control); > hsave->save = *save; > > Nice fix and we need to do it anyway. That said, the v1 change had some appeal to it. In the VMX case (if properly implemented) it would allow removing the weird nested_run_pending case from prepare_vmcs02_early. I think it's a valuable invariant that there are no events in the VMCS after each KVM_RUN iteration, and this special case is breaking the invariant. Paolo
On Thu, Jan 07, 2021, Paolo Bonzini wrote: > On 07/01/21 10:38, Maxim Levitsky wrote: > > The code to store it on the migration exists, but no code was restoring it. > > > > One of the side effects of fixing this is that L1->L2 injected events > > are no longer lost when migration happens with nested run pending. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > arch/x86/kvm/svm/nested.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > > index ee4f2082ad1bd..cc3130ab612e5 100644 > > --- a/arch/x86/kvm/svm/nested.c > > +++ b/arch/x86/kvm/svm/nested.c > > @@ -1200,6 +1200,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, > > * in the registers, the save area of the nested state instead > > * contains saved L1 state. > > */ > > + > > + svm->nested.nested_run_pending = > > + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > > + > > copy_vmcb_control_area(&hsave->control, &svm->vmcb->control); > > hsave->save = *save; > > > > Nice fix and we need to do it anyway. > > That said, the v1 change had some appeal to it. Which v1 change are you referring to? > In the VMX case (if properly implemented) it would allow removing the weird > nested_run_pending case from prepare_vmcs02_early. I think it's a valuable > invariant that there are no events in the VMCS after each KVM_RUN iteration, > and this special case is breaking the invariant. Hmm, as weird as that code is, I think it's actually the most architecturally correct behavior. Technically, the clearing of VM_ENTRY_INTR_INFO.VALID shouldn't be visible in vmcs12 until a nested VM-Exit occurs, e.g. copying the vmcs02 value to vmcs12 in vmx_get_nested_state() would work, but it's wrong at the same time. Ditto for L1 (or L2) writing vmcs12.VM_ENTRY_INTR_INFO while L2 is running (ignoring the SDM's very clear warning that doing so is bad); from L1/L2's perspective, there is no VM-Entry so writing vmcs12.VM_ENTRY_INTR_INFO should never generate an event in L2, even on CPUs without VMCS caching.
On 07/01/21 21:19, Sean Christopherson wrote: >> That said, the v1 change had some appeal to it. > > Which v1 change are you referring to? Moving the to-be-injected event from eventinj to vcpu->arch, and from there to vmcb02 on the next vmentry's inject_pending_event. >> In the VMX case (if properly implemented) it would allow removing the weird >> nested_run_pending case from prepare_vmcs02_early. I think it's a valuable >> invariant that there are no events in the VMCS after each KVM_RUN iteration, >> and this special case is breaking the invariant. > > Hmm, as weird as that code is, I think it's actually the most architecturally > correct behavior. I was referring to the "then" branch therein. :) Paolo
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index ee4f2082ad1bd..cc3130ab612e5 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -1200,6 +1200,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, * in the registers, the save area of the nested state instead * contains saved L1 state. */ + + svm->nested.nested_run_pending = + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); + copy_vmcb_control_area(&hsave->control, &svm->vmcb->control); hsave->save = *save;
The code to store it on the migration exists, but no code was restoring it. One of the side effects of fixing this is that L1->L2 injected events are no longer lost when migration happens with nested run pending. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/svm/nested.c | 4 ++++ 1 file changed, 4 insertions(+)