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

Commit Message

Liran Alon Nov. 6, 2017, 2:39 p.m. UTC

Signed-off-by: Liran Alon <[e-mail]>
Reviewed-by: Nikita Leshenko <[e-mail]>
Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
Reviewed-by: Paolo Bonzini <[e-mail]>
Cc: [e-mail]
---
 arch/x86/kvm/svm.c | 2 ++
 arch/x86/kvm/vmx.c | 2 ++
 2 files changed, 4 insertions(+)

Comments

Greg KH Nov. 6, 2017, 2:47 p.m. UTC | #1

On Mon, Nov 06, 2017 at 04:39:42PM +0200, Liran Alon wrote:
> Signed-off-by: Liran Alon <[e-mail]>
> Reviewed-by: Nikita Leshenko <[e-mail]>
> Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
> Reviewed-by: Paolo Bonzini <[e-mail]>
> Cc: [e-mail]
> ---
>  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

Paolo Bonzini Nov. 6, 2017, 2:50 p.m. UTC | #2

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 <[e-mail]>
>> Reviewed-by: Nikita Leshenko <[e-mail]>
>> Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
>> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
>> Reviewed-by: Paolo Bonzini <[e-mail]>
>> Cc: [e-mail]
>> ---
>>  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

Greg KH Nov. 6, 2017, 3:07 p.m. UTC | #3

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 <[e-mail]>
> >> Reviewed-by: Nikita Leshenko <[e-mail]>
> >> Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
> >> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
> >> Reviewed-by: Paolo Bonzini <[e-mail]>
> >> Cc: [e-mail]
> >> ---
> >>  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

Paolo Bonzini Nov. 6, 2017, 3:19 p.m. UTC | #4

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 <[e-mail]>
>>>> Reviewed-by: Nikita Leshenko <[e-mail]>
>>>> Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
>>>> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
>>>> Reviewed-by: Paolo Bonzini <[e-mail]>
>>>> Cc: [e-mail]
>>>> ---
>>>>  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

Wanpeng Li Nov. 7, 2017, 12:47 a.m. UTC | #5

2017-11-06 22:39 GMT+08:00 Liran Alon <[e-mail]>:
> Signed-off-by: Liran Alon <[e-mail]>
> Reviewed-by: Nikita Leshenko <[e-mail]>
> Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
> Reviewed-by: Paolo Bonzini <[e-mail]>
> Cc: [e-mail]

Except the changelog.

Reviewed-by: Wanpeng Li <[e-mail]>

> ---
>  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
>

Liran Alon Nov. 7, 2017, 8:12 a.m. UTC | #6

On 07/11/17 02:47, Wanpeng Li wrote:
> 2017-11-06 22:39 GMT+08:00 Liran Alon <[e-mail]>:
>> Signed-off-by: Liran Alon <[e-mail]>
>> Reviewed-by: Nikita Leshenko <[e-mail]>
>> Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
>> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
>> Reviewed-by: Paolo Bonzini <[e-mail]>
>> Cc: [e-mail]
>
> 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 <[e-mail]>
>
>> ---
>>   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
>>

Paolo Bonzini Nov. 7, 2017, 12:22 p.m. UTC | #7

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 <[e-mail]>:
>>> Signed-off-by: Liran Alon <[e-mail]>
>>> Reviewed-by: Nikita Leshenko <[e-mail]>
>>> Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
>>> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
>>> Reviewed-by: Paolo Bonzini <[e-mail]>
>>> Cc: [e-mail]
>>
>> 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 <[e-mail]>
>>
>>> ---
>>>   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
>>>

Liran Alon Nov. 7, 2017, 2:13 p.m. UTC | #8

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 <[e-mail]>:
>>>> Signed-off-by: Liran Alon <[e-mail]>
>>>> Reviewed-by: Nikita Leshenko <[e-mail]>
>>>> Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
>>>> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
>>>> Reviewed-by: Paolo Bonzini <[e-mail]>
>>>> Cc: [e-mail]
>>>
>>> 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 <[e-mail]>
>>>
>>>> ---
>>>>    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

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;