[PATCH 0/3] fix emulation error on Windows bootup

[PATCH 0/3] fix emulation error on Windows bootup

[Jan Dakinevich]

This series intended to fix (again) a bug that was a subject of the 
following change:

  6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")

Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
not set if fault happened during instruction decoding. Second, returning 
value of inject_emulated_instruction was used to make the decision to 
reenter guest, but this could happen iff on nested page fault, that is not 
the scope where this bug could occur.

However, I have still deep doubts about 3rd commit in the series. Could
you please, make me an advise if it is the correct handling of guest page 
fault?

Jan Dakinevich (3):
  KVM: x86: fix wrong return code
  KVM: x86: set ctxt->have_exception in x86_decode_insn()
  KVM: x86: always stop emulation on page fault

 arch/x86/kvm/emulate.c | 4 +++-
 arch/x86/kvm/x86.c     | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH 1/3] KVM: x86: fix wrong return code

[Jan Dakinevich]

x86_emulate_instruction(), the caller of x86_decode_insn(), expects
that x86_decode_insn()'s returning value belongs to EMULATION_* name
space. However, this function may return value from X86EMUL_* name
space.

Although, the code behaves properly (because both X86EMUL_CONTINUE and
EMULATION_OK are equal to 0) this change makes code more consistent and
it is required for further fixes.

Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/emulate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 718f7d9..6170ddf 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5144,7 +5144,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 	else {
 		rc = __do_insn_fetch_bytes(ctxt, 1);
 		if (rc != X86EMUL_CONTINUE)
-			return rc;
+			goto done;
 	}
 
 	switch (mode) {
-- 
2.1.4

Re: [PATCH 1/3] KVM: x86: fix wrong return code

[Sean Christopherson]

On Tue, Aug 27, 2019 at 01:07:04PM +0000, Jan Dakinevich wrote:
> x86_emulate_instruction(), the caller of x86_decode_insn(), expects
> that x86_decode_insn()'s returning value belongs to EMULATION_* name
> space. However, this function may return value from X86EMUL_* name
> space.
> 
> Although, the code behaves properly (because both X86EMUL_CONTINUE and
> EMULATION_OK are equal to 0) this change makes code more consistent and
> it is required for further fixes.
> 
> Cc: Denis Lunev <den@virtuozzo.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---
>  arch/x86/kvm/emulate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 718f7d9..6170ddf 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5144,7 +5144,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
>  	else {
>  		rc = __do_insn_fetch_bytes(ctxt, 1);
>  		if (rc != X86EMUL_CONTINUE)
> -			return rc;
> +			goto done;

Funny how things go unnoticed for years and then suddenly...

https://lkml.kernel.org/r/9bf79098-703c-e82b-7e7d-1c0a6a1023c2@redhat.com

>  	}
>  
>  	switch (mode) {
> -- 
> 2.1.4
> 

[PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn()

[Jan Dakinevich]

x86_emulate_instruction() takes into account ctxt->have_exception flag
during instruction decoding, but in practice this flag is never set in
x86_decode_insn().

Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/emulate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 6170ddf..f93880f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5395,6 +5395,8 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 					ctxt->memopp->addr.mem.ea + ctxt->_eip);
 
 done:
+	if (rc == X86EMUL_PROPAGATE_FAULT)
+		ctxt->have_exception = true;
 	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
 }
 
-- 
2.1.4

Re: [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn()

[Sean Christopherson]

On Tue, Aug 27, 2019 at 01:07:08PM +0000, Jan Dakinevich wrote:
> x86_emulate_instruction() takes into account ctxt->have_exception flag
> during instruction decoding, but in practice this flag is never set in
> x86_decode_insn().
> 
> Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> Cc: Denis Lunev <den@virtuozzo.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---
>  arch/x86/kvm/emulate.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 6170ddf..f93880f 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5395,6 +5395,8 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
>  					ctxt->memopp->addr.mem.ea + ctxt->_eip);
>  
>  done:
> +	if (rc == X86EMUL_PROPAGATE_FAULT)
> +		ctxt->have_exception = true;

We should add a sanity check or two on the vector since the emulator code
goes all over the place, e.g. #UD should not be injected/propagated, and
trap-like exceptions should not be handled/encountered during decode.
Note, exception_type() also warns on illegal vectors.

  WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR ||
	       exception_type(ctxt->exception.vector) == EXCPT_TRAP);

>  	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
>  }
>  
> -- 
> 2.1.4
> 

Re: [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn()

[Jan Dakinevich]

On Tue, 27 Aug 2019 07:53:58 -0700
Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> On Tue, Aug 27, 2019 at 01:07:08PM +0000, Jan Dakinevich wrote:
> > x86_emulate_instruction() takes into account ctxt->have_exception flag
> > during instruction decoding, but in practice this flag is never set in
> > x86_decode_insn().
> > 
> > Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > Cc: Denis Lunev <den@virtuozzo.com>
> > Cc: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> > ---
> >  arch/x86/kvm/emulate.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index 6170ddf..f93880f 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -5395,6 +5395,8 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
> >  					ctxt->memopp->addr.mem.ea + ctxt->_eip);
> >  
> >  done:
> > +	if (rc == X86EMUL_PROPAGATE_FAULT)
> > +		ctxt->have_exception = true;
> 
> We should add a sanity check or two on the vector since the emulator code
> goes all over the place, e.g. #UD should not be injected/propagated, and
> trap-like exceptions should not be handled/encountered during decode.
> Note, exception_type() also warns on illegal vectors.
> 
>   WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR ||
> 	       exception_type(ctxt->exception.vector) == EXCPT_TRAP);

Ok.

> 
> >  	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
> >  }
> >  
> > -- 
> > 2.1.4
> > 


-- 
Best regards
Jan Dakinevich

[PATCH 3/3] KVM: x86: always stop emulation on page fault

[Jan Dakinevich]

inject_emulated_exception() returns true if and only if nested page
fault happens. However, page fault can come from guest page tables
walk, either nested or not nested. In both cases we should stop an
attempt to read under RIP and give guest to step over its own page
fault handler.

Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/x86.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 93b0bd4..45caa69 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6521,8 +6521,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
 						emulation_type))
 				return EMULATE_DONE;
-			if (ctxt->have_exception && inject_emulated_exception(vcpu))
+			if (ctxt->have_exception) {
+				inject_emulated_exception(vcpu);
 				return EMULATE_DONE;
+			}
 			if (emulation_type & EMULTYPE_SKIP)
 				return EMULATE_FAIL;
 			return handle_emulation_failure(vcpu, emulation_type);
-- 
2.1.4

Re: [PATCH 3/3] KVM: x86: always stop emulation on page fault

[Sean Christopherson]

+Cc Peng Hao and Yi Wang

On Tue, Aug 27, 2019 at 01:07:09PM +0000, Jan Dakinevich wrote:
> inject_emulated_exception() returns true if and only if nested page
> fault happens. However, page fault can come from guest page tables
> walk, either nested or not nested. In both cases we should stop an
> attempt to read under RIP and give guest to step over its own page
> fault handler.
> 
> Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> Cc: Denis Lunev <den@virtuozzo.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---
>  arch/x86/kvm/x86.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 93b0bd4..45caa69 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6521,8 +6521,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>  			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
>  						emulation_type))
>  				return EMULATE_DONE;
> -			if (ctxt->have_exception && inject_emulated_exception(vcpu))
> +			if (ctxt->have_exception) {
> +				inject_emulated_exception(vcpu);
>  				return EMULATE_DONE;
> +			}


Yikes, this patch and the previous have quite the sordid history.


The non-void return from inject_emulated_exception() was added by commit

  ef54bcfeea6c ("KVM: x86: skip writeback on injection of nested exception")

for the purpose of skipping writeback.  At the time, the above blob in the
decode flow didn't exist.


Decode exception handling was added by commit

  6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")

but it was dead code even then.  The patch discussion[1] even point out that
it was dead code, i.e. the change probably should have been reverted.


Peng Hao and Yi Wang later ran into what appears to be the same bug you're
hitting[2][3], and even had patches temporarily queued[4][5], but the
patches never made it to mainline as they broke kvm-unit-tests.  Fun side
note, Radim even pointed out[4] the bug fixed by patch 1/3.

So, the patches look correct, but there's the open question of why the
hypercall test was failing for Paolo.  I've tried to reproduce the #DF to
no avail.

[1] https://lore.kernel.org/patchwork/patch/850077/
[2] https://lkml.kernel.org/r/1537311828-4547-1-git-send-email-penghao122@sina.com.cn
[3] https://lkml.kernel.org/r/20190111133002.GA14852@flask
[4] https://lkml.kernel.org/r/20190111133002.GA14852@flask
[5] https://lkml.kernel.org/r/9835d255-dd9a-222b-f4a2-93611175b326@redhat.com

>  			if (emulation_type & EMULTYPE_SKIP)
>  				return EMULATE_FAIL;
>  			return handle_emulation_failure(vcpu, emulation_type);
> -- 
> 2.1.4
> 

Re: [PATCH 3/3] KVM: x86: always stop emulation on page fault

[Sean Christopherson]

Actually adding Peng Hao and Yi Wang...

On Tue, Aug 27, 2019 at 07:50:30AM -0700, Sean Christopherson wrote:
> +Cc Peng Hao and Yi Wang
> 
> On Tue, Aug 27, 2019 at 01:07:09PM +0000, Jan Dakinevich wrote:
> > inject_emulated_exception() returns true if and only if nested page
> > fault happens. However, page fault can come from guest page tables
> > walk, either nested or not nested. In both cases we should stop an
> > attempt to read under RIP and give guest to step over its own page
> > fault handler.
> > 
> > Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > Cc: Denis Lunev <den@virtuozzo.com>
> > Cc: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> > ---
> >  arch/x86/kvm/x86.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 93b0bd4..45caa69 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6521,8 +6521,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> >  			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
> >  						emulation_type))
> >  				return EMULATE_DONE;
> > -			if (ctxt->have_exception && inject_emulated_exception(vcpu))
> > +			if (ctxt->have_exception) {
> > +				inject_emulated_exception(vcpu);
> >  				return EMULATE_DONE;
> > +			}
> 
> 
> Yikes, this patch and the previous have quite the sordid history.
> 
> 
> The non-void return from inject_emulated_exception() was added by commit
> 
>   ef54bcfeea6c ("KVM: x86: skip writeback on injection of nested exception")
> 
> for the purpose of skipping writeback.  At the time, the above blob in the
> decode flow didn't exist.
> 
> 
> Decode exception handling was added by commit
> 
>   6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")
> 
> but it was dead code even then.  The patch discussion[1] even point out that
> it was dead code, i.e. the change probably should have been reverted.
> 
> 
> Peng Hao and Yi Wang later ran into what appears to be the same bug you're
> hitting[2][3], and even had patches temporarily queued[4][5], but the
> patches never made it to mainline as they broke kvm-unit-tests.  Fun side
> note, Radim even pointed out[4] the bug fixed by patch 1/3.
> 
> So, the patches look correct, but there's the open question of why the
> hypercall test was failing for Paolo.  I've tried to reproduce the #DF to
> no avail.
> 
> [1] https://lore.kernel.org/patchwork/patch/850077/
> [2] https://lkml.kernel.org/r/1537311828-4547-1-git-send-email-penghao122@sina.com.cn
> [3] https://lkml.kernel.org/r/20190111133002.GA14852@flask
> [4] https://lkml.kernel.org/r/20190111133002.GA14852@flask
> [5] https://lkml.kernel.org/r/9835d255-dd9a-222b-f4a2-93611175b326@redhat.com
> 
> >  			if (emulation_type & EMULTYPE_SKIP)
> >  				return EMULATE_FAIL;
> >  			return handle_emulation_failure(vcpu, emulation_type);
> > -- 
> > 2.1.4
> > 

Re: [PATCH 3/3] KVM: x86: always stop emulation on page fault

[Jan Dakinevich]

On Tue, 27 Aug 2019 07:50:30 -0700
Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> +Cc Peng Hao and Yi Wang
> 
> On Tue, Aug 27, 2019 at 01:07:09PM +0000, Jan Dakinevich wrote:
> > inject_emulated_exception() returns true if and only if nested page
> > fault happens. However, page fault can come from guest page tables
> > walk, either nested or not nested. In both cases we should stop an
> > attempt to read under RIP and give guest to step over its own page
> > fault handler.
> > 
> > Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > Cc: Denis Lunev <den@virtuozzo.com>
> > Cc: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> > ---
> >  arch/x86/kvm/x86.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 93b0bd4..45caa69 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6521,8 +6521,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> >  			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
> >  						emulation_type))
> >  				return EMULATE_DONE;
> > -			if (ctxt->have_exception && inject_emulated_exception(vcpu))
> > +			if (ctxt->have_exception) {
> > +				inject_emulated_exception(vcpu);
> >  				return EMULATE_DONE;
> > +			}
> 
> 
> Yikes, this patch and the previous have quite the sordid history.
> 
> 
> The non-void return from inject_emulated_exception() was added by commit
> 
>   ef54bcfeea6c ("KVM: x86: skip writeback on injection of nested exception")
> 
> for the purpose of skipping writeback.  At the time, the above blob in the
> decode flow didn't exist.
> 
> 
> Decode exception handling was added by commit
> 
>   6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")
> 
> but it was dead code even then.  The patch discussion[1] even point out that
> it was dead code, i.e. the change probably should have been reverted.
> 
> 
> Peng Hao and Yi Wang later ran into what appears to be the same bug you're
> hitting[2][3], and even had patches temporarily queued[4][5], but the
> patches never made it to mainline as they broke kvm-unit-tests.  Fun side
> note, Radim even pointed out[4] the bug fixed by patch 1/3.
> 
> So, the patches look correct, but there's the open question of why the
> hypercall test was failing for Paolo.  

Sorry, I'm little confused. Could you please, point me which test or tests 
were broken? I've just run kvm-unit-test and I see same results with and 
without my changes.

> I've tried to reproduce the #DF to
> no avail.
> 
> [1] https://lore.kernel.org/patchwork/patch/850077/
> [2] https://lkml.kernel.org/r/1537311828-4547-1-git-send-email-penghao122@sina.com.cn
> [3] https://lkml.kernel.org/r/20190111133002.GA14852@flask
> [4] https://lkml.kernel.org/r/20190111133002.GA14852@flask
> [5] https://lkml.kernel.org/r/9835d255-dd9a-222b-f4a2-93611175b326@redhat.com
> 
> >  			if (emulation_type & EMULTYPE_SKIP)
> >  				return EMULATE_FAIL;
> >  			return handle_emulation_failure(vcpu, emulation_type);
> > -- 
> > 2.1.4
> > 


-- 
Best regards
Jan Dakinevich

Re: [PATCH 3/3] KVM: x86: always stop emulation on page fault

[Sean Christopherson]

On Wed, Aug 28, 2019 at 10:19:51AM +0000, Jan Dakinevich wrote:
> On Tue, 27 Aug 2019 07:50:30 -0700
> Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > Yikes, this patch and the previous have quite the sordid history.
> > 
> > 
> > The non-void return from inject_emulated_exception() was added by commit
> > 
> >   ef54bcfeea6c ("KVM: x86: skip writeback on injection of nested exception")
> > 
> > for the purpose of skipping writeback.  At the time, the above blob in the
> > decode flow didn't exist.
> > 
> > 
> > Decode exception handling was added by commit
> > 
> >   6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > 
> > but it was dead code even then.  The patch discussion[1] even point out that
> > it was dead code, i.e. the change probably should have been reverted.
> > 
> > 
> > Peng Hao and Yi Wang later ran into what appears to be the same bug you're
> > hitting[2][3], and even had patches temporarily queued[4][5], but the
> > patches never made it to mainline as they broke kvm-unit-tests.  Fun side
> > note, Radim even pointed out[4] the bug fixed by patch 1/3.
> > 
> > So, the patches look correct, but there's the open question of why the
> > hypercall test was failing for Paolo.  
> 
> Sorry, I'm little confused. Could you please, point me which test or tests 
> were broken? I've just run kvm-unit-test and I see same results with and 
> without my changes.
> 
> > I've tried to reproduce the #DF to
> > no avail.

Aha!  The #DF occurs if patch 2/3, but not patch 3/3, is applied, and the
VMware backdoor is enabled.  The backdoor is off by default, which is why
only Paolo was seeing the #DF.

To handle the VMware backdoor, KVM intercepts #GP faults, which includes
the non-canonical #GP from the hypercall unit test.  With only patch 2/3
applied, x86_emulate_instruction() injects a #GP for the non-canonical RIP
but returns EMULATE_FAIL instead of EMULATE_DONE.   EMULATE_FAIL causes
handle_exception_nmi() (or gp_interception() for SVM) to re-inject the
original #GP because it thinks emulation failed due to a non-VMware opcode.

Applying patch 3/3 resolves the issue as x86_emulate_instruction() returns
EMULATE_DONE after injecting the #GP.


TL;DR:

Swap the order of patches and everything should be hunky dory.  Please
rebase to the latest kvm/queue, which has an equivalent to patch 1/3.

Re: [PATCH 0/3] fix emulation error on Windows bootup

[Paolo Bonzini]

On 27/08/19 15:07, Jan Dakinevich wrote:
> This series intended to fix (again) a bug that was a subject of the 
> following change:
> 
>   6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> 
> Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
> not set if fault happened during instruction decoding. Second, returning 
> value of inject_emulated_instruction was used to make the decision to 
> reenter guest, but this could happen iff on nested page fault, that is not 
> the scope where this bug could occur.
> 
> However, I have still deep doubts about 3rd commit in the series. Could
> you please, make me an advise if it is the correct handling of guest page 
> fault?
> 
> Jan Dakinevich (3):
>   KVM: x86: fix wrong return code
>   KVM: x86: set ctxt->have_exception in x86_decode_insn()
>   KVM: x86: always stop emulation on page fault
> 
>  arch/x86/kvm/emulate.c | 4 +++-
>  arch/x86/kvm/x86.c     | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 

Queued, thanks.  I added the WARN_ON_ONCE that Sean suggested.

Paolo

Re: [PATCH 0/3] fix emulation error on Windows bootup

[Sean Christopherson]

On Wed, Sep 11, 2019 at 05:51:05PM +0200, Paolo Bonzini wrote:
> On 27/08/19 15:07, Jan Dakinevich wrote:
> > This series intended to fix (again) a bug that was a subject of the 
> > following change:
> > 
> >   6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > 
> > Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
> > not set if fault happened during instruction decoding. Second, returning 
> > value of inject_emulated_instruction was used to make the decision to 
> > reenter guest, but this could happen iff on nested page fault, that is not 
> > the scope where this bug could occur.
> > 
> > However, I have still deep doubts about 3rd commit in the series. Could
> > you please, make me an advise if it is the correct handling of guest page 
> > fault?
> > 
> > Jan Dakinevich (3):
> >   KVM: x86: fix wrong return code
> >   KVM: x86: set ctxt->have_exception in x86_decode_insn()
> >   KVM: x86: always stop emulation on page fault
> > 
> >  arch/x86/kvm/emulate.c | 4 +++-
> >  arch/x86/kvm/x86.c     | 4 +++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> 
> Queued, thanks.  I added the WARN_ON_ONCE that Sean suggested.

Which version did you queue?  It sounds like you queued v1, which breaks
VMware backdoor emulation due to incorrect patch ordering.  v3[*] fixes
the ordering issue and adds the WARN_ON_ONCE.

[*] https://patchwork.kernel.org/cover/11120627/

Re: [PATCH 0/3] fix emulation error on Windows bootup

[Paolo Bonzini]

On 11/09/19 21:53, Sean Christopherson wrote:
> On Wed, Sep 11, 2019 at 05:51:05PM +0200, Paolo Bonzini wrote:
>> On 27/08/19 15:07, Jan Dakinevich wrote:
>>> This series intended to fix (again) a bug that was a subject of the 
>>> following change:
>>>
>>>   6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
>>>
>>> Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
>>> not set if fault happened during instruction decoding. Second, returning 
>>> value of inject_emulated_instruction was used to make the decision to 
>>> reenter guest, but this could happen iff on nested page fault, that is not 
>>> the scope where this bug could occur.
>>>
>>> However, I have still deep doubts about 3rd commit in the series. Could
>>> you please, make me an advise if it is the correct handling of guest page 
>>> fault?
>>>
>>> Jan Dakinevich (3):
>>>   KVM: x86: fix wrong return code
>>>   KVM: x86: set ctxt->have_exception in x86_decode_insn()
>>>   KVM: x86: always stop emulation on page fault
>>>
>>>  arch/x86/kvm/emulate.c | 4 +++-
>>>  arch/x86/kvm/x86.c     | 4 +++-
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>
>> Queued, thanks.  I added the WARN_ON_ONCE that Sean suggested.
> 
> Which version did you queue?  It sounds like you queued v1, which breaks
> VMware backdoor emulation due to incorrect patch ordering.  v3[*] fixes
> the ordering issue and adds the WARN_ON_ONCE.

I applied v1 with all the fixes, then found out v3 existed and replaced
with it (but still added a comment).

Paolo