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

[Maciej S. Szmigiero]

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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
>
> Best regards,
> 	Maxim Levitsky

Thanks,
Maciej