| Message ID | 20211213104634.199141-2-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: KVM: SVM: Allow L1's AVIC to co-exist with nesting | expand |
On 12/13/21 11:46, Maxim Levitsky wrote: > Fix a corner case in which L1 hypervisor intercepts interrupts (INTERCEPT_INTR) > and either doesn't use virtual interrupt masking (V_INTR_MASKING) or > enters a nested guest with EFLAGS.IF disabled prior to the entry. > > In this case, despite the fact that L1 intercepts the interrupts, > KVM still needs to set up an interrupt window to wait before it > can deliver INTR vmexit. > > Currently instead, the KVM enters an endless loop of 'req_immediate_exit'. > > Note that on VMX this case is impossible as there is only > 'vmexit on external interrupts' execution control which either set, > in which case both host and guest's EFLAGS.IF > is ignored, or clear, in which case no VMexit is delivered. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/x86/kvm/svm/svm.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index e57e6857e0630..c9668a3b51011 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -3372,17 +3372,21 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu) > static int svm_interrupt_allowed(struct kvm_vcpu *vcpu, bool for_injection) > { > struct vcpu_svm *svm = to_svm(vcpu); > + bool blocked; > + > if (svm->nested.nested_run_pending) > return -EBUSY; > > + blocked = svm_interrupt_blocked(vcpu); > + > /* > * An IRQ must not be injected into L2 if it's supposed to VM-Exit, > * e.g. if the IRQ arrived asynchronously after checking nested events. > */ > if (for_injection && is_guest_mode(vcpu) && nested_exit_on_intr(svm)) > - return -EBUSY; > - > - return !svm_interrupt_blocked(vcpu); > + return !blocked ? -EBUSY : 0; > + else > + return !blocked; > } > > static void svm_enable_irq_window(struct kvm_vcpu *vcpu) > Right, another case is when CLGI is not trapped and the guest therefore runs with GIF=0. I think that means that a similar change has to be done in all the *_allowed functions. I would write it as if (svm->nested.nested_run_pending) return -EBUSY; if (svm_interrupt_blocked(vcpu)) return 0; /* * An IRQ must not be injected into L2 if it's supposed to VM-Exit, * e.g. if the IRQ arrived asynchronously after checking nested events. */ if (for_injection && is_guest_mode(vcpu) && nested_exit_on_intr(svm)) return -EBUSY; return 1; Paolo
On Mon, 2021-12-13 at 12:34 +0100, Paolo Bonzini wrote: > On 12/13/21 11:46, Maxim Levitsky wrote: > > Fix a corner case in which L1 hypervisor intercepts interrupts (INTERCEPT_INTR) > > and either doesn't use virtual interrupt masking (V_INTR_MASKING) or > > enters a nested guest with EFLAGS.IF disabled prior to the entry. > > > > In this case, despite the fact that L1 intercepts the interrupts, > > KVM still needs to set up an interrupt window to wait before it > > can deliver INTR vmexit. > > > > Currently instead, the KVM enters an endless loop of 'req_immediate_exit'. > > > > Note that on VMX this case is impossible as there is only > > 'vmexit on external interrupts' execution control which either set, > > in which case both host and guest's EFLAGS.IF > > is ignored, or clear, in which case no VMexit is delivered. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > arch/x86/kvm/svm/svm.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > > index e57e6857e0630..c9668a3b51011 100644 > > --- a/arch/x86/kvm/svm/svm.c > > +++ b/arch/x86/kvm/svm/svm.c > > @@ -3372,17 +3372,21 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu) > > static int svm_interrupt_allowed(struct kvm_vcpu *vcpu, bool for_injection) > > { > > struct vcpu_svm *svm = to_svm(vcpu); > > + bool blocked; > > + > > if (svm->nested.nested_run_pending) > > return -EBUSY; > > > > + blocked = svm_interrupt_blocked(vcpu); > > + > > /* > > * An IRQ must not be injected into L2 if it's supposed to VM-Exit, > > * e.g. if the IRQ arrived asynchronously after checking nested events. > > */ > > if (for_injection && is_guest_mode(vcpu) && nested_exit_on_intr(svm)) > > - return -EBUSY; > > - > > - return !svm_interrupt_blocked(vcpu); > > + return !blocked ? -EBUSY : 0; > > + else > > + return !blocked; > > } > > > > static void svm_enable_irq_window(struct kvm_vcpu *vcpu) > > > > Right, another case is when CLGI is not trapped and the guest therefore > runs with GIF=0. I think that means that a similar change has to be > done in all the *_allowed functions. I think that SVM sets real GIF to 1 on VMentry regardless if it is trapped or not. However if not trapped, and neither EFLAGS.IF is trapped, one could enter a guest that has EFLAGS.IF == 0, then the guest could disable GIF, enable EFLAGS.IF, and then enable GIF, but then GIF enablement should trigger out interrupt window VINTR as well. > > I would write it as > > if (svm->nested.nested_run_pending) > return -EBUSY; > > if (svm_interrupt_blocked(vcpu)) > return 0; > > /* > * An IRQ must not be injected into L2 if it's supposed to VM-Exit, > * e.g. if the IRQ arrived asynchronously after checking nested events. > */ > if (for_injection && is_guest_mode(vcpu) && nested_exit_on_intr(svm)) > return -EBUSY; > return 1; Thanks! I was worried to not break the non nested case but looking again at the code, it is logically equivalent. Thanks for the review, Best regards, Maxim Levitsky > > Paolo >
On 12/13/21 14:07, Maxim Levitsky wrote: >> Right, another case is when CLGI is not trapped and the guest therefore >> runs with GIF=0. I think that means that a similar change has to be >> done in all the *_allowed functions. > > I think that SVM sets real GIF to 1 on VMentry regardless if it is trapped or not. Yes, the issue is only when CLGI is not trapped (and vGIF is disabled). > However if not trapped, and neither EFLAGS.IF is trapped, one could enter a guest > that has EFLAGS.IF == 0, then the guest could disable GIF, enable EFLAGS.IF, > and then enable GIF, but then GIF enablement should trigger out interrupt window > VINTR as well. While GIF=0 you have svm_nmi_blocked returning true and svm_nmi_allowed returning -EBUSY; that's wrong isn't it? Paolo
On Mon, 2021-12-13 at 14:15 +0100, Paolo Bonzini wrote: > On 12/13/21 14:07, Maxim Levitsky wrote: > > > Right, another case is when CLGI is not trapped and the guest therefore > > > runs with GIF=0. I think that means that a similar change has to be > > > done in all the *_allowed functions. > > > > I think that SVM sets real GIF to 1 on VMentry regardless if it is trapped or not. > > Yes, the issue is only when CLGI is not trapped (and vGIF is disabled). Yes, but I just wanted to clarify that GIF is initially enabled on VM entry regardless if it is trapped or not, after that the guest can indeed disable the GIF if CLGI/STGI is not trapped and vGIF disabled. > > > However if not trapped, and neither EFLAGS.IF is trapped, one could enter a guest > > that has EFLAGS.IF == 0, then the guest could disable GIF, enable EFLAGS.IF, > > and then enable GIF, but then GIF enablement should trigger out interrupt window > > VINTR as well. > > While GIF=0 you have svm_nmi_blocked returning true and svm_nmi_allowed > returning -EBUSY; that's wrong isn't it? Yes, 100% agree, patch (and unit test for this as well) is on the way! Best regards. Maxim Levitsky > > Paolo >
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index e57e6857e0630..c9668a3b51011 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -3372,17 +3372,21 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu) static int svm_interrupt_allowed(struct kvm_vcpu *vcpu, bool for_injection) { struct vcpu_svm *svm = to_svm(vcpu); + bool blocked; + if (svm->nested.nested_run_pending) return -EBUSY; + blocked = svm_interrupt_blocked(vcpu); + /* * An IRQ must not be injected into L2 if it's supposed to VM-Exit, * e.g. if the IRQ arrived asynchronously after checking nested events. */ if (for_injection && is_guest_mode(vcpu) && nested_exit_on_intr(svm)) - return -EBUSY; - - return !svm_interrupt_blocked(vcpu); + return !blocked ? -EBUSY : 0; + else + return !blocked; } static void svm_enable_irq_window(struct kvm_vcpu *vcpu)
Fix a corner case in which L1 hypervisor intercepts interrupts (INTERCEPT_INTR) and either doesn't use virtual interrupt masking (V_INTR_MASKING) or enters a nested guest with EFLAGS.IF disabled prior to the entry. In this case, despite the fact that L1 intercepts the interrupts, KVM still needs to set up an interrupt window to wait before it can deliver INTR vmexit. Currently instead, the KVM enters an endless loop of 'req_immediate_exit'. Note that on VMX this case is impossible as there is only 'vmexit on external interrupts' execution control which either set, in which case both host and guest's EFLAGS.IF is ignored, or clear, in which case no VMexit is delivered. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/svm/svm.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)