Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1718808 > unrolled thread
| Started by | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| First post | 2017-08-24 06:30 +0200 |
| Last post | 2017-08-24 11:50 +0200 |
| Articles | 8 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected Wanpeng Li <kernellwp@gmail.com> - 2017-08-24 06:30 +0200
Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected Wanpeng Li <kernellwp@gmail.com> - 2017-08-24 09:00 +0200
Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected Paolo Bonzini <pbonzini@redhat.com> - 2017-08-24 11:00 +0200
Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected Wanpeng Li <kernellwp@gmail.com> - 2017-08-24 11:20 +0200
Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected Wanpeng Li <kernellwp@gmail.com> - 2017-08-24 11:40 +0200
Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected Paolo Bonzini <pbonzini@redhat.com> - 2017-08-24 11:50 +0200
Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected Paolo Bonzini <pbonzini@redhat.com> - 2017-08-24 11:40 +0200
Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected Wanpeng Li <kernellwp@gmail.com> - 2017-08-24 11:50 +0200
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2017-08-24 06:30 +0200 |
| Subject | [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected |
| Message-ID | <uhRWi-1yC-3@gated-at.bofh.it> |
From: Wanpeng Li <wanpeng.li@hotmail.com>
vmx_complete_interrupts() assumes that the exception is always injected,
so it would be dropped by kvm_clear_exception_queue(). This patch separates
exception.pending from exception.injected, exception.inject represents the
exception is injected or the exception should be reinjected due to vmexit
occurs during event delivery in VMX non-root operation. exception.pending
represents the exception is queued and will be cleared when injecting the
exception to the guest. So exception.pending and exception.injected can
cooperate to guarantee exception will not be lost.
Reported-by: Radim Krčmář <rkrcmar@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v2 -> v3:
* the changes to inject_pending_event and adds the WARN_ON
* combine injected and pending exception for GET/SET_VCPU_EVENTS
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/svm.c | 2 +-
arch/x86/kvm/vmx.c | 4 +--
arch/x86/kvm/x86.c | 56 ++++++++++++++++++++++++++---------------
arch/x86/kvm/x86.h | 4 +--
5 files changed, 42 insertions(+), 26 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9d90787..6e385ac 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -547,8 +547,8 @@ struct kvm_vcpu_arch {
struct kvm_queued_exception {
bool pending;
+ bool injected;
bool has_error_code;
- bool reinject;
u8 nr;
u32 error_code;
u8 nested_apf;
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index a2fddce..6a439a1 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -655,7 +655,7 @@ static void svm_queue_exception(struct kvm_vcpu *vcpu)
struct vcpu_svm *svm = to_svm(vcpu);
unsigned nr = vcpu->arch.exception.nr;
bool has_error_code = vcpu->arch.exception.has_error_code;
- bool reinject = vcpu->arch.exception.reinject;
+ bool reinject = vcpu->arch.exception.injected;
u32 error_code = vcpu->arch.exception.error_code;
/*
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c5f43ab..902b780 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2503,7 +2503,7 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu)
struct vcpu_vmx *vmx = to_vmx(vcpu);
unsigned nr = vcpu->arch.exception.nr;
bool has_error_code = vcpu->arch.exception.has_error_code;
- bool reinject = vcpu->arch.exception.reinject;
+ bool reinject = vcpu->arch.exception.injected;
u32 error_code = vcpu->arch.exception.error_code;
u32 intr_info = nr | INTR_INFO_VALID_MASK;
@@ -10948,7 +10948,7 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu,
u32 idt_vectoring;
unsigned int nr;
- if (vcpu->arch.exception.pending && vcpu->arch.exception.reinject) {
+ if (vcpu->arch.exception.injected) {
nr = vcpu->arch.exception.nr;
idt_vectoring = nr | VECTORING_INFO_VALID_MASK;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8f41b88..b698b2f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -389,15 +389,18 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
kvm_make_request(KVM_REQ_EVENT, vcpu);
- if (!vcpu->arch.exception.pending) {
+ if (!vcpu->arch.exception.pending ||
+ !vcpu->arch.exception.injected) {
queue:
if (has_error && !is_protmode(vcpu))
has_error = false;
- vcpu->arch.exception.pending = true;
+ if (reinject)
+ vcpu->arch.exception.injected = true;
+ else
+ vcpu->arch.exception.pending = true;
vcpu->arch.exception.has_error_code = has_error;
vcpu->arch.exception.nr = nr;
vcpu->arch.exception.error_code = error_code;
- vcpu->arch.exception.reinject = reinject;
return;
}
@@ -3069,8 +3072,14 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
struct kvm_vcpu_events *events)
{
process_nmi(vcpu);
+ /*
+ * FIXME: pass injected and pending separately. This is only
+ * needed for nested virtualization, whose state cannot be
+ * migrated yet. For now we combine them just in case.
+ */
events->exception.injected =
- vcpu->arch.exception.pending &&
+ (vcpu->arch.exception.pending ||
+ vcpu->arch.exception.injected) &&
!kvm_exception_is_soft(vcpu->arch.exception.nr);
events->exception.nr = vcpu->arch.exception.nr;
events->exception.has_error_code = vcpu->arch.exception.has_error_code;
@@ -3125,6 +3134,7 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
return -EINVAL;
process_nmi(vcpu);
+ vcpu->arch.exception.injected = false;
vcpu->arch.exception.pending = events->exception.injected;
vcpu->arch.exception.nr = events->exception.nr;
vcpu->arch.exception.has_error_code = events->exception.has_error_code;
@@ -6350,21 +6360,7 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
int r;
/* try to reinject previous events if any */
- if (vcpu->arch.exception.pending) {
- trace_kvm_inj_exception(vcpu->arch.exception.nr,
- vcpu->arch.exception.has_error_code,
- vcpu->arch.exception.error_code);
-
- if (exception_type(vcpu->arch.exception.nr) == EXCPT_FAULT)
- __kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
- X86_EFLAGS_RF);
-
- if (vcpu->arch.exception.nr == DB_VECTOR &&
- (vcpu->arch.dr7 & DR7_GD)) {
- vcpu->arch.dr7 &= ~DR7_GD;
- kvm_update_dr7(vcpu);
- }
-
+ if (vcpu->arch.exception.injected) {
kvm_x86_ops->queue_exception(vcpu);
return 0;
}
@@ -6386,7 +6382,25 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
}
/* try to inject new event if pending */
- if (vcpu->arch.smi_pending && !is_smm(vcpu)) {
+ if (vcpu->arch.exception.pending) {
+ trace_kvm_inj_exception(vcpu->arch.exception.nr,
+ vcpu->arch.exception.has_error_code,
+ vcpu->arch.exception.error_code);
+
+ vcpu->arch.exception.pending = false;
+ vcpu->arch.exception.injected = true;
+ kvm_x86_ops->queue_exception(vcpu);
+
+ if (exception_type(vcpu->arch.exception.nr) == EXCPT_FAULT)
+ __kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
+ X86_EFLAGS_RF);
+
+ if (vcpu->arch.exception.nr == DB_VECTOR &&
+ (vcpu->arch.dr7 & DR7_GD)) {
+ vcpu->arch.dr7 &= ~DR7_GD;
+ kvm_update_dr7(vcpu);
+ }
+ } else if (vcpu->arch.smi_pending && !is_smm(vcpu)) {
vcpu->arch.smi_pending = false;
enter_smm(vcpu);
} else if (vcpu->arch.nmi_pending && kvm_x86_ops->nmi_allowed(vcpu)) {
@@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
kvm_x86_ops->enable_nmi_window(vcpu);
if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win)
kvm_x86_ops->enable_irq_window(vcpu);
+ WARN_ON(vcpu->arch.exception.pending);
}
if (kvm_lapic_enabled(vcpu)) {
@@ -7738,6 +7753,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
vcpu->arch.nmi_injected = false;
kvm_clear_interrupt_queue(vcpu);
kvm_clear_exception_queue(vcpu);
+ vcpu->arch.exception.pending = false;
memset(vcpu->arch.db, 0, sizeof(vcpu->arch.db));
kvm_update_dr0123(vcpu);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 1134603..e6ec0de 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -11,7 +11,7 @@
static inline void kvm_clear_exception_queue(struct kvm_vcpu *vcpu)
{
- vcpu->arch.exception.pending = false;
+ vcpu->arch.exception.injected = false;
}
static inline void kvm_queue_interrupt(struct kvm_vcpu *vcpu, u8 vector,
@@ -29,7 +29,7 @@ static inline void kvm_clear_interrupt_queue(struct kvm_vcpu *vcpu)
static inline bool kvm_event_needs_reinjection(struct kvm_vcpu *vcpu)
{
- return vcpu->arch.exception.pending || vcpu->arch.interrupt.pending ||
+ return vcpu->arch.exception.injected || vcpu->arch.interrupt.pending ||
vcpu->arch.nmi_injected;
}
--
2.7.4
[toc] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2017-08-24 09:00 +0200 |
| Message-ID | <uhUhr-2ZF-5@gated-at.bofh.it> |
| In reply to | #1718808 |
2017-08-24 12:21 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> vmx_complete_interrupts() assumes that the exception is always injected,
> so it would be dropped by kvm_clear_exception_queue(). This patch separates
> exception.pending from exception.injected, exception.inject represents the
> exception is injected or the exception should be reinjected due to vmexit
> occurs during event delivery in VMX non-root operation. exception.pending
> represents the exception is queued and will be cleared when injecting the
> exception to the guest. So exception.pending and exception.injected can
> cooperate to guarantee exception will not be lost.
>
> Reported-by: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v2 -> v3:
> * the changes to inject_pending_event and adds the WARN_ON
> * combine injected and pending exception for GET/SET_VCPU_EVENTS
>
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kvm/svm.c | 2 +-
> arch/x86/kvm/vmx.c | 4 +--
> arch/x86/kvm/x86.c | 56 ++++++++++++++++++++++++++---------------
> arch/x86/kvm/x86.h | 4 +--
> 5 files changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 9d90787..6e385ac 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -547,8 +547,8 @@ struct kvm_vcpu_arch {
>
> struct kvm_queued_exception {
> bool pending;
> + bool injected;
> bool has_error_code;
> - bool reinject;
> u8 nr;
> u32 error_code;
> u8 nested_apf;
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index a2fddce..6a439a1 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -655,7 +655,7 @@ static void svm_queue_exception(struct kvm_vcpu *vcpu)
> struct vcpu_svm *svm = to_svm(vcpu);
> unsigned nr = vcpu->arch.exception.nr;
> bool has_error_code = vcpu->arch.exception.has_error_code;
> - bool reinject = vcpu->arch.exception.reinject;
> + bool reinject = vcpu->arch.exception.injected;
> u32 error_code = vcpu->arch.exception.error_code;
>
> /*
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c5f43ab..902b780 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2503,7 +2503,7 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu)
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned nr = vcpu->arch.exception.nr;
> bool has_error_code = vcpu->arch.exception.has_error_code;
> - bool reinject = vcpu->arch.exception.reinject;
> + bool reinject = vcpu->arch.exception.injected;
> u32 error_code = vcpu->arch.exception.error_code;
> u32 intr_info = nr | INTR_INFO_VALID_MASK;
>
> @@ -10948,7 +10948,7 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu,
> u32 idt_vectoring;
> unsigned int nr;
>
> - if (vcpu->arch.exception.pending && vcpu->arch.exception.reinject) {
> + if (vcpu->arch.exception.injected) {
> nr = vcpu->arch.exception.nr;
> idt_vectoring = nr | VECTORING_INFO_VALID_MASK;
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8f41b88..b698b2f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -389,15 +389,18 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
>
> kvm_make_request(KVM_REQ_EVENT, vcpu);
>
> - if (!vcpu->arch.exception.pending) {
> + if (!vcpu->arch.exception.pending ||
> + !vcpu->arch.exception.injected) {
> queue:
> if (has_error && !is_protmode(vcpu))
> has_error = false;
> - vcpu->arch.exception.pending = true;
> + if (reinject)
> + vcpu->arch.exception.injected = true;
> + else
> + vcpu->arch.exception.pending = true;
> vcpu->arch.exception.has_error_code = has_error;
> vcpu->arch.exception.nr = nr;
> vcpu->arch.exception.error_code = error_code;
> - vcpu->arch.exception.reinject = reinject;
> return;
> }
>
> @@ -3069,8 +3072,14 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
> struct kvm_vcpu_events *events)
> {
> process_nmi(vcpu);
> + /*
> + * FIXME: pass injected and pending separately. This is only
> + * needed for nested virtualization, whose state cannot be
> + * migrated yet. For now we combine them just in case.
> + */
> events->exception.injected =
> - vcpu->arch.exception.pending &&
> + (vcpu->arch.exception.pending ||
> + vcpu->arch.exception.injected) &&
> !kvm_exception_is_soft(vcpu->arch.exception.nr);
> events->exception.nr = vcpu->arch.exception.nr;
> events->exception.has_error_code = vcpu->arch.exception.has_error_code;
> @@ -3125,6 +3134,7 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
> return -EINVAL;
>
> process_nmi(vcpu);
> + vcpu->arch.exception.injected = false;
> vcpu->arch.exception.pending = events->exception.injected;
> vcpu->arch.exception.nr = events->exception.nr;
> vcpu->arch.exception.has_error_code = events->exception.has_error_code;
> @@ -6350,21 +6360,7 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
> int r;
>
> /* try to reinject previous events if any */
> - if (vcpu->arch.exception.pending) {
> - trace_kvm_inj_exception(vcpu->arch.exception.nr,
> - vcpu->arch.exception.has_error_code,
> - vcpu->arch.exception.error_code);
> -
> - if (exception_type(vcpu->arch.exception.nr) == EXCPT_FAULT)
> - __kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
> - X86_EFLAGS_RF);
> -
> - if (vcpu->arch.exception.nr == DB_VECTOR &&
> - (vcpu->arch.dr7 & DR7_GD)) {
> - vcpu->arch.dr7 &= ~DR7_GD;
> - kvm_update_dr7(vcpu);
> - }
> -
> + if (vcpu->arch.exception.injected) {
> kvm_x86_ops->queue_exception(vcpu);
> return 0;
> }
> @@ -6386,7 +6382,25 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
> }
>
> /* try to inject new event if pending */
> - if (vcpu->arch.smi_pending && !is_smm(vcpu)) {
> + if (vcpu->arch.exception.pending) {
> + trace_kvm_inj_exception(vcpu->arch.exception.nr,
> + vcpu->arch.exception.has_error_code,
> + vcpu->arch.exception.error_code);
> +
> + vcpu->arch.exception.pending = false;
> + vcpu->arch.exception.injected = true;
> + kvm_x86_ops->queue_exception(vcpu);
> +
> + if (exception_type(vcpu->arch.exception.nr) == EXCPT_FAULT)
> + __kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
> + X86_EFLAGS_RF);
> +
> + if (vcpu->arch.exception.nr == DB_VECTOR &&
> + (vcpu->arch.dr7 & DR7_GD)) {
> + vcpu->arch.dr7 &= ~DR7_GD;
> + kvm_update_dr7(vcpu);
> + }
> + } else if (vcpu->arch.smi_pending && !is_smm(vcpu)) {
> vcpu->arch.smi_pending = false;
> enter_smm(vcpu);
> } else if (vcpu->arch.nmi_pending && kvm_x86_ops->nmi_allowed(vcpu)) {
> @@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> kvm_x86_ops->enable_nmi_window(vcpu);
> if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win)
> kvm_x86_ops->enable_irq_window(vcpu);
> + WARN_ON(vcpu->arch.exception.pending);
This WARN_ON() is suggested during the review of last version,
however, there are many cases in inject_pending_event() can result in
return directly w/ vcpu->arch.exception.pending is true. Actually I
have already catched the warning several times during the testing. I
think we should remove it when committing.
Regards,
Wanpeng Li
> }
>
> if (kvm_lapic_enabled(vcpu)) {
> @@ -7738,6 +7753,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> vcpu->arch.nmi_injected = false;
> kvm_clear_interrupt_queue(vcpu);
> kvm_clear_exception_queue(vcpu);
> + vcpu->arch.exception.pending = false;
>
> memset(vcpu->arch.db, 0, sizeof(vcpu->arch.db));
> kvm_update_dr0123(vcpu);
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 1134603..e6ec0de 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -11,7 +11,7 @@
>
> static inline void kvm_clear_exception_queue(struct kvm_vcpu *vcpu)
> {
> - vcpu->arch.exception.pending = false;
> + vcpu->arch.exception.injected = false;
> }
>
> static inline void kvm_queue_interrupt(struct kvm_vcpu *vcpu, u8 vector,
> @@ -29,7 +29,7 @@ static inline void kvm_clear_interrupt_queue(struct kvm_vcpu *vcpu)
>
> static inline bool kvm_event_needs_reinjection(struct kvm_vcpu *vcpu)
> {
> - return vcpu->arch.exception.pending || vcpu->arch.interrupt.pending ||
> + return vcpu->arch.exception.injected || vcpu->arch.interrupt.pending ||
> vcpu->arch.nmi_injected;
> }
>
> --
> 2.7.4
>
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2017-08-24 11:00 +0200 |
| Subject | Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected |
| Message-ID | <uhW9B-4bT-41@gated-at.bofh.it> |
| In reply to | #1718890 |
On 24/08/2017 08:52, Wanpeng Li wrote:
>> @@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> kvm_x86_ops->enable_nmi_window(vcpu);
>> if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win)
>> kvm_x86_ops->enable_irq_window(vcpu);
>> + WARN_ON(vcpu->arch.exception.pending);
>
> This WARN_ON() is suggested during the review of last version,
> however, there are many cases in inject_pending_event() can result in
> return directly w/ vcpu->arch.exception.pending is true. Actually I
> have already catched the warning several times during the testing. I
> think we should remove it when committing.
No, it's a good thing that it's failing, because it's finding a bug.
There's no such thing as an "exception window", so at the very least it
should set req_immediate_exit to true.
Does this help?
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b698b2f135a2..76d5a192be6c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6365,14 +6365,20 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
return 0;
}
- if (vcpu->arch.nmi_injected) {
- kvm_x86_ops->set_nmi(vcpu);
- return 0;
- }
+ /*
+ * Exceptions must be injected immediately, or the exception
+ * frame will have the address of the NMI or interrupt handler.
+ */
+ if (!vcpu->arch.exception.pending) {
+ if (vcpu->arch.nmi_injected) {
+ kvm_x86_ops->set_nmi(vcpu);
+ return 0;
+ }
- if (vcpu->arch.interrupt.pending) {
- kvm_x86_ops->set_irq(vcpu);
- return 0;
+ if (vcpu->arch.interrupt.pending) {
+ kvm_x86_ops->set_irq(vcpu);
+ return 0;
+ }
}
if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) {
Paolo
[toc] | [prev] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2017-08-24 11:20 +0200 |
| Message-ID | <uhWsV-4xQ-13@gated-at.bofh.it> |
| In reply to | #1719031 |
2017-08-24 16:57 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 24/08/2017 08:52, Wanpeng Li wrote:
>>> @@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>> kvm_x86_ops->enable_nmi_window(vcpu);
>>> if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win)
>>> kvm_x86_ops->enable_irq_window(vcpu);
>>> + WARN_ON(vcpu->arch.exception.pending);
>>
>> This WARN_ON() is suggested during the review of last version,
>> however, there are many cases in inject_pending_event() can result in
>> return directly w/ vcpu->arch.exception.pending is true. Actually I
>> have already catched the warning several times during the testing. I
>> think we should remove it when committing.
>
> No, it's a good thing that it's failing, because it's finding a bug.
> There's no such thing as an "exception window", so at the very least it
Good point, the code looks good, I will fold it in next version.
However, I still can observe the warning.
Regards,
Wanpeng Li
> should set req_immediate_exit to true.
>
> Does this help?
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b698b2f135a2..76d5a192be6c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6365,14 +6365,20 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
> return 0;
> }
>
> - if (vcpu->arch.nmi_injected) {
> - kvm_x86_ops->set_nmi(vcpu);
> - return 0;
> - }
> + /*
> + * Exceptions must be injected immediately, or the exception
> + * frame will have the address of the NMI or interrupt handler.
> + */
> + if (!vcpu->arch.exception.pending) {
> + if (vcpu->arch.nmi_injected) {
> + kvm_x86_ops->set_nmi(vcpu);
> + return 0;
> + }
>
> - if (vcpu->arch.interrupt.pending) {
> - kvm_x86_ops->set_irq(vcpu);
> - return 0;
> + if (vcpu->arch.interrupt.pending) {
> + kvm_x86_ops->set_irq(vcpu);
> + return 0;
> + }
> }
>
> if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) {
>
> Paolo
[toc] | [prev] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2017-08-24 11:40 +0200 |
| Message-ID | <uhWMi-4E5-17@gated-at.bofh.it> |
| In reply to | #1719047 |
2017-08-24 17:13 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2017-08-24 16:57 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> On 24/08/2017 08:52, Wanpeng Li wrote:
>>>> @@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>>> kvm_x86_ops->enable_nmi_window(vcpu);
>>>> if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win)
>>>> kvm_x86_ops->enable_irq_window(vcpu);
>>>> + WARN_ON(vcpu->arch.exception.pending);
>>>
>>> This WARN_ON() is suggested during the review of last version,
>>> however, there are many cases in inject_pending_event() can result in
>>> return directly w/ vcpu->arch.exception.pending is true. Actually I
>>> have already catched the warning several times during the testing. I
>>> think we should remove it when committing.
>>
>> No, it's a good thing that it's failing, because it's finding a bug.
>> There's no such thing as an "exception window", so at the very least it
>
> Good point, the code looks good, I will fold it in next version.
> However, I still can observe the warning.
>
> Regards,
> Wanpeng Li
I observed sometimes both vcpu->arch.exception.pending and
vcpu->arch.expception.injected are true before executing below codes:
if (vcpu->arch.exception.injected) {
kvm_x86_ops->queue_exception(vcpu);
return 0;
}
Regards,
Wanpeng Li
>
>> should set req_immediate_exit to true.
>>
>> Does this help?
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index b698b2f135a2..76d5a192be6c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6365,14 +6365,20 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
>> return 0;
>> }
>>
>> - if (vcpu->arch.nmi_injected) {
>> - kvm_x86_ops->set_nmi(vcpu);
>> - return 0;
>> - }
>> + /*
>> + * Exceptions must be injected immediately, or the exception
>> + * frame will have the address of the NMI or interrupt handler.
>> + */
>> + if (!vcpu->arch.exception.pending) {
>> + if (vcpu->arch.nmi_injected) {
>> + kvm_x86_ops->set_nmi(vcpu);
>> + return 0;
>> + }
>>
>> - if (vcpu->arch.interrupt.pending) {
>> - kvm_x86_ops->set_irq(vcpu);
>> - return 0;
>> + if (vcpu->arch.interrupt.pending) {
>> + kvm_x86_ops->set_irq(vcpu);
>> + return 0;
>> + }
>> }
>>
>> if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) {
>>
>> Paolo
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2017-08-24 11:50 +0200 |
| Subject | Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected |
| Message-ID | <uhWVY-4Hm-5@gated-at.bofh.it> |
| In reply to | #1719067 |
On 24/08/2017 11:34, Wanpeng Li wrote:
> 2017-08-24 17:13 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>> 2017-08-24 16:57 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>> On 24/08/2017 08:52, Wanpeng Li wrote:
>>>>> @@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>>>> kvm_x86_ops->enable_nmi_window(vcpu);
>>>>> if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win)
>>>>> kvm_x86_ops->enable_irq_window(vcpu);
>>>>> + WARN_ON(vcpu->arch.exception.pending);
>>>>
>>>> This WARN_ON() is suggested during the review of last version,
>>>> however, there are many cases in inject_pending_event() can result in
>>>> return directly w/ vcpu->arch.exception.pending is true. Actually I
>>>> have already catched the warning several times during the testing. I
>>>> think we should remove it when committing.
>>>
>>> No, it's a good thing that it's failing, because it's finding a bug.
>>> There's no such thing as an "exception window", so at the very least it
>>
>> Good point, the code looks good, I will fold it in next version.
>> However, I still can observe the warning.
>>
>> Regards,
>> Wanpeng Li
>
> I observed sometimes both vcpu->arch.exception.pending and
> vcpu->arch.expception.injected are true before executing below codes:
>
> if (vcpu->arch.exception.injected) {
> kvm_x86_ops->queue_exception(vcpu);
> return 0;
> }
More missing pieces:
1) kvm_x86_ops->queue_exception must be called in the
vcpu->arch.exception.pending case. Compare it with the others, which
are calling enter_smm, kvm_x86_ops->set_irq, kvm_x86_ops->set_nmi.
2) here:
if (!vcpu->arch.exception.pending ||
!vcpu->arch.exception.injected) {
queue:
if (has_error && !is_protmode(vcpu))
has_error = false;
if (reinject)
vcpu->arch.exception.injected = true;
else
vcpu->arch.exception.pending = true;
you need to reset the other field, because you can get here from the
double-fault case. Likewise below:
if ((class1 == EXCPT_CONTRIBUTORY && class2 == EXCPT_CONTRIBUTORY)
|| (class1 == EXCPT_PF && class2 != EXCPT_BENIGN)) {
/* generate double fault per SDM Table 5-5 */
vcpu->arch.exception.pending = true;
injected must be cleared.
> Regards,
> Wanpeng Li
>
>>
>>> should set req_immediate_exit to true.
>>>
>>> Does this help?
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index b698b2f135a2..76d5a192be6c 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -6365,14 +6365,20 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
>>> return 0;
>>> }
>>>
>>> - if (vcpu->arch.nmi_injected) {
>>> - kvm_x86_ops->set_nmi(vcpu);
>>> - return 0;
>>> - }
>>> + /*
>>> + * Exceptions must be injected immediately, or the exception
>>> + * frame will have the address of the NMI or interrupt handler.
>>> + */
>>> + if (!vcpu->arch.exception.pending) {
>>> + if (vcpu->arch.nmi_injected) {
>>> + kvm_x86_ops->set_nmi(vcpu);
>>> + return 0;
>>> + }
>>>
>>> - if (vcpu->arch.interrupt.pending) {
>>> - kvm_x86_ops->set_irq(vcpu);
>>> - return 0;
>>> + if (vcpu->arch.interrupt.pending) {
>>> + kvm_x86_ops->set_irq(vcpu);
>>> + return 0;
>>> + }
>>> }
>>>
>>> if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) {
>>>
>>> Paolo
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2017-08-24 11:40 +0200 |
| Subject | Re: [PATCH v3 2/4] KVM: X86: Fix loss of exception which has not yet injected |
| Message-ID | <uhWMi-4E5-19@gated-at.bofh.it> |
| In reply to | #1719047 |
On 24/08/2017 11:13, Wanpeng Li wrote:
> 2017-08-24 16:57 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> On 24/08/2017 08:52, Wanpeng Li wrote:
>>>> @@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>>> kvm_x86_ops->enable_nmi_window(vcpu);
>>>> if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win)
>>>> kvm_x86_ops->enable_irq_window(vcpu);
>>>> + WARN_ON(vcpu->arch.exception.pending);
>>>
>>> This WARN_ON() is suggested during the review of last version,
>>> however, there are many cases in inject_pending_event() can result in
>>> return directly w/ vcpu->arch.exception.pending is true. Actually I
>>> have already catched the warning several times during the testing. I
>>> think we should remove it when committing.
>>
>> No, it's a good thing that it's failing, because it's finding a bug.
>> There's no such thing as an "exception window", so at the very least it
>
> Good point, the code looks good, I will fold it in next version.
> However, I still can observe the warning.
In patch 4, vmx_check_nested_events must clear vcpu->ex.pending.
Paolo
> Regards,
> Wanpeng Li
>
>> should set req_immediate_exit to true.
>>
>> Does this help?
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index b698b2f135a2..76d5a192be6c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6365,14 +6365,20 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
>> return 0;
>> }
>>
>> - if (vcpu->arch.nmi_injected) {
>> - kvm_x86_ops->set_nmi(vcpu);
>> - return 0;
>> - }
>> + /*
>> + * Exceptions must be injected immediately, or the exception
>> + * frame will have the address of the NMI or interrupt handler.
>> + */
>> + if (!vcpu->arch.exception.pending) {
>> + if (vcpu->arch.nmi_injected) {
>> + kvm_x86_ops->set_nmi(vcpu);
>> + return 0;
>> + }
>>
>> - if (vcpu->arch.interrupt.pending) {
>> - kvm_x86_ops->set_irq(vcpu);
>> - return 0;
>> + if (vcpu->arch.interrupt.pending) {
>> + kvm_x86_ops->set_irq(vcpu);
>> + return 0;
>> + }
>> }
>>
>> if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) {
>>
>> Paolo
[toc] | [prev] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2017-08-24 11:50 +0200 |
| Message-ID | <uhWVZ-4Hm-33@gated-at.bofh.it> |
| In reply to | #1719068 |
2017-08-24 17:35 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 24/08/2017 11:13, Wanpeng Li wrote:
>> 2017-08-24 16:57 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>> On 24/08/2017 08:52, Wanpeng Li wrote:
>>>>> @@ -6862,6 +6876,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>>>> kvm_x86_ops->enable_nmi_window(vcpu);
>>>>> if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win)
>>>>> kvm_x86_ops->enable_irq_window(vcpu);
>>>>> + WARN_ON(vcpu->arch.exception.pending);
>>>>
>>>> This WARN_ON() is suggested during the review of last version,
>>>> however, there are many cases in inject_pending_event() can result in
>>>> return directly w/ vcpu->arch.exception.pending is true. Actually I
>>>> have already catched the warning several times during the testing. I
>>>> think we should remove it when committing.
>>>
>>> No, it's a good thing that it's failing, because it's finding a bug.
>>> There's no such thing as an "exception window", so at the very least it
>>
>> Good point, the code looks good, I will fold it in next version.
>> However, I still can observe the warning.
>
> In patch 4, vmx_check_nested_events must clear vcpu->ex.pending.
It is true, but I can still observe the warning.
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6f88a79..4f97c4f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10990,6 +10990,8 @@ static int vmx_check_nested_events(struct
kvm_vcpu *vcpu, bool external_intr)
if (vmx->nested.nested_run_pending)
return -EBUSY;
+ vcpu->arch.exception.pending = false;
+ vcpu->arch.exception.injected = true;
nested_vmx_inject_exception_vmexit(vcpu, exit_qual);
return 0;
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b698b2f..77e3031 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6365,14 +6365,20 @@ static int inject_pending_event(struct
kvm_vcpu *vcpu, bool req_int_win)
return 0;
}
- if (vcpu->arch.nmi_injected) {
- kvm_x86_ops->set_nmi(vcpu);
- return 0;
- }
+ /*
+ * Exceptions must be injected immediately, or the exception
+ * frame will have the address of the NMI or interrupt handler.
+ */
+ if (!vcpu->arch.exception.pending) {
+ if (vcpu->arch.nmi_injected) {
+ kvm_x86_ops->set_nmi(vcpu);
+ return 0;
+ }
- if (vcpu->arch.interrupt.pending) {
- kvm_x86_ops->set_irq(vcpu);
- return 0;
+ if (vcpu->arch.interrupt.pending) {
+ kvm_x86_ops->set_irq(vcpu);
+ return 0;
+ }
}
if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) {
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web