| Message ID | 20210618113118.70621-1-laramglazier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg | expand |
On 6/18/21 4:31 AM, Lara Lazier wrote: > Updated cr4 so that cr4 and vmcb->save.cr4 are the same > and the report statement prints out the correct cr4. > Moved it to the correct test. > > Signed-off-by: Lara Lazier <laramglazier@gmail.com> > --- > x86/svm_tests.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/x86/svm_tests.c b/x86/svm_tests.c > index 8387bea..080a1a8 100644 > --- a/x86/svm_tests.c > +++ b/x86/svm_tests.c > @@ -2252,7 +2252,6 @@ static void test_efer(void) > /* > * EFER.LME and CR0.PG are both set and CR0.PE is zero. > */ > - vmcb->save.cr4 = cr4_saved | X86_CR4_PAE; This test requires CR4.PAE to be set. The preceding test required it to be unset. Did I miss something ? > cr0 &= ~X86_CR0_PE; > vmcb->save.cr0 = cr0; > report(svm_vmrun() == SVM_EXIT_ERR, "EFER.LME=1 (%lx), " > @@ -2266,6 +2265,8 @@ static void test_efer(void) > > cr0 |= X86_CR0_PE; > vmcb->save.cr0 = cr0; > + cr4 = cr4_saved | X86_CR4_PAE; > + vmcb->save.cr4 = cr4; > cs_attrib = cs_attrib_saved | SVM_SELECTOR_L_MASK | > SVM_SELECTOR_DB_MASK; > vmcb->save.cs.attrib = cs_attrib;
Am Fr., 18. Juni 2021 um 20:28 Uhr schrieb Krish Sadhukhan <krish.sadhukhan@oracle.com>: > > > On 6/18/21 4:31 AM, Lara Lazier wrote: > > Updated cr4 so that cr4 and vmcb->save.cr4 are the same > > and the report statement prints out the correct cr4. > > Moved it to the correct test. > > > > Signed-off-by: Lara Lazier <laramglazier@gmail.com> > > --- > > x86/svm_tests.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/x86/svm_tests.c b/x86/svm_tests.c > > index 8387bea..080a1a8 100644 > > --- a/x86/svm_tests.c > > +++ b/x86/svm_tests.c > > @@ -2252,7 +2252,6 @@ static void test_efer(void) > > /* > > * EFER.LME and CR0.PG are both set and CR0.PE is zero. > > */ > > - vmcb->save.cr4 = cr4_saved | X86_CR4_PAE; > > > This test requires CR4.PAE to be set. The preceding test required it to > be unset. > > Did I miss something ? Hey :) My understanding is as follows: The "first" test should succeed with an SVM_EXIT_ERR when EFER.LME and CR0.PG are both non-zero and CR0.PE is zero (so I believe we do not really care whether CR4.PAE is set or not though I might be overlooking something here). The "second" test checks if we also get an SVM_EXIT_ERR when EFER.LME, CR0.PG, CR4.PAE, CS.L, and CS.D are all non-zero. While I did not change any test internally, the thing that I changed is to split the assignment vmcb->save.cr4 = cr4_saved | X86_CR4_PAE; up into cr4 = cr4_saved | X86_CR4_PAE; vmcb->save.cr4 = cr4; so that we print out the correct value in report(svm_vmrun() == SVM_EXIT_ERR, "EFER.LME=1 (%lx), CR0.PG=1 (%lx), CR4.PAE=1 (%lx), CS.L=1 and CS.D=1 (%x)",efer, cr0, cr4, cs_attrib); Before we printed out cr4.PAE=1 (0) as the local variable cr4 was not updated and did not correctly reflect the state of vmcb->save.cr4, which was a bit confusing. > > > > cr0 &= ~X86_CR0_PE; > > vmcb->save.cr0 = cr0; > > report(svm_vmrun() == SVM_EXIT_ERR, "EFER.LME=1 (%lx), " > > @@ -2266,6 +2265,8@@ static void test_efer(void) > > > > > cr0 |= X86_CR0_PE; > > vmcb->save.cr0 = cr0; > > + cr4 = cr4_saved | X86_CR4_PAE; > > + vmcb->save.cr4 = cr4; > > cs_attrib = cs_attrib_saved | SVM_SELECTOR_L_MASK | > > SVM_SELECTOR_DB_MASK; > > vmcb->save.cs.attrib = cs_attrib;
On Fri, Jun 18, 2021 at 12:59 PM Lara Lazier <laramglazier@gmail.com> wrote: > My understanding is as follows: > The "first" test should succeed with an SVM_EXIT_ERR when EFER.LME and > CR0.PG are both > non-zero and CR0.PE is zero (so I believe we do not really care > whether CR4.PAE is set or not though > I might be overlooking something here). You are overlooking the fact that the test will fail if CR4.PAE is clear. If CR4.PAE is 0 *and* CR0.PE is 0, then you can't be sure which one triggered the failure.
Am Fr., 18. Juni 2021 um 22:26 Uhr schrieb Jim Mattson <jmattson@google.com>: > > On Fri, Jun 18, 2021 at 12:59 PM Lara Lazier <laramglazier@gmail.com> wrote: > > > My understanding is as follows: > > The "first" test should succeed with an SVM_EXIT_ERR when EFER.LME and > > CR0.PG are both > > non-zero and CR0.PE is zero (so I believe we do not really care > > whether CR4.PAE is set or not though > > I might be overlooking something here). > > You are overlooking the fact that the test will fail if CR4.PAE is > clear. If CR4.PAE is 0 *and* CR0.PE is 0, then you can't be sure which > one triggered the failure. Oh, yes that makes sense! Thank you for the explanation. I will move it back up.
On Fri, Jun 18, 2021 at 1:57 PM Lara Lazier <laramglazier@gmail.com> wrote: > > Am Fr., 18. Juni 2021 um 22:26 Uhr schrieb Jim Mattson <jmattson@google.com>: > > > > On Fri, Jun 18, 2021 at 12:59 PM Lara Lazier <laramglazier@gmail.com> wrote: > > > > > My understanding is as follows: > > > The "first" test should succeed with an SVM_EXIT_ERR when EFER.LME and > > > CR0.PG are both > > > non-zero and CR0.PE is zero (so I believe we do not really care > > > whether CR4.PAE is set or not though > > > I might be overlooking something here). > > > > You are overlooking the fact that the test will fail if CR4.PAE is > > clear. If CR4.PAE is 0 *and* CR0.PE is 0, then you can't be sure which > > one triggered the failure. > Oh, yes that makes sense! Thank you for the explanation. > I will move it back up. I think this may be subtle enough to warrant a comment as well, if you're so inclined.
Am Fr., 18. Juni 2021 um 23:10 Uhr schrieb Jim Mattson <jmattson@google.com>: > > On Fri, Jun 18, 2021 at 1:57 PM Lara Lazier <laramglazier@gmail.com> wrote: > > > > Am Fr., 18. Juni 2021 um 22:26 Uhr schrieb Jim Mattson <jmattson@google.com>: > > > > > > On Fri, Jun 18, 2021 at 12:59 PM Lara Lazier <laramglazier@gmail.com> wrote: > > > > > > > My understanding is as follows: > > > > The "first" test should succeed with an SVM_EXIT_ERR when EFER.LME and > > > > CR0.PG are both > > > > non-zero and CR0.PE is zero (so I believe we do not really care > > > > whether CR4.PAE is set or not though > > > > I might be overlooking something here). > > > > > > You are overlooking the fact that the test will fail if CR4.PAE is > > > clear. If CR4.PAE is 0 *and* CR0.PE is 0, then you can't be sure which > > > one triggered the failure. > > Oh, yes that makes sense! Thank you for the explanation. > > I will move it back up. > > I think this may be subtle enough to warrant a comment as well, if > you're so inclined. Sure, I will add a corresponding comment.
diff --git a/x86/svm_tests.c b/x86/svm_tests.c index 8387bea..080a1a8 100644 --- a/x86/svm_tests.c +++ b/x86/svm_tests.c @@ -2252,7 +2252,6 @@ static void test_efer(void) /* * EFER.LME and CR0.PG are both set and CR0.PE is zero. */ - vmcb->save.cr4 = cr4_saved | X86_CR4_PAE; cr0 &= ~X86_CR0_PE; vmcb->save.cr0 = cr0; report(svm_vmrun() == SVM_EXIT_ERR, "EFER.LME=1 (%lx), " @@ -2266,6 +2265,8 @@ static void test_efer(void) cr0 |= X86_CR0_PE; vmcb->save.cr0 = cr0; + cr4 = cr4_saved | X86_CR4_PAE; + vmcb->save.cr4 = cr4; cs_attrib = cs_attrib_saved | SVM_SELECTOR_L_MASK | SVM_SELECTOR_DB_MASK; vmcb->save.cs.attrib = cs_attrib;
Updated cr4 so that cr4 and vmcb->save.cr4 are the same and the report statement prints out the correct cr4. Moved it to the correct test. Signed-off-by: Lara Lazier <laramglazier@gmail.com> --- x86/svm_tests.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)