| Message ID | 20170518230239.86518-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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!
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.
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"?
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
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
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"? >
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"? >>
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"? >>> >
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 */
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(-)