[PATCH kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg

[PATCH kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg

[Lara Lazier]

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

Re: [PATCH kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg

[Krish Sadhukhan]

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;

Re: [PATCH kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg

[Lara Lazier]

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;

Re: [PATCH kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg

[Jim Mattson]

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.

Re: [PATCH kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg

[Lara Lazier]

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.

Re: [PATCH kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg

[Jim Mattson]

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.

Re: [PATCH kvm-unit-tests] svm: Updated cr4 in test_efer to fix report msg

[Lara Lazier]

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.