[PATCH v2 0/3] KVM: x86: Various emulator fixes

[PATCH v2 0/3] KVM: x86: Various emulator fixes

[Liran Alon]

First 2 patches of these series were already reviewed by Paolo.
The third patch includes a minor enhancement suggested by Paolo.

I was not sure what is the process to submit a v2 of a single patch
from the entire series so I sent the entire series again with relevant
tags added. Please correct me about the process if this is wrong.

In addition, is there any additional operation we need to do after
the patches are reviewed by Paolo?

Thanks.
-Liran Alon

[PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Liran Alon]

Signed-off-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kvm/svm.c | 2 ++
 arch/x86/kvm/vmx.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0e68f0b3cbf7..e0162b20e3c9 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2189,6 +2189,8 @@ static int ud_interception(struct vcpu_svm *svm)
 	int er;
 
 	er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
+	if (er == EMULATE_USER_EXIT)
+		return 0;
 	if (er != EMULATE_DONE)
 		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
 	return 1;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 95a01609d7ee..2b63d9edc207 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5886,6 +5886,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
 			return 1;
 		}
 		er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
+		if (er == EMULATE_USER_EXIT)
+			return 0;
 		if (er != EMULATE_DONE)
 			kvm_queue_exception(vcpu, UD_VECTOR);
 		return 1;
-- 
1.9.1

Re: [PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Greg KH]

On Mon, Nov 06, 2017 at 04:39:42PM +0200, Liran Alon wrote:
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/kvm/svm.c | 2 ++
>  arch/x86/kvm/vmx.c | 2 ++
>  2 files changed, 4 insertions(+)

No changelog text for a patch you feel is a big enough bugfix that it
needs to be backported to the stable trees?

Not good, please fix.

thanks,

greg k-h

Re: [PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Paolo Bonzini]

On 06/11/2017 15:47, Greg KH wrote:
> On Mon, Nov 06, 2017 at 04:39:42PM +0200, Liran Alon wrote:
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  arch/x86/kvm/svm.c | 2 ++
>>  arch/x86/kvm/vmx.c | 2 ++
>>  2 files changed, 4 insertions(+)
> 
> No changelog text for a patch you feel is a big enough bugfix that it
> needs to be backported to the stable trees?
> 
> Not good, please fix.

Yup, will do.  (New contributor, let's be gentle :))

Paolo

Re: [PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Greg KH]

On Mon, Nov 06, 2017 at 03:50:44PM +0100, Paolo Bonzini wrote:
> On 06/11/2017 15:47, Greg KH wrote:
> > On Mon, Nov 06, 2017 at 04:39:42PM +0200, Liran Alon wrote:
> >> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> >> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> >> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>  arch/x86/kvm/svm.c | 2 ++
> >>  arch/x86/kvm/vmx.c | 2 ++
> >>  2 files changed, 4 insertions(+)
> > 
> > No changelog text for a patch you feel is a big enough bugfix that it
> > needs to be backported to the stable trees?
> > 
> > Not good, please fix.
> 
> Yup, will do.  (New contributor, let's be gentle :))

I'm being nice, but really, someone on this reviewed-by chain should
have caught that, I blame them, not the original submitter :(

thanks,

greg k-h

Re: [PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Paolo Bonzini]

On 06/11/2017 16:07, Greg KH wrote:
> On Mon, Nov 06, 2017 at 03:50:44PM +0100, Paolo Bonzini wrote:
>> On 06/11/2017 15:47, Greg KH wrote:
>>> On Mon, Nov 06, 2017 at 04:39:42PM +0200, Liran Alon wrote:
>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>>>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>  arch/x86/kvm/svm.c | 2 ++
>>>>  arch/x86/kvm/vmx.c | 2 ++
>>>>  2 files changed, 4 insertions(+)
>>>
>>> No changelog text for a patch you feel is a big enough bugfix that it
>>> needs to be backported to the stable trees?
>>>
>>> Not good, please fix.
>>
>> Yup, will do.  (New contributor, let's be gentle :))
> 
> I'm being nice, but really, someone on this reviewed-by chain should
> have caught that, I blame them, not the original submitter :(

The Cc was noted by me in the v1, more or less as a reminder to whoever
would be applying this patch---either me or Radim---and Liran was kind
enough to copy it into v2.  He did provide more information in a cover
letter, though only in the first version of the series.

Paolo

Re: [PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Wanpeng Li]

2017-11-06 22:39 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: stable@vger.kernel.org

Except the changelog.

Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>

> ---
>  arch/x86/kvm/svm.c | 2 ++
>  arch/x86/kvm/vmx.c | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0e68f0b3cbf7..e0162b20e3c9 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2189,6 +2189,8 @@ static int ud_interception(struct vcpu_svm *svm)
>         int er;
>
>         er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
> +       if (er == EMULATE_USER_EXIT)
> +               return 0;
>         if (er != EMULATE_DONE)
>                 kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>         return 1;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 95a01609d7ee..2b63d9edc207 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5886,6 +5886,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
>                         return 1;
>                 }
>                 er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> +               if (er == EMULATE_USER_EXIT)
> +                       return 0;
>                 if (er != EMULATE_DONE)
>                         kvm_queue_exception(vcpu, UD_VECTOR);
>                 return 1;
> --
> 1.9.1
>

Re: [PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Liran Alon]

On 07/11/17 02:47, Wanpeng Li wrote:
> 2017-11-06 22:39 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: stable@vger.kernel.org
>
> Except the changelog.
Thanks for the review.
Currently both you and Paolo added "Reviewed-by" to this commit.

Is there anything else you wish me to add to the commit message before 
this commit being accepted? Do you have a suggestion? I though the 
commit-title explains it enough for this trivial patch and didn't saw 
any complain about not having body by ./scripts/checkpatch.pl.

In addition, if I would need to edit commit message body, should I send 
the next version of this commit as a standalone or re-send the entire 
series?

Thanks,
-Liran

>
> Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>
>
>> ---
>>   arch/x86/kvm/svm.c | 2 ++
>>   arch/x86/kvm/vmx.c | 2 ++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 0e68f0b3cbf7..e0162b20e3c9 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2189,6 +2189,8 @@ static int ud_interception(struct vcpu_svm *svm)
>>          int er;
>>
>>          er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
>> +       if (er == EMULATE_USER_EXIT)
>> +               return 0;
>>          if (er != EMULATE_DONE)
>>                  kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>>          return 1;
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 95a01609d7ee..2b63d9edc207 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -5886,6 +5886,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
>>                          return 1;
>>                  }
>>                  er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>> +               if (er == EMULATE_USER_EXIT)
>> +                       return 0;
>>                  if (er != EMULATE_DONE)
>>                          kvm_queue_exception(vcpu, UD_VECTOR);
>>                  return 1;
>> --
>> 1.9.1
>>

Re: [PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Paolo Bonzini]

On 07/11/2017 09:12, Liran Alon wrote:
> 
> 
> On 07/11/17 02:47, Wanpeng Li wrote:
>> 2017-11-06 22:39 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: stable@vger.kernel.org
>>
>> Except the changelog.
> Thanks for the review.
> Currently both you and Paolo added "Reviewed-by" to this commit.
> 
> Is there anything else you wish me to add to the commit message before
> this commit being accepted? Do you have a suggestion? I though the
> commit-title explains it enough for this trivial patch and didn't saw
> any complain about not having body by ./scripts/checkpatch.pl.
> 
> In addition, if I would need to edit commit message body, should I send
> the next version of this commit as a standalone or re-send the entire
> series?

No, don't worry.  Generally, when a maintainer adds a Reviewed-by it
means that it's just a matter of time before the patch goes in.

For the commit message, I was thinking of something like:

---
Instruction emulation after trapping a #UD exception can result in an
MMIO access, for example when emulating a MOVBE on a processor that
doesn't support the instruction.  In this case, the #UD vmexit handler
must exit to user mode, but there wasn't any code to do so.  Add it for
both VMX and SVM.
---

Sounds good?

Paolo

> Thanks,
> -Liran
> 
>>
>> Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>>> ---
>>>   arch/x86/kvm/svm.c | 2 ++
>>>   arch/x86/kvm/vmx.c | 2 ++
>>>   2 files changed, 4 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index 0e68f0b3cbf7..e0162b20e3c9 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -2189,6 +2189,8 @@ static int ud_interception(struct vcpu_svm *svm)
>>>          int er;
>>>
>>>          er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
>>> +       if (er == EMULATE_USER_EXIT)
>>> +               return 0;
>>>          if (er != EMULATE_DONE)
>>>                  kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>>>          return 1;
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 95a01609d7ee..2b63d9edc207 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -5886,6 +5886,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
>>>                          return 1;
>>>                  }
>>>                  er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>>> +               if (er == EMULATE_USER_EXIT)
>>> +                       return 0;
>>>                  if (er != EMULATE_DONE)
>>>                          kvm_queue_exception(vcpu, UD_VECTOR);
>>>                  return 1;
>>> -- 
>>> 1.9.1
>>>

Re: [PATCH v2 1/3] KVM: x86: Exit to user-mode on #UD intercept when emulator requires

[Liran Alon]

On 07/11/17 14:22, Paolo Bonzini wrote:
> On 07/11/2017 09:12, Liran Alon wrote:
>>
>>
>> On 07/11/17 02:47, Wanpeng Li wrote:
>>> 2017-11-06 22:39 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>>>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: stable@vger.kernel.org
>>>
>>> Except the changelog.
>> Thanks for the review.
>> Currently both you and Paolo added "Reviewed-by" to this commit.
>>
>> Is there anything else you wish me to add to the commit message before
>> this commit being accepted? Do you have a suggestion? I though the
>> commit-title explains it enough for this trivial patch and didn't saw
>> any complain about not having body by ./scripts/checkpatch.pl.
>>
>> In addition, if I would need to edit commit message body, should I send
>> the next version of this commit as a standalone or re-send the entire
>> series?
>
> No, don't worry.  Generally, when a maintainer adds a Reviewed-by it
> means that it's just a matter of time before the patch goes in.
>
> For the commit message, I was thinking of something like:
>
> ---
> Instruction emulation after trapping a #UD exception can result in an
> MMIO access, for example when emulating a MOVBE on a processor that
> doesn't support the instruction.  In this case, the #UD vmexit handler
> must exit to user mode, but there wasn't any code to do so.  Add it for
> both VMX and SVM.
> ---
>
> Sounds good?
Sounds good. Thanks.
So if I understood correctly, I leave it to you to insert the patch with 
this commit message when it is inserted.

Thanks. :)
>
> Paolo
>
>> Thanks,
>> -Liran
>>
>>>
>>> Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>>
>>>> ---
>>>>    arch/x86/kvm/svm.c | 2 ++
>>>>    arch/x86/kvm/vmx.c | 2 ++
>>>>    2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>>> index 0e68f0b3cbf7..e0162b20e3c9 100644
>>>> --- a/arch/x86/kvm/svm.c
>>>> +++ b/arch/x86/kvm/svm.c
>>>> @@ -2189,6 +2189,8 @@ static int ud_interception(struct vcpu_svm *svm)
>>>>           int er;
>>>>
>>>>           er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
>>>> +       if (er == EMULATE_USER_EXIT)
>>>> +               return 0;
>>>>           if (er != EMULATE_DONE)
>>>>                   kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>>>>           return 1;
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index 95a01609d7ee..2b63d9edc207 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -5886,6 +5886,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
>>>>                           return 1;
>>>>                   }
>>>>                   er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>>>> +               if (er == EMULATE_USER_EXIT)
>>>> +                       return 0;
>>>>                   if (er != EMULATE_DONE)
>>>>                           kvm_queue_exception(vcpu, UD_VECTOR);
>>>>                   return 1;
>>>> --
>>>> 1.9.1
>>>>
>

[PATCH v2 2/3] KVM: x86: emulator: Return to user-mode on L1 CPL=0 emulation failure

[Liran Alon]

On this case, handle_emulation_failure() fills kvm_run with
internal-error information which it expects to be delivered
to user-mode for further processing.
However, the code reports a wrong return-value which makes KVM to never
return to user-mode on this scenario.

Fixes: 6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to
userspace")

Signed-off-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 03869eb7fcd6..f4edb4baf441 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5413,7 +5413,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
 		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
 		vcpu->run->internal.ndata = 0;
-		r = EMULATE_FAIL;
+		r = EMULATE_USER_EXIT;
 	}
 	kvm_queue_exception(vcpu, UD_VECTOR);
 
-- 
1.9.1

Re: [PATCH v2 2/3] KVM: x86: emulator: Return to user-mode on L1 CPL=0 emulation failure

[Wanpeng Li]

2017-11-06 22:39 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
> On this case, handle_emulation_failure() fills kvm_run with
> internal-error information which it expects to be delivered
> to user-mode for further processing.
> However, the code reports a wrong return-value which makes KVM to never
> return to user-mode on this scenario.
>
> Fixes: 6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to
> userspace")
>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>

> ---
>  arch/x86/kvm/x86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 03869eb7fcd6..f4edb4baf441 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5413,7 +5413,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
>                 vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>                 vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>                 vcpu->run->internal.ndata = 0;
> -               r = EMULATE_FAIL;
> +               r = EMULATE_USER_EXIT;
>         }
>         kvm_queue_exception(vcpu, UD_VECTOR);
>
> --
> 1.9.1
>

[PATCH v2 3/3] KVM: x86: Don't re-execute instruction when not passing CR2 value

[Liran Alon]

In case of instruction-decode failure or emulation failure,
x86_emulate_instruction() will call reexecute_instruction() which will
attempt to use the cr2 value passed to x86_emulate_instruction().
However, when x86_emulate_instruction() is called from
emulate_instruction(), cr2 is not passed (passed as 0) and therefore
it doesn't make sense to execute reexecute_instruction() logic at all.

Fixes: 51d8b66199e9 ("KVM: cleanup emulate_instruction")

Signed-off-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/kvm_host.h | 3 ++-
 arch/x86/kvm/vmx.c              | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c73e493adf07..bc1347949cef 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1156,7 +1156,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, unsigned long cr2,
 static inline int emulate_instruction(struct kvm_vcpu *vcpu,
 			int emulation_type)
 {
-	return x86_emulate_instruction(vcpu, 0, emulation_type, NULL, 0);
+	return x86_emulate_instruction(vcpu, 0,
+			emulation_type | EMULTYPE_NO_REEXECUTE, NULL, 0);
 }
 
 void kvm_enable_efer_bits(u64);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 2b63d9edc207..258d1843f0cb 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6569,7 +6569,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 		if (kvm_test_request(KVM_REQ_EVENT, vcpu))
 			return 1;
 
-		err = emulate_instruction(vcpu, EMULTYPE_NO_REEXECUTE);
+		err = emulate_instruction(vcpu, 0);
 
 		if (err == EMULATE_USER_EXIT) {
 			++vcpu->stat.mmio_exits;
-- 
1.9.1

Re: [PATCH v2 3/3] KVM: x86: Don't re-execute instruction when not passing CR2 value

[Wanpeng Li]

2017-11-06 22:39 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
> In case of instruction-decode failure or emulation failure,
> x86_emulate_instruction() will call reexecute_instruction() which will
> attempt to use the cr2 value passed to x86_emulate_instruction().
> However, when x86_emulate_instruction() is called from
> emulate_instruction(), cr2 is not passed (passed as 0) and therefore
> it doesn't make sense to execute reexecute_instruction() logic at all.
>
> Fixes: 51d8b66199e9 ("KVM: cleanup emulate_instruction")
>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>

> ---
>  arch/x86/include/asm/kvm_host.h | 3 ++-
>  arch/x86/kvm/vmx.c              | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c73e493adf07..bc1347949cef 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1156,7 +1156,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, unsigned long cr2,
>  static inline int emulate_instruction(struct kvm_vcpu *vcpu,
>                         int emulation_type)
>  {
> -       return x86_emulate_instruction(vcpu, 0, emulation_type, NULL, 0);
> +       return x86_emulate_instruction(vcpu, 0,
> +                       emulation_type | EMULTYPE_NO_REEXECUTE, NULL, 0);
>  }
>
>  void kvm_enable_efer_bits(u64);
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 2b63d9edc207..258d1843f0cb 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6569,7 +6569,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
>                 if (kvm_test_request(KVM_REQ_EVENT, vcpu))
>                         return 1;
>
> -               err = emulate_instruction(vcpu, EMULTYPE_NO_REEXECUTE);
> +               err = emulate_instruction(vcpu, 0);
>
>                 if (err == EMULATE_USER_EXIT) {
>                         ++vcpu->stat.mmio_exits;
> --
> 1.9.1
>

Re: [PATCH v2 0/3] KVM: x86: Various emulator fixes

[Paolo Bonzini]

On 06/11/2017 15:39, Liran Alon wrote:
> First 2 patches of these series were already reviewed by Paolo.
> The third patch includes a minor enhancement suggested by Paolo.
> 
> I was not sure what is the process to submit a v2 of a single patch
> from the entire series so I sent the entire series again with relevant
> tags added. Please correct me about the process if this is wrong.

It's fine.

> In addition, is there any additional operation we need to do after
> the patches are reviewed by Paolo?

No, either I or Radim are going to apply the patches.  Right now, Radim
"owns" the kvm tree so I just provided my review instead of queuing them
myself for the next Linux merge window.

(There's no stupid question).

Thanks!

Paolo

Re: [PATCH v2 0/3] KVM: x86: Various emulator fixes

[Radim Krčmář]

2017-11-06 16:39+0200, Liran Alon:
> First 2 patches of these series were already reviewed by Paolo.
> The third patch includes a minor enhancement suggested by Paolo.
> 
> I was not sure what is the process to submit a v2 of a single patch
> from the entire series so I sent the entire series again with relevant
> tags added. Please correct me about the process if this is wrong.
> 
> In addition, is there any additional operation we need to do after
> the patches are reviewed by Paolo?

Added Paolo's commit message to [1/3] and applied all, thanks.