> On Jun 26, 2019, at 5:35 PM, Marc Orr <marcorr@xxxxxxxxxx> wrote:
>
> On Tue, Jun 25, 2019 at 12:25 PM Nadav Amit <nadav.amit@xxxxxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: Nadav Amit <nadav.amit@xxxxxxxxx>
>> ---
>> 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.
