Re: [PATCH 8/8] KVM: selftests: nSVM: Add svm_nested_soft_inject_test

Maciej S. Szmigiero
2022-04-04T21:49:44Z

On 4.04.2022 14:27, Maxim Levitsky wrote:
> On Sat, 2022-04-02 at 01:09 +0000, Sean Christopherson wrote:
>> From: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>
>> Add a KVM self-test that checks whether a nSVM L1 is able to successfully
>> inject a software interrupt and a soft exception into its L2 guest.
>>
>> In practice, this tests both the next_rip field consistency and
>> L1-injected event with intervening L0 VMEXIT during its delivery:
>> the first nested VMRUN (that's also trying to inject a software interrupt)
>> will immediately trigger a L0 NPF.
>> This L0 NPF will have zero in its CPU-returned next_rip field, which if
>> incorrectly reused by KVM will trigger a #PF when trying to return to
>> such address 0 from the interrupt handler.
>>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> ---
>> tools/testing/selftests/kvm/.gitignore | 1 +
>> tools/testing/selftests/kvm/Makefile | 1 +
>> .../selftests/kvm/include/x86_64/svm_util.h | 2 +
>> .../kvm/x86_64/svm_nested_soft_inject_test.c | 147 ++++++++++++++++++
>> 4 files changed, 151 insertions(+)
>> create mode 100644 tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
>>
(..)
>> diff --git a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
>> new file mode 100644
>> index 000000000000..d39be5d885c1
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
>> @@ -0,0 +1,147 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2022 Oracle and/or its affiliates.
>> + *
>> + * Based on:
>> + * svm_int_ctl_test
>> + *
>> + * Copyright (C) 2021, Red Hat, Inc.
>> + *
>> + */
>> +
>> +#include "test_util.h"
>> +#include "kvm_util.h"
>> +#include "processor.h"
>> +#include "svm_util.h"
>> +
>> +#define VCPU_ID 0
>> +#define INT_NR 0x20
>> +#define X86_FEATURE_NRIPS BIT(3)
>> +
>> +#define vmcall() \
>> + __asm__ __volatile__( \
>> + "vmmcall\n" \
>> + )
>> +
>> +#define ud2() \
>> + __asm__ __volatile__( \
>> + "ud2\n" \
>> + )
>> +
>> +#define hlt() \
>> + __asm__ __volatile__( \
>> + "hlt\n" \
>> + )
>
> Minor nitpick: I guess it would be nice to put these in some common header file.

Will move these to include/x86_64/processor.h (that's probably the most
matching header file).

>> +
>> +static void l1_guest_code(struct svm_test_data *svm)
>> +{
>> + #define L2_GUEST_STACK_SIZE 64
>> + unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
>> + struct vmcb *vmcb = svm->vmcb;
>> +
>> + /* Prepare for L2 execution. */
>> + generic_svm_setup(svm, l2_guest_code,
>> + &l2_guest_stack[L2_GUEST_STACK_SIZE]);
>> +
>> + vmcb->control.intercept_exceptions |= BIT(PF_VECTOR) | BIT(UD_VECTOR);
>> + vmcb->control.intercept |= BIT(INTERCEPT_HLT);
>> +
>> + vmcb->control.event_inj = INT_NR | SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_SOFT;
>> + /* The return address pushed on stack */
>> + vmcb->control.next_rip = vmcb->save.rip;
>
> I'll would be putting even something more spicy here just to see that KVM preserves this field
> like say put two ud2 in the start of guest code, and here have
>
> vmcb->control.next_rip = vmcb->save.rip + 4; // skip over two ud2 instructions.
>
> That way KVM won't be able to skip over a single instruction to get correct next_rip

Good point, will add these two additional ud2s at the start of L2 guest code.

>
>
> Other that nitpicks:
>
> Reviewed-by: Maxim levitsky <mlevitsk@redhat.com>
>
> Best regards,
> Maxim Levitsky
>
>

Thanks,
Maciej