On 8/2/23 23:07, Sean Christopherson wrote:
> On Fri, Jul 28, 2023, Michal Luczaj wrote:
>> +#define TIMEOUT 2 /* seconds, roughly */
>
> I think it makes sense to make this a const in race_sync_regs(), that way its
> usage is a bit more obvious.
Yeah, sure.
>> +/*
>> + * WARNING: CPU: 0 PID: 1115 at arch/x86/kvm/x86.c:10095 kvm_check_and_inject_events+0x220/0x500 [kvm]
>> + *
>> + * arch/x86/kvm/x86.c:kvm_check_and_inject_events():
>> + * WARN_ON_ONCE(vcpu->arch.exception.injected &&
>> + * vcpu->arch.exception.pending);
>> + */
>
> For comments in selftests, describe what's happening without referencing KVM code,
> things like this in particular will become stale sooner than later. It's a-ok
> (and encouraged) to put the WARNs and function references in changelogs though,
> as those are explicitly tied to a specific time in history.
Right, I'll try to remember. Actually, those comments were notes for myself and
then I've just left them thinking they can't hurt. But I agree that wasn't the
best idea.
>> +static void race_sync_regs(void *racer, bool poke_mmu)
>> +{
>> + struct kvm_translation tr;
>> + struct kvm_vcpu *vcpu;
>> + struct kvm_run *run;
>> + struct kvm_vm *vm;
>> + pthread_t thread;
>> + time_t t;
>> +
>> + vm = vm_create_with_one_vcpu(&vcpu, guest_code);
>> + run = vcpu->run;
>> +
>> + run->kvm_valid_regs = KVM_SYNC_X86_SREGS;
>> + vcpu_run(vcpu);
>> + TEST_REQUIRE(run->s.regs.sregs.cr4 & X86_CR4_PAE);
>
> This can be an assert, and should also check EFER.LME. Jump-starting in long mode
> is a property of selftests, i.e. not something that should ever randomly "fail".
Right, sorry for the misuse.
>> + run->kvm_valid_regs = 0;
>> +
>> + ASSERT_EQ(pthread_create(&thread, NULL, racer, (void *)run), 0);
>> +
>> + for (t = time(NULL) + TIMEOUT; time(NULL) < t;) {
>> + __vcpu_run(vcpu);
>> +
>> + if (poke_mmu) {
>
> Rather than pass a boolean, I think it makes sense to do
>
> if (racer == race_sregs_cr4)
>
> It's arguably just trading ugliness for subtlety, but IMO it's worth avoiding
> the boolean.
Ah, ok.
>> + /*
>> + * If kvm->bugged then we won't survive TEST_ASSERT(). Leak.
>> + *
>> + * kvm_vm_free()
>> + * __vm_mem_region_delete()
>> + * vm_ioctl(vm, KVM_SET_USER_MEMORY_REGION, ®ion->region)
>> + * _vm_ioctl(vm, cmd, #cmd, arg)
>> + * TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret))
>> + */
>
> We want the assert, it makes failures explicit. The signature is a bit unfortunate,
> but the WARN in the kernel log should provide a big clue.
Sure, I get it. And not that there is a way to check if VM is bugged/dead?
> I'll fix up all of the above when applying, and will also split this into three
> patches, mostly so that each splat can be covered in a changelog, i.e. is tied
> to its testcase.
Great, thank you for all the comments and fixes!
Michal
