[kvm-unit-tests PATCH] x86: Remove assumptions on CR4.MCE

[kvm-unit-tests PATCH] x86: Remove assumptions on CR4.MCE

[Nadav Amit]

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)
-- 
2.17.1

Re: [kvm-unit-tests PATCH] x86: Remove assumptions on CR4.MCE

[Krish Sadhukhan]

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>

Re: [kvm-unit-tests PATCH] x86: Remove assumptions on CR4.MCE

[Marc Orr]

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>

Re: [kvm-unit-tests PATCH] x86: Remove assumptions on CR4.MCE

[Nadav Amit]

> 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.

Re: [kvm-unit-tests PATCH] x86: Remove assumptions on CR4.MCE

[Paolo Bonzini]

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