From: "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
To: Sean Christopherson <seanjc@google.com>,
Maxim Levitsky <mlevitsk@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/8] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction
Date: Mon, 4 Apr 2022 22:44:04 +0200 [thread overview]
Message-ID: <7caee33a-da0f-00be-3195-82c3d1cd4cb4@maciej.szmigiero.name> (raw)
In-Reply-To: <YkshgrUaF4+MrrXf@google.com>
On 4.04.2022 18:49, Sean Christopherson wrote:
> On Mon, Apr 04, 2022, Maxim Levitsky wrote:
>> On Sat, 2022-04-02 at 01:09 +0000, Sean Christopherson wrote:
>>> Re-inject INT3/INTO instead of retrying the instruction if the CPU
>>> encountered an intercepted exception while vectoring the software
>>> exception, e.g. if vectoring INT3 encounters a #PF and KVM is using
>>> shadow paging. Retrying the instruction is architecturally wrong, e.g.
>>> will result in a spurious #DB if there's a code breakpoint on the INT3/O,
>>> and lack of re-injection also breaks nested virtualization, e.g. if L1
>>> injects a software exception and vectoring the injected exception
>>> encounters an exception that is intercepted by L0 but not L1.
>>>
>>> Due to, ahem, deficiencies in the SVM architecture, acquiring the next
>>> RIP may require flowing through the emulator even if NRIPS is supported,
>>> as the CPU clears next_rip if the VM-Exit is due to an exception other
>>> than "exceptions caused by the INT3, INTO, and BOUND instructions". To
>>> deal with this, "skip" the instruction to calculate next_ript, and then
>>> unwind the RIP write and any side effects (RFLAGS updates).
>
> ...
>
(..)
>>> +
>>> kvm_make_request(KVM_REQ_EVENT, vcpu);
>>>
>>> vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
>>> @@ -3711,9 +3762,9 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>>> * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
>>> * be the reported vectored event, but RIP still needs to be unwound.
>>> */
>>> - if (int3_injected && type == SVM_EXITINTINFO_TYPE_EXEPT &&
>>> - kvm_is_linear_rip(vcpu, svm->int3_rip))
>>> - kvm_rip_write(vcpu, kvm_rip_read(vcpu) - int3_injected);
>>> + if (soft_int_injected && type == SVM_EXITINTINFO_TYPE_EXEPT &&
>>> + kvm_is_linear_rip(vcpu, svm->soft_int_linear_rip))
>>> + kvm_rip_write(vcpu, kvm_rip_read(vcpu) - soft_int_injected);
>>>
>>> switch (type) {
>>> case SVM_EXITINTINFO_TYPE_NMI:
>>> @@ -3726,14 +3777,6 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>>> if (vector == X86_TRAP_VC)
>>> break;
>>>
>>> - /*
>>> - * In case of software exceptions, do not reinject the vector,
>>> - * but re-execute the instruction instead. Rewind RIP first
>>> - * if we emulated INT3 before.
>>> - */
>>> - if (kvm_exception_is_soft(vector))
>>> - break;
>>> -
>>> if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
>>> u32 err = svm->vmcb->control.exit_int_info_err;
>>> kvm_requeue_exception_e(vcpu, vector, err);
>>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>>> 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.
>
>> I think that svm_complete_interrupts should store next_rip it in some field
>> like VMX does (vcpu->arch.event_exit_inst_len).
>
> Yeah. The ugly part is that because next_rip is guaranteed to be cleared on exit
> (the exit is gauranteed to be due to a fault-like exception), KVM has to snapshot
> next_rip during the "original" injection and use the linear_rip matching heuristic
> to detect this scenario.
>
>> That field also should be migrated, or we must prove that it works anyway.
>> E.g, what happens when we tried to inject event,
>> injection was interrupted by other exception, and then we migrate?
>
> Ya, should Just Work if control.next_rip is used to cache the next rip.
>
> Handling this doesn't seem to be too awful (haven't tested yet), it's largely the
> same logic as the existing !nrips code.
>
> In svm_update_soft_interrupt_rip(), snapshot all information regardless of whether
> or not nrips is enabled:
>
> svm->soft_int_injected = true;
> svm->soft_int_csbase = svm->vmcb->save.cs.base;
> svm->soft_int_old_rip = old_rip;
> svm->soft_int_next_rip = rip;
>
> if (nrips)
> kvm_rip_write(vcpu, old_rip);
>
> if (static_cpu_has(X86_FEATURE_NRIPS))
> svm->vmcb->control.next_rip = rip;
>
> and then in svm_complete_interrupts(), change the linear RIP matching code to look
> for the old rip in the nrips case and stuff svm->vmcb->control.next_rip on match.
>
> bool soft_int_injected = svm->soft_int_injected;
> unsigned soft_int_rip;
>
> svm->soft_int_injected = false;
>
> if (soft_int_injected) {
> if (nrips)
> soft_int_rip = svm->soft_int_old_rip;
> else
> soft_int_rip = svm->soft_int_next_rip;
> }
>
> ...
>
> if soft_int_injected && type == SVM_EXITINTINFO_TYPE_EXEPT &&
> kvm_is_linear_rip(vcpu, soft_int_rip + svm->soft_int_csbase)) {
> if (nrips)
> svm->vmcb->control.next_rip = svm->soft_int_next_rip;
> else
> kvm_rip_write(vcpu, svm->soft_int_old_rip);
> }
>
>
>
Despite what the svm_update_soft_interrupt_rip() name might suggest this
handles only *soft exceptions*, not *soft interrupts*
(which are injected by svm_inject_irq() and also need proper next_rip
management).
Also, I'm not sure that even the proposed updated code above will
actually restore the L1-requested next_rip correctly on L1 -> L2
re-injection (will review once the full version is available).
Thanks,
Maciej
next prev parent reply other threads:[~2022-04-04 22:03 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-02 1:08 [PATCH 0/8] KVM: SVM: Fix soft int/ex re-injection Sean Christopherson
2022-04-02 1:08 ` [PATCH 1/8] KVM: nSVM: Sync next_rip field from vmcb12 to vmcb02 Sean Christopherson
2022-04-04 9:54 ` Maxim Levitsky
2022-04-04 16:50 ` Maciej S. Szmigiero
2022-04-04 17:21 ` Sean Christopherson
2022-04-04 17:45 ` Maciej S. Szmigiero
2022-04-20 15:00 ` Paolo Bonzini
2022-04-20 15:05 ` Maciej S. Szmigiero
2022-04-20 16:15 ` Sean Christopherson
2022-04-20 16:33 ` Paolo Bonzini
2022-04-20 16:44 ` Sean Christopherson
2022-04-02 1:08 ` [PATCH 2/8] KVM: SVM: Downgrade BUG_ON() to WARN_ON() in svm_inject_irq() Sean Christopherson
2022-04-02 1:08 ` [PATCH 3/8] KVM: SVM: Unwind "speculative" RIP advancement if INTn injection "fails" Sean Christopherson
2022-04-04 10:03 ` Maxim Levitsky
2022-04-20 15:01 ` Paolo Bonzini
2022-04-02 1:08 ` [PATCH 4/8] KVM: SVM: Stuff next_rip on emualted INT3 injection if NRIPS is supported Sean Christopherson
2022-04-04 12:00 ` Maxim Levitsky
2022-04-02 1:09 ` [PATCH 5/8] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction Sean Christopherson
2022-04-04 12:12 ` Maxim Levitsky
2022-04-04 16:49 ` Sean Christopherson
2022-04-04 16:53 ` Maciej S. Szmigiero
2022-04-04 19:33 ` Sean Christopherson
2022-04-04 19:50 ` Maciej S. Szmigiero
2022-04-04 19:54 ` Sean Christopherson
2022-04-04 20:46 ` Maciej S. Szmigiero
2022-04-04 20:44 ` Maciej S. Szmigiero [this message]
2022-04-06 1:48 ` Sean Christopherson
2022-04-06 13:13 ` Maciej S. Szmigiero
2022-04-06 17:10 ` Sean Christopherson
2022-04-06 19:08 ` Maciej S. Szmigiero
2022-04-06 19:48 ` Sean Christopherson
2022-04-06 20:30 ` Maciej S. Szmigiero
2022-04-06 20:52 ` Sean Christopherson
2022-04-06 22:34 ` Maciej S. Szmigiero
2022-04-06 23:03 ` Sean Christopherson
2022-04-07 15:32 ` Maciej S. Szmigiero
2022-04-02 1:09 ` [PATCH 6/8] KVM: SVM: Re-inject INTn instead of retrying the insn on "failure" Sean Christopherson
2022-04-04 17:14 ` Sean Christopherson
2022-04-04 20:27 ` Maciej S. Szmigiero
2022-04-02 1:09 ` [PATCH 7/8] KVM: x86: Trace re-injected exceptions Sean Christopherson
2022-04-04 12:14 ` Maxim Levitsky
2022-04-04 16:14 ` Sean Christopherson
2022-04-02 1:09 ` [PATCH 8/8] KVM: selftests: nSVM: Add svm_nested_soft_inject_test Sean Christopherson
2022-04-04 12:27 ` Maxim Levitsky
2022-04-04 16:59 ` Maciej S. Szmigiero
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7caee33a-da0f-00be-3195-82c3d1cd4cb4@maciej.szmigiero.name \
--to=mail@maciej.szmigiero.name \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mlevitsk@redhat.com \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).