| Message ID | 20190625120322.8483-1-nadav.amit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: Remove assumptions on CR4.MCE | expand |
On 6/25/19 5:03 AM, Nadav Amit wrote: > CR4.MCE might be set after boot. Remove the assertion that checks that > it is clear. Change the test to toggle the bit instead of setting it. > > Cc: Marc Orr <marcorr@google.com> > Signed-off-by: Nadav Amit <nadav.amit@gmail.com> > --- > x86/vmx_tests.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index b50d858..3731757 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -7096,8 +7096,11 @@ static int write_cr4_checking(unsigned long val) > > static void vmx_cr_load_test(void) > { > + unsigned long cr3, cr4, orig_cr3, orig_cr4; > struct cpuid _cpuid = cpuid(1); > - unsigned long cr4 = read_cr4(), cr3 = read_cr3(); > + > + orig_cr4 = read_cr4(); > + orig_cr3 = read_cr3(); > > if (!(_cpuid.c & X86_FEATURE_PCID)) { > report_skip("PCID not detected"); > @@ -7108,12 +7111,11 @@ static void vmx_cr_load_test(void) > return; > } > > - TEST_ASSERT(!(cr4 & (X86_CR4_PCIDE | X86_CR4_MCE))); > - TEST_ASSERT(!(cr3 & X86_CR3_PCID_MASK)); > + TEST_ASSERT(!(orig_cr3 & X86_CR3_PCID_MASK)); > > /* Enable PCID for L1. */ > - cr4 |= X86_CR4_PCIDE; > - cr3 |= 0x1; > + cr4 = orig_cr4 | X86_CR4_PCIDE; > + cr3 = orig_cr3 | 0x1; > TEST_ASSERT(!write_cr4_checking(cr4)); > write_cr3(cr3); > > @@ -7126,17 +7128,16 @@ static void vmx_cr_load_test(void) > * No exception is expected. > * > * NB. KVM loads the last guest write to CR4 into CR4 read > - * shadow. In order to trigger an exit to KVM, we can set a > - * bit that was zero in the above CR4 write and is owned by > - * KVM. We choose to set CR4.MCE, which shall have no side > - * effect because normally no guest MCE (e.g., as the result > - * of bad memory) would happen during this test. > + * shadow. In order to trigger an exit to KVM, we can toggle a > + * bit that is owned by KVM. We choose to set CR4.MCE, which shall > + * have no side effect because normally no guest MCE (e.g., as the > + * result of bad memory) would happen during this test. > */ > - TEST_ASSERT(!write_cr4_checking(cr4 | X86_CR4_MCE)); > + TEST_ASSERT(!write_cr4_checking(cr4 ^ X86_CR4_MCE)); > > - /* Cleanup L1 state: disable PCID. */ > - write_cr3(cr3 & ~X86_CR3_PCID_MASK); > - TEST_ASSERT(!write_cr4_checking(cr4 & ~X86_CR4_PCIDE)); > + /* Cleanup L1 state. */ > + write_cr3(orig_cr3); > + TEST_ASSERT(!write_cr4_checking(orig_cr4)); > } > > static void vmx_nm_test_guest(void) Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
On Tue, Jun 25, 2019 at 12:25 PM Nadav Amit <nadav.amit@gmail.com> wrote: > > CR4.MCE might be set after boot. Remove the assertion that checks that > it is clear. Change the test to toggle the bit instead of setting it. > > Cc: Marc Orr <marcorr@google.com> > Signed-off-by: Nadav Amit <nadav.amit@gmail.com> > --- > x86/vmx_tests.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index b50d858..3731757 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -7096,8 +7096,11 @@ static int write_cr4_checking(unsigned long val) > > static void vmx_cr_load_test(void) > { > + unsigned long cr3, cr4, orig_cr3, orig_cr4; > struct cpuid _cpuid = cpuid(1); > - unsigned long cr4 = read_cr4(), cr3 = read_cr3(); > + > + orig_cr4 = read_cr4(); > + orig_cr3 = read_cr3(); > > if (!(_cpuid.c & X86_FEATURE_PCID)) { > report_skip("PCID not detected"); > @@ -7108,12 +7111,11 @@ static void vmx_cr_load_test(void) > return; > } > > - TEST_ASSERT(!(cr4 & (X86_CR4_PCIDE | X86_CR4_MCE))); > - TEST_ASSERT(!(cr3 & X86_CR3_PCID_MASK)); > + TEST_ASSERT(!(orig_cr3 & X86_CR3_PCID_MASK)); > > /* Enable PCID for L1. */ > - cr4 |= X86_CR4_PCIDE; > - cr3 |= 0x1; > + cr4 = orig_cr4 | X86_CR4_PCIDE; > + cr3 = orig_cr3 | 0x1; > TEST_ASSERT(!write_cr4_checking(cr4)); > write_cr3(cr3); > > @@ -7126,17 +7128,16 @@ static void vmx_cr_load_test(void) > * No exception is expected. > * > * NB. KVM loads the last guest write to CR4 into CR4 read > - * shadow. In order to trigger an exit to KVM, we can set a > - * bit that was zero in the above CR4 write and is owned by > - * KVM. We choose to set CR4.MCE, which shall have no side > - * effect because normally no guest MCE (e.g., as the result > - * of bad memory) would happen during this test. > + * shadow. In order to trigger an exit to KVM, we can toggle a > + * bit that is owned by KVM. We choose to set CR4.MCE, which shall "set ..." doesn't make sense, right? Maybe just delete "We choose to ... during this test.". > + * have no side effect because normally no guest MCE (e.g., as the > + * result of bad memory) would happen during this test. > */ > - TEST_ASSERT(!write_cr4_checking(cr4 | X86_CR4_MCE)); > + TEST_ASSERT(!write_cr4_checking(cr4 ^ X86_CR4_MCE)); > > - /* Cleanup L1 state: disable PCID. */ > - write_cr3(cr3 & ~X86_CR3_PCID_MASK); > - TEST_ASSERT(!write_cr4_checking(cr4 & ~X86_CR4_PCIDE)); > + /* Cleanup L1 state. */ > + write_cr3(orig_cr3); > + TEST_ASSERT(!write_cr4_checking(orig_cr4)); > } > > static void vmx_nm_test_guest(void) > -- > 2.17.1 > Reviewed-by: Marc Orr <marcorr@google.com>
> On Jun 26, 2019, at 5:35 PM, Marc Orr <marcorr@google.com> wrote: > > On Tue, Jun 25, 2019 at 12:25 PM Nadav Amit <nadav.amit@gmail.com> wrote: >> CR4.MCE might be set after boot. Remove the assertion that checks that >> it is clear. Change the test to toggle the bit instead of setting it. >> >> Cc: Marc Orr <marcorr@google.com> >> Signed-off-by: Nadav Amit <nadav.amit@gmail.com> >> --- >> x86/vmx_tests.c | 29 +++++++++++++++-------------- >> 1 file changed, 15 insertions(+), 14 deletions(-) >> >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >> index b50d858..3731757 100644 >> --- a/x86/vmx_tests.c >> +++ b/x86/vmx_tests.c >> @@ -7096,8 +7096,11 @@ static int write_cr4_checking(unsigned long val) >> >> static void vmx_cr_load_test(void) >> { >> + unsigned long cr3, cr4, orig_cr3, orig_cr4; >> struct cpuid _cpuid = cpuid(1); >> - unsigned long cr4 = read_cr4(), cr3 = read_cr3(); >> + >> + orig_cr4 = read_cr4(); >> + orig_cr3 = read_cr3(); >> >> if (!(_cpuid.c & X86_FEATURE_PCID)) { >> report_skip("PCID not detected"); >> @@ -7108,12 +7111,11 @@ static void vmx_cr_load_test(void) >> return; >> } >> >> - TEST_ASSERT(!(cr4 & (X86_CR4_PCIDE | X86_CR4_MCE))); >> - TEST_ASSERT(!(cr3 & X86_CR3_PCID_MASK)); >> + TEST_ASSERT(!(orig_cr3 & X86_CR3_PCID_MASK)); >> >> /* Enable PCID for L1. */ >> - cr4 |= X86_CR4_PCIDE; >> - cr3 |= 0x1; >> + cr4 = orig_cr4 | X86_CR4_PCIDE; >> + cr3 = orig_cr3 | 0x1; >> TEST_ASSERT(!write_cr4_checking(cr4)); >> write_cr3(cr3); >> >> @@ -7126,17 +7128,16 @@ static void vmx_cr_load_test(void) >> * No exception is expected. >> * >> * NB. KVM loads the last guest write to CR4 into CR4 read >> - * shadow. In order to trigger an exit to KVM, we can set a >> - * bit that was zero in the above CR4 write and is owned by >> - * KVM. We choose to set CR4.MCE, which shall have no side >> - * effect because normally no guest MCE (e.g., as the result >> - * of bad memory) would happen during this test. >> + * shadow. In order to trigger an exit to KVM, we can toggle a >> + * bit that is owned by KVM. We choose to set CR4.MCE, which shall > > "set ..." doesn't make sense, right? Maybe just delete "We choose to > ... during this test.". Will do on v2. Thanks.
On 27/06/19 20:03, Nadav Amit wrote: >> "set ..." doesn't make sense, right? Maybe just delete "We choose to >> ... during this test.". > Will do on v2. Thanks. > Or "We use CR4.MCE...". I can do this when applying (which is now). Paolo
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index b50d858..3731757 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -7096,8 +7096,11 @@ static int write_cr4_checking(unsigned long val) static void vmx_cr_load_test(void) { + unsigned long cr3, cr4, orig_cr3, orig_cr4; struct cpuid _cpuid = cpuid(1); - unsigned long cr4 = read_cr4(), cr3 = read_cr3(); + + orig_cr4 = read_cr4(); + orig_cr3 = read_cr3(); if (!(_cpuid.c & X86_FEATURE_PCID)) { report_skip("PCID not detected"); @@ -7108,12 +7111,11 @@ static void vmx_cr_load_test(void) return; } - TEST_ASSERT(!(cr4 & (X86_CR4_PCIDE | X86_CR4_MCE))); - TEST_ASSERT(!(cr3 & X86_CR3_PCID_MASK)); + TEST_ASSERT(!(orig_cr3 & X86_CR3_PCID_MASK)); /* Enable PCID for L1. */ - cr4 |= X86_CR4_PCIDE; - cr3 |= 0x1; + cr4 = orig_cr4 | X86_CR4_PCIDE; + cr3 = orig_cr3 | 0x1; TEST_ASSERT(!write_cr4_checking(cr4)); write_cr3(cr3); @@ -7126,17 +7128,16 @@ static void vmx_cr_load_test(void) * No exception is expected. * * NB. KVM loads the last guest write to CR4 into CR4 read - * shadow. In order to trigger an exit to KVM, we can set a - * bit that was zero in the above CR4 write and is owned by - * KVM. We choose to set CR4.MCE, which shall have no side - * effect because normally no guest MCE (e.g., as the result - * of bad memory) would happen during this test. + * shadow. In order to trigger an exit to KVM, we can toggle a + * bit that is owned by KVM. We choose to set CR4.MCE, which shall + * have no side effect because normally no guest MCE (e.g., as the + * result of bad memory) would happen during this test. */ - TEST_ASSERT(!write_cr4_checking(cr4 | X86_CR4_MCE)); + TEST_ASSERT(!write_cr4_checking(cr4 ^ X86_CR4_MCE)); - /* Cleanup L1 state: disable PCID. */ - write_cr3(cr3 & ~X86_CR3_PCID_MASK); - TEST_ASSERT(!write_cr4_checking(cr4 & ~X86_CR4_PCIDE)); + /* Cleanup L1 state. */ + write_cr3(orig_cr3); + TEST_ASSERT(!write_cr4_checking(orig_cr4)); } static void vmx_nm_test_guest(void)
CR4.MCE might be set after boot. Remove the assertion that checks that it is clear. Change the test to toggle the bit instead of setting it. Cc: Marc Orr <marcorr@google.com> Signed-off-by: Nadav Amit <nadav.amit@gmail.com> --- x86/vmx_tests.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)