| Message ID | 20190508102715.685-3-namit@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: nVMX: Fix NMI/INTR-window tests | expand |
From: Nadav Amit <nadav.amit@gmail.com> Date: Wed, May 8, 2019 at 10:47 AM To: Paolo Bonzini Cc: <kvm@vger.kernel.org>, Nadav Amit, Jim Mattson, Sean Christopherson > From: Nadav Amit <nadav.amit@gmail.com> > > Intel SDM 26.6.5 says regarding interrupt-window exiting that: "These > events wake the logical processor if it just entered the HLT state > because of a VM entry." A similar statement is told about NMI-window > exiting. > > However, running tests which are similar to verify_nmi_window_exit() and > verify_intr_window_exit() on bare-metal suggests that real CPUs do not > wake up. Until someone figures what the correct behavior is, just reset > the activity state to "active" after each test to prevent the whole > test-suite from getting stuck. > > Cc: Jim Mattson <jmattson@google.com> > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Nadav Amit <nadav.amit@gmail.com> Reviewed-by: Jim Mattson <jmattson@google.com> I think I have been assuming that "wake the logical processor" means "causes the logical processor to enter the 'active' activity state." Maybe that's not what "wake" means?
> On May 8, 2019, at 4:21 PM, Jim Mattson <jmattson@google.com> wrote: > > From: Nadav Amit <nadav.amit@gmail.com> > Date: Wed, May 8, 2019 at 10:47 AM > To: Paolo Bonzini > Cc: <kvm@vger.kernel.org>, Nadav Amit, Jim Mattson, Sean Christopherson > >> From: Nadav Amit <nadav.amit@gmail.com> >> >> Intel SDM 26.6.5 says regarding interrupt-window exiting that: "These >> events wake the logical processor if it just entered the HLT state >> because of a VM entry." A similar statement is told about NMI-window >> exiting. >> >> However, running tests which are similar to verify_nmi_window_exit() and >> verify_intr_window_exit() on bare-metal suggests that real CPUs do not >> wake up. Until someone figures what the correct behavior is, just reset >> the activity state to "active" after each test to prevent the whole >> test-suite from getting stuck. >> >> Cc: Jim Mattson <jmattson@google.com> >> Cc: Sean Christopherson <sean.j.christopherson@intel.com> >> Signed-off-by: Nadav Amit <nadav.amit@gmail.com> > Reviewed-by: Jim Mattson <jmattson@google.com> > > I think I have been assuming that "wake the logical processor" means > "causes the logical processor to enter the 'active' activity state." > Maybe that's not what "wake" means? I really don’t know. Reading the specifications, I thought that the test is valid. I don’t manage to read it any differently than you did.
On Wed, May 08, 2019 at 03:27:15AM -0700, Nadav Amit wrote: > From: Nadav Amit <nadav.amit@gmail.com> > > Intel SDM 26.6.5 says regarding interrupt-window exiting that: "These > events wake the logical processor if it just entered the HLT state > because of a VM entry." A similar statement is told about NMI-window > exiting. > > However, running tests which are similar to verify_nmi_window_exit() and > verify_intr_window_exit() on bare-metal suggests that real CPUs do not > wake up. Until someone figures what the correct behavior is, just reset > the activity state to "active" after each test to prevent the whole > test-suite from getting stuck. > > Cc: Jim Mattson <jmattson@google.com> > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Nadav Amit <nadav.amit@gmail.com> > --- > x86/vmx_tests.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index f921286..2d6b12d 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -7063,6 +7063,7 @@ static void verify_nmi_window_exit(u64 rip) > report("Activity state (%ld) is 'ACTIVE'", > vmcs_read(GUEST_ACTV_STATE) == ACTV_ACTIVE, > vmcs_read(GUEST_ACTV_STATE)); > + vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE); Don't you need to remove (or modify) the above report() as well to avoid failing the current test? > } > > static void vmx_nmi_window_test(void) > @@ -7199,6 +7200,7 @@ static void verify_intr_window_exit(u64 rip) > report("Activity state (%ld) is 'ACTIVE'", > vmcs_read(GUEST_ACTV_STATE) == ACTV_ACTIVE, > vmcs_read(GUEST_ACTV_STATE)); > + vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE); > } > > static void vmx_intr_window_test(void) > -- > 2.17.1 >
On Wed, May 08, 2019 at 04:38:10PM -0700, Nadav Amit wrote: > > On May 8, 2019, at 4:21 PM, Jim Mattson <jmattson@google.com> wrote: > > > > From: Nadav Amit <nadav.amit@gmail.com> > > Date: Wed, May 8, 2019 at 10:47 AM > > To: Paolo Bonzini > > Cc: <kvm@vger.kernel.org>, Nadav Amit, Jim Mattson, Sean Christopherson > > > >> From: Nadav Amit <nadav.amit@gmail.com> > >> > >> Intel SDM 26.6.5 says regarding interrupt-window exiting that: "These > >> events wake the logical processor if it just entered the HLT state > >> because of a VM entry." A similar statement is told about NMI-window > >> exiting. > >> > >> However, running tests which are similar to verify_nmi_window_exit() and > >> verify_intr_window_exit() on bare-metal suggests that real CPUs do not > >> wake up. Until someone figures what the correct behavior is, just reset > >> the activity state to "active" after each test to prevent the whole > >> test-suite from getting stuck. > >> > >> Cc: Jim Mattson <jmattson@google.com> > >> Cc: Sean Christopherson <sean.j.christopherson@intel.com> > >> Signed-off-by: Nadav Amit <nadav.amit@gmail.com> > > Reviewed-by: Jim Mattson <jmattson@google.com> > > > > I think I have been assuming that "wake the logical processor" means > > "causes the logical processor to enter the 'active' activity state." > > Maybe that's not what "wake" means? > > I really don’t know. Reading the specifications, I thought that the test is > valid. I don’t manage to read it any differently than you did. "logic processor" in this context means the physical CPU, it doesn't imply anything about what gets saved into the VMCS. I assume the purpose of that blurb is to make it clear that the guest won't get stuck in HLT state if there's a virtual interrupt pending. The relevant SDM section is "Saving Non-Register State": The activity-state field is saved with the logical processor's activity state before the VM exit[1]. See Section 27.1 for details of how events leading to a VM exit may affect the activity state. The revelant bits of Section 27.1 - "Architectural State Before a VM Exit": If the logical processor is in an inactive state and not executing instructions, some events may be blocked but other may return the logical processor to the active state. Unblocked events may cause VM exits. If an unblocked event causes a VM exit directly, a return to the active state occurs only after the VM exit completes. <more irrevelant words> In other words, because the CPU was in HLT before VM-Exit, that's what gets saved into the VMCS. My guess is that the behavior is defined this way because technically the vCPU hasn't received a wake event, the VMM has simply requested a VM Exit. The wake event (from the vCPU's perspective) comes when the VMM actually injects an interrupt/NMI.
> On May 9, 2019, at 1:32 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Wed, May 08, 2019 at 03:27:15AM -0700, Nadav Amit wrote: >> From: Nadav Amit <nadav.amit@gmail.com> >> >> Intel SDM 26.6.5 says regarding interrupt-window exiting that: "These >> events wake the logical processor if it just entered the HLT state >> because of a VM entry." A similar statement is told about NMI-window >> exiting. >> >> However, running tests which are similar to verify_nmi_window_exit() and >> verify_intr_window_exit() on bare-metal suggests that real CPUs do not >> wake up. Until someone figures what the correct behavior is, just reset >> the activity state to "active" after each test to prevent the whole >> test-suite from getting stuck. >> >> Cc: Jim Mattson <jmattson@google.com> >> Cc: Sean Christopherson <sean.j.christopherson@intel.com> >> Signed-off-by: Nadav Amit <nadav.amit@gmail.com> >> --- >> x86/vmx_tests.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >> index f921286..2d6b12d 100644 >> --- a/x86/vmx_tests.c >> +++ b/x86/vmx_tests.c >> @@ -7063,6 +7063,7 @@ static void verify_nmi_window_exit(u64 rip) >> report("Activity state (%ld) is 'ACTIVE'", >> vmcs_read(GUEST_ACTV_STATE) == ACTV_ACTIVE, >> vmcs_read(GUEST_ACTV_STATE)); >> + vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE); > > Don't you need to remove (or modify) the above report() as well to avoid > failing the current test? Thanks for checking it (in your second email). So should I remove this test completely for v2? Or do you have any different test you want to run?
On Thu, May 09, 2019 at 02:29:45PM -0700, Nadav Amit wrote: > > On May 9, 2019, at 1:32 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > On Wed, May 08, 2019 at 03:27:15AM -0700, Nadav Amit wrote: > >> From: Nadav Amit <nadav.amit@gmail.com> > >> > >> Intel SDM 26.6.5 says regarding interrupt-window exiting that: "These > >> events wake the logical processor if it just entered the HLT state > >> because of a VM entry." A similar statement is told about NMI-window > >> exiting. > >> > >> However, running tests which are similar to verify_nmi_window_exit() and > >> verify_intr_window_exit() on bare-metal suggests that real CPUs do not > >> wake up. Until someone figures what the correct behavior is, just reset > >> the activity state to "active" after each test to prevent the whole > >> test-suite from getting stuck. > >> > >> Cc: Jim Mattson <jmattson@google.com> > >> Cc: Sean Christopherson <sean.j.christopherson@intel.com> > >> Signed-off-by: Nadav Amit <nadav.amit@gmail.com> > >> --- > >> x86/vmx_tests.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > >> index f921286..2d6b12d 100644 > >> --- a/x86/vmx_tests.c > >> +++ b/x86/vmx_tests.c > >> @@ -7063,6 +7063,7 @@ static void verify_nmi_window_exit(u64 rip) > >> report("Activity state (%ld) is 'ACTIVE'", > >> vmcs_read(GUEST_ACTV_STATE) == ACTV_ACTIVE, > >> vmcs_read(GUEST_ACTV_STATE)); > >> + vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE); > > > > Don't you need to remove (or modify) the above report() as well to avoid > > failing the current test? > > Thanks for checking it (in your second email). > > So should I remove this test completely for v2? Or do you have any different > test you want to run? I'd say just remove the activity state check. KVM is technically broken, and is unlikely to be fixed any time soon. I don't see much value in adding more code to the test just to highlight that KVM doesn't strictly adhere to the SDM for activity state transitions.
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index f921286..2d6b12d 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -7063,6 +7063,7 @@ static void verify_nmi_window_exit(u64 rip) report("Activity state (%ld) is 'ACTIVE'", vmcs_read(GUEST_ACTV_STATE) == ACTV_ACTIVE, vmcs_read(GUEST_ACTV_STATE)); + vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE); } static void vmx_nmi_window_test(void) @@ -7199,6 +7200,7 @@ static void verify_intr_window_exit(u64 rip) report("Activity state (%ld) is 'ACTIVE'", vmcs_read(GUEST_ACTV_STATE) == ACTV_ACTIVE, vmcs_read(GUEST_ACTV_STATE)); + vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE); } static void vmx_intr_window_test(void)