On 4.04.2022 21:54, Sean Christopherson wrote:
> 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)
It totally makes sense to add the above as an additional assert to the
self test - the more checks the test have the better at catching bugs
it is.
Thanks,
Maciej