[PATCH] kvm: vmx: Properly handle machine check during VM-entry

[PATCH] kvm: vmx: Properly handle machine check during VM-entry

[Jim Mattson]

When bit 31 of the exit reason is set to indicate a VM-entry failure,
only the exit reason and exit qualification fields are set. All other
VM-exit information fields, including "VM-exit interruption
information," are unmodified.

Fixes: 00eba012d53e6 ("KVM: VMX: Refactor vmx_complete_atomic_exit()")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6f4ad44aa95..e73977ec15df 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8624,7 +8624,8 @@ static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
 	exit_intr_info = vmx->exit_intr_info;
 
 	/* Handle machine checks before interrupts are enabled */
-	if (is_machine_check(exit_intr_info))
+	if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY ||
+	    is_machine_check(exit_intr_info))
 		kvm_machine_check();
 
 	/* We need to handle NMIs before interrupts are enabled */
-- 
2.13.0.303.g4ebf302169-goog

Re: [PATCH] kvm: vmx: Properly handle machine check during VM-entry

[David Hildenbrand]

On 19.05.2017 01:02, Jim Mattson wrote:
> When bit 31 of the exit reason is set to indicate a VM-entry failure,
> only the exit reason and exit qualification fields are set. All other
> VM-exit information fields, including "VM-exit interruption
> information," are unmodified.
> 
> Fixes: 00eba012d53e6 ("KVM: VMX: Refactor vmx_complete_atomic_exit()")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/vmx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c6f4ad44aa95..e73977ec15df 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8624,7 +8624,8 @@ static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
>  	exit_intr_info = vmx->exit_intr_info;
>  
>  	/* Handle machine checks before interrupts are enabled */
> -	if (is_machine_check(exit_intr_info))
> +	if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY ||
> +	    is_machine_check(exit_intr_info))
>  		kvm_machine_check();
>  
>  	/* We need to handle NMIs before interrupts are enabled */
> 

This looks sane to me!

-- 

Thanks,

David

Re: [PATCH] kvm: vmx: Properly handle machine check during VM-entry

[Radim Krčmář]

2017-05-18 16:02-0700, Jim Mattson:
> When bit 31 of the exit reason is set to indicate a VM-entry failure,
> only the exit reason and exit qualification fields are set. All other
> VM-exit information fields, including "VM-exit interruption
> information," are unmodified.
> 
> Fixes: 00eba012d53e6 ("KVM: VMX: Refactor vmx_complete_atomic_exit()")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/vmx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c6f4ad44aa95..e73977ec15df 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8624,7 +8624,8 @@ static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
>  	exit_intr_info = vmx->exit_intr_info;
>  
>  	/* Handle machine checks before interrupts are enabled */
> -	if (is_machine_check(exit_intr_info))
> +	if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY ||
> +	    is_machine_check(exit_intr_info))
>  		kvm_machine_check();

Don't we need a 'return;' afterwards?
(i.e. will kvm_machine_check() always kill us?)

>  	/* We need to handle NMIs before interrupts are enabled */

If kvm_machine_check() managed to return, then we could double-inject
NMI, because exit_intr_info was not updated.

>	if (is_nmi(exit_intr_info)) {

Thanks.

[PATCH v2] kvm: vmx: Properly handle machine check during VM-entry

[Jim Mattson]

When bit 31 of the exit reason is set to indicate a VM-entry failure,
only the exit reason and exit qualification fields are set. All other
VM-exit information fields, including "VM-exit interruption
information," are unmodified.

Fixes: 00eba012d53e6 ("KVM: VMX: Refactor vmx_complete_atomic_exit()")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6f4ad44aa95..7c7ebefa8d15 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8614,17 +8614,19 @@ static void vmx_apicv_post_state_restore(struct kvm_vcpu *vcpu)
 
 static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
 {
-	u32 exit_intr_info;
+	u32 exit_intr_info = 0;
 
 	if (!(vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY
 	      || vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI))
 		return;
 
-	vmx->exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
-	exit_intr_info = vmx->exit_intr_info;
+	if (vmx->exit_reason != EXIT_REASON_MCE_DURING_VMENTRY)
+		exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+	vmx->exit_intr_info = exit_intr_info;
 
 	/* Handle machine checks before interrupts are enabled */
-	if (is_machine_check(exit_intr_info))
+	if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY ||
+	    is_machine_check(exit_intr_info))
 		kvm_machine_check();
 
 	/* We need to handle NMIs before interrupts are enabled */
-- 
2.13.0.303.g4ebf302169-goog

Re: [PATCH] kvm: vmx: Properly handle machine check during VM-entry

[Xiao Guangrong]

On 05/19/2017 07:02 AM, Jim Mattson wrote:
> When bit 31 of the exit reason is set to indicate a VM-entry failure,
> only the exit reason and exit qualification fields are set. All other
> VM-exit information fields, including "VM-exit interruption
> information," are unmodified.

This log does not reflects what it is doing. The case that
exit-reason.bit31 = 1 is skipped by the check at the very beginning of
vmx_complete_atomic_exit().

Maybe what you want to say is just "exit_intr_info is not valid if vmx
exit is caused by EXIT_REASON_MCE_DURING_VMENTRY"?

Re: [PATCH] kvm: vmx: Properly handle machine check during VM-entry

[Wanpeng Li]

2017-05-22 16:26 GMT+08:00 Xiao Guangrong <guangrong.xiao@gmail.com>:
>
>
> On 05/19/2017 07:02 AM, Jim Mattson wrote:
>>
>> When bit 31 of the exit reason is set to indicate a VM-entry failure,
>> only the exit reason and exit qualification fields are set. All other
>> VM-exit information fields, including "VM-exit interruption
>> information," are unmodified.
>
>
> This log does not reflects what it is doing. The case that
> exit-reason.bit31 = 1 is skipped by the check at the very beginning of
> vmx_complete_atomic_exit().
>
> Maybe what you want to say is just "exit_intr_info is not valid if vmx
> exit is caused by EXIT_REASON_MCE_DURING_VMENTRY"?
>

exit_intr_info is not valid for all the vmentry fail. So mce should be
captured by exit reason instead of interpreting exit_intr_info.

Regards,
Wanpeng Li

Re: [PATCH] kvm: vmx: Properly handle machine check during VM-entry

[Jim Mattson]

I don't understand the confusion here. Is it because I used Intel's
name for the VMCS field rather than the local variable in this
function? Is it "unmodified" versus "invalid"?

In any case, feel free to use whatever verbiage you are comfortable with.

On Mon, May 22, 2017 at 2:06 AM, Wanpeng Li <kernellwp@gmail.com> wrote:
> 2017-05-22 16:26 GMT+08:00 Xiao Guangrong <guangrong.xiao@gmail.com>:
>>
>>
>> On 05/19/2017 07:02 AM, Jim Mattson wrote:
>>>
>>> When bit 31 of the exit reason is set to indicate a VM-entry failure,
>>> only the exit reason and exit qualification fields are set. All other
>>> VM-exit information fields, including "VM-exit interruption
>>> information," are unmodified.
>>
>>
>> This log does not reflects what it is doing. The case that
>> exit-reason.bit31 = 1 is skipped by the check at the very beginning of
>> vmx_complete_atomic_exit().
>>
>> Maybe what you want to say is just "exit_intr_info is not valid if vmx
>> exit is caused by EXIT_REASON_MCE_DURING_VMENTRY"?
>>
>
> exit_intr_info is not valid for all the vmentry fail. So mce should be
> captured by exit reason instead of interpreting exit_intr_info.
>
> Regards,
> Wanpeng Li

Re: [PATCH] kvm: vmx: Properly handle machine check during VM-entry

[Jim Mattson]

Ah. I had assumed that vmx->exit_reason recorded ony the basic exit
reason, since all of the EXIT_REASON macros have bit 31 stripped. The
check at the very beginning of vmx_complete_atomic_exit is therefore
wrong, since EXIT_REASON_MCE_DURING_VMENTRY (41) will never be seen in
the exit reason field of the VMCS. If a machine check occurs during
VM-entry, the exit reason is 0x80000029, not 41.

On Mon, May 22, 2017 at 1:26 AM, Xiao Guangrong
<guangrong.xiao@gmail.com> wrote:
>
>
> On 05/19/2017 07:02 AM, Jim Mattson wrote:
>>
>> When bit 31 of the exit reason is set to indicate a VM-entry failure,
>> only the exit reason and exit qualification fields are set. All other
>> VM-exit information fields, including "VM-exit interruption
>> information," are unmodified.
>
>
> This log does not reflects what it is doing. The case that
> exit-reason.bit31 = 1 is skipped by the check at the very beginning of
> vmx_complete_atomic_exit().
>
> Maybe what you want to say is just "exit_intr_info is not valid if vmx
> exit is caused by EXIT_REASON_MCE_DURING_VMENTRY"?
>

[PATCH v3] kvm: vmx: Properly handle machine check during VM-entry

[Jim Mattson]

vmx_complete_atomic_exit should call kvm_machine_check for any
VM-entry failure due to a machine-check event. Such an exit should be
recognized solely by its basic exit reason (i.e. the low 16 bits of
the VMCS exit reason field). None of the other VMCS exit information
fields contain valid information when the VM-exit is due to "VM-entry
failure due to machine-check event".

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6f4ad44aa95..1aa89693ae80 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8614,17 +8614,20 @@ static void vmx_apicv_post_state_restore(struct kvm_vcpu *vcpu)
 
 static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
 {
-	u32 exit_intr_info;
+	u32 exit_intr_info = 0;
+	u16 basic_exit_reason = (u16)vmx->exit_reason;
 
-	if (!(vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY
-	      || vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI))
+	if (!(basic_exit_reason == EXIT_REASON_MCE_DURING_VMENTRY
+	      || basic_exit_reason == EXIT_REASON_EXCEPTION_NMI))
 		return;
 
-	vmx->exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
-	exit_intr_info = vmx->exit_intr_info;
+	if (basic_exit_reason != EXIT_REASON_MCE_DURING_VMENTRY)
+		exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+	vmx->exit_intr_info = exit_intr_info;
 
 	/* Handle machine checks before interrupts are enabled */
-	if (is_machine_check(exit_intr_info))
+	if (basic_exit_reason == EXIT_REASON_MCE_DURING_VMENTRY ||
+	    is_machine_check(exit_intr_info))
 		kvm_machine_check();
 
 	/* We need to handle NMIs before interrupts are enabled */
-- 
2.13.0.303.g4ebf302169-goog

Re: [PATCH] kvm: vmx: Properly handle machine check during VM-entry

[Xiao Guangrong]

Right, this is exactly what i wanted to say. :)

On 05/23/2017 12:04 AM, Jim Mattson wrote:
> Ah. I had assumed that vmx->exit_reason recorded ony the basic exit
> reason, since all of the EXIT_REASON macros have bit 31 stripped. The
> check at the very beginning of vmx_complete_atomic_exit is therefore
> wrong, since EXIT_REASON_MCE_DURING_VMENTRY (41) will never be seen in
> the exit reason field of the VMCS. If a machine check occurs during
> VM-entry, the exit reason is 0x80000029, not 41.
> 
> On Mon, May 22, 2017 at 1:26 AM, Xiao Guangrong
> <guangrong.xiao@gmail.com> wrote:
>>
>>
>> On 05/19/2017 07:02 AM, Jim Mattson wrote:
>>>
>>> When bit 31 of the exit reason is set to indicate a VM-entry failure,
>>> only the exit reason and exit qualification fields are set. All other
>>> VM-exit information fields, including "VM-exit interruption
>>> information," are unmodified.
>>
>>
>> This log does not reflects what it is doing. The case that
>> exit-reason.bit31 = 1 is skipped by the check at the very beginning of
>> vmx_complete_atomic_exit().
>>
>> Maybe what you want to say is just "exit_intr_info is not valid if vmx
>> exit is caused by EXIT_REASON_MCE_DURING_VMENTRY"?
>>

Re: [PATCH] kvm: vmx: Properly handle machine check during VM-entry

[Jim Mattson]

There is the added complication that *none* of the VMCS exit
information fields are valid when vmx->fail is true. However, that
oversight affects more than just this function, so I propose to
address it separately.

On Mon, May 22, 2017 at 7:20 PM, Xiao Guangrong
<guangrong.xiao@gmail.com> wrote:
>
> Right, this is exactly what i wanted to say. :)
>
>
> On 05/23/2017 12:04 AM, Jim Mattson wrote:
>>
>> Ah. I had assumed that vmx->exit_reason recorded ony the basic exit
>> reason, since all of the EXIT_REASON macros have bit 31 stripped. The
>> check at the very beginning of vmx_complete_atomic_exit is therefore
>> wrong, since EXIT_REASON_MCE_DURING_VMENTRY (41) will never be seen in
>> the exit reason field of the VMCS. If a machine check occurs during
>> VM-entry, the exit reason is 0x80000029, not 41.
>>
>> On Mon, May 22, 2017 at 1:26 AM, Xiao Guangrong
>> <guangrong.xiao@gmail.com> wrote:
>>>
>>>
>>>
>>> On 05/19/2017 07:02 AM, Jim Mattson wrote:
>>>>
>>>>
>>>> When bit 31 of the exit reason is set to indicate a VM-entry failure,
>>>> only the exit reason and exit qualification fields are set. All other
>>>> VM-exit information fields, including "VM-exit interruption
>>>> information," are unmodified.
>>>
>>>
>>>
>>> This log does not reflects what it is doing. The case that
>>> exit-reason.bit31 = 1 is skipped by the check at the very beginning of
>>> vmx_complete_atomic_exit().
>>>
>>> Maybe what you want to say is just "exit_intr_info is not valid if vmx
>>> exit is caused by EXIT_REASON_MCE_DURING_VMENTRY"?
>>>
>