> On Jul 8, 2020, at 5:01 PM, Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx> wrote:
>
>
> On 7/8/20 4:07 AM, Paolo Bonzini wrote:
>> On 08/07/20 02:39, Krish Sadhukhan wrote:
>>> + SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
>>> + SVM_CR3_LEGACY_PAE_RESERVED_MASK);
>>> +
>>> + cr4 = cr4_saved & ~X86_CR4_PAE;
>>> + vmcb->save.cr4 = cr4;
>>> + SVM_TEST_CR_RESERVED_BITS(0, 11, 2, 3, cr3_saved,
>>> + SVM_CR3_LEGACY_RESERVED_MASK);
>>> +
>>> + cr4 |= X86_CR4_PAE;
>>> + vmcb->save.cr4 = cr4;
>>> + efer |= EFER_LMA;
>>> + vmcb->save.efer = efer;
>>> + SVM_TEST_CR_RESERVED_BITS(0, 63, 2, 3, cr3_saved,
>>> + SVM_CR3_LONG_RESERVED_MASK);
>> The test is not covering *non*-reserved bits, so it doesn't catch that
>> in both cases KVM is checking against long-mode bits. Doing this would
>> require setting up the VMCB for immediate VMEXIT (for example, injecting
>> an event while the IDT limit is zero), so it can be done later.
>>
>> Instead, you need to set/clear EFER_LME. Please be more careful to
>> check that the test is covering what you expect.
>
>
> Sorry, I wasn't aware that setting/unsetting EFER.LMA wouldn't work !
>
> Another test case that I missed here is testing the reserved bits in CR3[11:0] in long-mode based on the setting of CR4.PCIDE.
>
>> Also, the tests show
>>
>> PASS: Test CR3 2:0: 641001
>> PASS: Test CR3 2:0: 2
>> PASS: Test CR3 2:0: 4
>> PASS: Test CR3 11:0: 1
>> PASS: Test CR3 11:0: 4
>> PASS: Test CR3 11:0: 40
>> PASS: Test CR3 11:0: 100
>> PASS: Test CR3 11:0: 400
>> PASS: Test CR3 63:0: 1
>> PASS: Test CR3 63:0: 4
>> PASS: Test CR3 63:0: 40
>> PASS: Test CR3 63:0: 100
>> PASS: Test CR3 63:0: 400
>> PASS: Test CR3 63:0: 10000000000000
>> PASS: Test CR3 63:0: 40000000000000
>> PASS: Test CR3 63:0: 100000000000000
>> PASS: Test CR3 63:0: 400000000000000
>> PASS: Test CR3 63:0: 1000000000000000
>> PASS: Test CR3 63:0: 4000000000000000
>> PASS: Test CR4 31:12: 0
>> PASS: Test CR4 31:12: 0
>>
>> and then exits. There is an issue with compiler optimization for which
>> I've sent a patch, but even after fixing it the premature exit is a
>> problem: it is caused by a problem in __cr4_reserved_bits and a typo in
>> the tests:
>>
>> diff --git a/x86/svm.h b/x86/svm.h
>> index f6b9a31..58c9069 100644
>> --- a/x86/svm.h
>> +++ b/x86/svm.h
>> @@ -328,8 +328,8 @@ struct __attribute__ ((__packed__)) vmcb {
>> #define SVM_CR3_LEGACY_RESERVED_MASK 0xfe7U
>> #define SVM_CR3_LEGACY_PAE_RESERVED_MASK 0x7U
>> #define SVM_CR3_LONG_RESERVED_MASK 0xfff0000000000fe7U
>> -#define SVM_CR4_LEGACY_RESERVED_MASK 0xffbaf000U
>> -#define SVM_CR4_RESERVED_MASK 0xffffffffffbaf000U
>> +#define SVM_CR4_LEGACY_RESERVED_MASK 0xffcaf000U
>> +#define SVM_CR4_RESERVED_MASK 0xffffffffffcaf000U
>> #define SVM_DR6_RESERVED_MASK 0xffffffffffff1ff0U
>> #define SVM_DR7_RESERVED_MASK 0xffffffff0000cc00U
>> #define SVM_EFER_RESERVED_MASK 0xffffffffffff0200U
>>
>> (Also, this kind of problem is made harder to notice by only testing
>> even bits, which may make sense for high order bits, but certainly not
>> for low-order ones).
>>
>> All in all, fixing this series has taken me almost 2 hours. Since I have
>> done the work I'm queuing
>
>
> Sorry to hear it caused so many issues ! Thanks for looking into it !
>
>> but, but I wonder: the compiler optimization
>> issue could depend on register allocation, but did all of these issues
>> really happen only on my machine?
>
>
> Just as a reference, I compiled it using gcc 4.8.5 and ran it on an AMD EPYC that was running kernel 5.8.0-rc4+. Surprisingly, all tests passed:
>
> PASS: Test CR3 2:0: 641001
> PASS: Test CR3 2:0: 641002
> PASS: Test CR3 2:0: 641004
> PASS: Test CR3 11:0: 641001
> PASS: Test CR3 11:0: 641004
> PASS: Test CR3 11:0: 641040
> PASS: Test CR3 11:0: 641100
> PASS: Test CR3 11:0: 641400
> PASS: Test CR3 63:0: 641001
> PASS: Test CR3 63:0: 641004
> PASS: Test CR3 63:0: 641040
> PASS: Test CR3 63:0: 641100
> PASS: Test CR3 63:0: 641400
Well, the tests (which I pulled) do not pass on bare-metal, so KVM is even
better than bare-metal… Here are the results for long-mode:
FAIL: Test CR3 63:0: 641001
FAIL: Test CR3 63:0: 641002
FAIL: Test CR3 63:0: 641004
FAIL: Test CR3 63:0: 641020
FAIL: Test CR3 63:0: 641040
FAIL: Test CR3 63:0: 641080
FAIL: Test CR3 63:0: 641100
FAIL: Test CR3 63:0: 641200
FAIL: Test CR3 63:0: 641400
FAIL: Test CR3 63:0: 641800
PASS: Test CR3 63:0: 10000000641000
PASS: Test CR3 63:0: 20000000641000
PASS: Test CR3 63:0: 40000000641000
PASS: Test CR3 63:0: 80000000641000
PASS: Test CR3 63:0: 100000000641000
PASS: Test CR3 63:0: 200000000641000
PASS: Test CR3 63:0: 400000000641000
PASS: Test CR3 63:0: 800000000641000
PASS: Test CR3 63:0: 1000000000641000
PASS: Test CR3 63:0: 2000000000641000
PASS: Test CR3 63:0: 4000000000641000
PASS: Test CR3 63:0: 8000000000641000
The PAE/legacy tests crashed my machine. Presumably the VM PTEs are
completely messed up on PAE/legacy so a failure can cause a crash. It would
be better to do something smarter to avoid a crash, perhaps by setting an
invalid PDEs or something in the guest.
Anyhow, to double-check that the VM-entry was successful, I checked the exit
reason on long-mode, and indeed I get a VMMCALL exit, and CR3 of the guest
holds the “illegal” value.
Checking the APM shows that the low bits of CR3 are marked as reserved but
not MBZ. So the condition that the test tries to check "Any MBZ bit of CR3
is set” does not apply to the low-bits.
