>>> index 47e7427d0395..a770a1c7ddd2 100644
>>> --- a/arch/x86/kvm/svm/svm.h
>>> +++ b/arch/x86/kvm/svm/svm.h
>>> @@ -230,8 +230,8 @@ struct vcpu_svm {
>>> bool nmi_singlestep;
>>> u64 nmi_singlestep_guest_rflags;
>>>
>>> - unsigned int3_injected;
>>> - unsigned long int3_rip;
>>> + unsigned soft_int_injected;
>>> + unsigned long soft_int_linear_rip;
>>>
>>> /* optional nested SVM features that are enabled for this guest */
>>> bool nrips_enabled : 1;
>>
>>
>> I mostly agree with this patch, but think that it doesn't address the
>> original issue that Maciej wanted to address:
>>
>> Suppose that there is *no* instruction in L2 code which caused the software
>> exception, but rather L1 set arbitrary next_rip, and set EVENTINJ to software
>> exception with some vector, and that injection got interrupted.
>>
>> I don't think that this code will support this.
>
> Argh, you're right. Maciej's selftest injects without an instruction, but it doesn't
> configure the scenario where that injection fails due to an exception+VM-Exit that
> isn't intercepted by L1 and is handled by L0. The event_inj test gets the coverage
> for the latter, but always has a backing instruction.
Still reviewing the whole patch set, but want to clear this point quickly:
The selftest does have an implicit intervening NPF (handled by L0) while
injecting the first L1 -> L2 event.
Thanks,
Maciej
On Mon, Apr 04, 2022, Maciej S. Szmigiero wrote:
> > > > index 47e7427d0395..a770a1c7ddd2 100644
> > > > --- a/arch/x86/kvm/svm/svm.h
> > > > +++ b/arch/x86/kvm/svm/svm.h
> > > > @@ -230,8 +230,8 @@ struct vcpu_svm {
> > > > bool nmi_singlestep;
> > > > u64 nmi_singlestep_guest_rflags;
> > > > - unsigned int3_injected;
> > > > - unsigned long int3_rip;
> > > > + unsigned soft_int_injected;
> > > > + unsigned long soft_int_linear_rip;
> > > > /* optional nested SVM features that are enabled for this guest */
> > > > bool nrips_enabled : 1;
> > >
> > >
> > > I mostly agree with this patch, but think that it doesn't address the
> > > original issue that Maciej wanted to address:
> > >
> > > Suppose that there is *no* instruction in L2 code which caused the software
> > > exception, but rather L1 set arbitrary next_rip, and set EVENTINJ to software
> > > exception with some vector, and that injection got interrupted.
> > >
> > > I don't think that this code will support this.
> >
> > Argh, you're right. Maciej's selftest injects without an instruction, but it doesn't
> > configure the scenario where that injection fails due to an exception+VM-Exit that
> > isn't intercepted by L1 and is handled by L0. The event_inj test gets the coverage
> > for the latter, but always has a backing instruction.
>
> Still reviewing the whole patch set, but want to clear this point quickly:
> The selftest does have an implicit intervening NPF (handled by L0) while
> injecting the first L1 -> L2 event.
I'll do some debug to figure out why the test passes for me. I'm guessing I either
got lucky, e.g. IDT was faulted in already, or I screwed up and the test doesn't
actually pass.
On Mon, Apr 04, 2022, Sean Christopherson wrote:
> On Mon, Apr 04, 2022, Maciej S. Szmigiero wrote:
> > > > > index 47e7427d0395..a770a1c7ddd2 100644
> > > > > --- a/arch/x86/kvm/svm/svm.h
> > > > > +++ b/arch/x86/kvm/svm/svm.h
> > > > > @@ -230,8 +230,8 @@ struct vcpu_svm {
> > > > > bool nmi_singlestep;
> > > > > u64 nmi_singlestep_guest_rflags;
> > > > > - unsigned int3_injected;
> > > > > - unsigned long int3_rip;
> > > > > + unsigned soft_int_injected;
> > > > > + unsigned long soft_int_linear_rip;
> > > > > /* optional nested SVM features that are enabled for this guest */
> > > > > bool nrips_enabled : 1;
> > > >
> > > >
> > > > I mostly agree with this patch, but think that it doesn't address the
> > > > original issue that Maciej wanted to address:
> > > >
> > > > Suppose that there is *no* instruction in L2 code which caused the software
> > > > exception, but rather L1 set arbitrary next_rip, and set EVENTINJ to software
> > > > exception with some vector, and that injection got interrupted.
> > > >
> > > > I don't think that this code will support this.
> > >
> > > Argh, you're right. Maciej's selftest injects without an instruction, but it doesn't
> > > configure the scenario where that injection fails due to an exception+VM-Exit that
> > > isn't intercepted by L1 and is handled by L0. The event_inj test gets the coverage
> > > for the latter, but always has a backing instruction.
> >
> > Still reviewing the whole patch set, but want to clear this point quickly:
> > The selftest does have an implicit intervening NPF (handled by L0) while
> > injecting the first L1 -> L2 event.
>
> I'll do some debug to figure out why the test passes for me. I'm guessing I either
> got lucky, e.g. IDT was faulted in already, or I screwed up and the test doesn't
> actually pass.
Well that was easy. My code is indeed flawed and skips the wrong instruction,
the skipped instruction just so happens to be a (spurious?) adjustment of RSP. The
L2 guest function never runs to completion and so the "bad" RSP is never consumed.
KVM: incomplete injection for L2, vector 32 @ 401c70. next_rip = 0
KVM: injecting for L2, vector 0 @ 401c70. next_rip = 401c74
0000000000401c70 <l2_guest_code>:
401c70: 48 83 ec 08 sub $0x8,%rsp
401c74: 83 3d 75 a7 0e 00 01 cmpl $0x1,0xea775(%rip) # 4ec3f0 <int_fired>
401c7b: 74 1e je 401c9b <l2_guest_code+0x2b>
401c7d: 45 31 c0 xor %r8d,%r8d
401c80: b9 32 00 00 00 mov $0x32,%ecx
401c85: ba 90 40 4b 00 mov $0x4b4090,%edx
401c8a: 31 c0 xor %eax,%eax
401c8c: be 02 00 00 00 mov $0x2,%esi
401c91: bf 02 00 00 00 mov $0x2,%edi
401c96: e8 05 ae 00 00 call 40caa0 <ucall>
401c9b: 0f 01 d9 vmmcall
401c9e: 0f 0b ud2
401ca0: 83 3d 4d a7 0e 00 01 cmpl $0x1,0xea74d(%rip) # 4ec3f4 <bp_fired>
401ca7: 74 1e je 401cc7 <l2_guest_code+0x57>
401ca9: 45 31 c0 xor %r8d,%r8d
401cac: b9 36 00 00 00 mov $0x36,%ecx
401cb1: ba b8 40 4b 00 mov $0x4b40b8,%edx
401cb6: 31 c0 xor %eax,%eax
401cb8: be 02 00 00 00 mov $0x2,%esi
401cbd: bf 02 00 00 00 mov $0x2,%edi
401cc2: e8 d9 ad 00 00 call 40caa0 <ucall>
401cc7: f4 hlt
401cc8: 48 83 c4 08 add $0x8,%rsp
401ccc: c3 ret
401ccd: 0f 1f 00 nopl (%rax)
I don't see why the compiler is creating room for a single variable, but it doesn't
really matter, the easiest way to detect this bug is to assert that the return RIP
in the INT 0x20 handler points at l2_guest_code, e.g. this fails:
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
index d39be5d885c1..257aa2280b5c 100644
--- 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
@@ -40,9 +40,13 @@ static void guest_bp_handler(struct ex_regs *regs)
}
static unsigned int int_fired;
+static void l2_guest_code(void);
+
static void guest_int_handler(struct ex_regs *regs)
{
int_fired++;
+ GUEST_ASSERT_2(regs->rip == (unsigned long)l2_guest_code,
+ regs->rip, (unsigned long)l2_guest_code);
}
static void l2_guest_code(void)
On 4.04.2022 21:33, Sean Christopherson wrote:
> On Mon, Apr 04, 2022, Maciej S. Szmigiero wrote:
>>>>> index 47e7427d0395..a770a1c7ddd2 100644
>>>>> --- a/arch/x86/kvm/svm/svm.h
>>>>> +++ b/arch/x86/kvm/svm/svm.h
>>>>> @@ -230,8 +230,8 @@ struct vcpu_svm {
>>>>> bool nmi_singlestep;
>>>>> u64 nmi_singlestep_guest_rflags;
>>>>> - unsigned int3_injected;
>>>>> - unsigned long int3_rip;
>>>>> + unsigned soft_int_injected;
>>>>> + unsigned long soft_int_linear_rip;
>>>>> /* optional nested SVM features that are enabled for this guest */
>>>>> bool nrips_enabled : 1;
>>>>
>>>>
>>>> I mostly agree with this patch, but think that it doesn't address the
>>>> original issue that Maciej wanted to address:
>>>>
>>>> Suppose that there is *no* instruction in L2 code which caused the software
>>>> exception, but rather L1 set arbitrary next_rip, and set EVENTINJ to software
>>>> exception with some vector, and that injection got interrupted.
>>>>
>>>> I don't think that this code will support this.
>>>
>>> Argh, you're right. Maciej's selftest injects without an instruction, but it doesn't
>>> configure the scenario where that injection fails due to an exception+VM-Exit that
>>> isn't intercepted by L1 and is handled by L0. The event_inj test gets the coverage
>>> for the latter, but always has a backing instruction.
>>
>> Still reviewing the whole patch set, but want to clear this point quickly:
>> The selftest does have an implicit intervening NPF (handled by L0) while
>> injecting the first L1 -> L2 event.
>
> I'll do some debug to figure out why the test passes for me. I'm guessing I either
> got lucky, e.g. IDT was faulted in already, or I screwed up and the test doesn't
> actually pass.
Try applying Maxim's proposed change to this test (adding two additional ud2s at the
start of L2 guest code and adjusting just the next_rip field accordingly).
We might have been lucky in the patched KVM code and have next_rip = rip at this
re-injection case now (the next_rip field was zero in the original KVM code).
Thanks,
Maciej