lkml.org 
[lkml]   [2022]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 05/11] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction
On 28.04.2022 11:37, Maxim Levitsky wrote:
> On Sat, 2022-04-23 at 02:14 +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_rip (if it's
>> not already known), and then unwind the RIP write and any side effects
>> (RFLAGS updates).
>>
>> Save the computed next_rip and use it to re-stuff next_rip if injection
>> doesn't complete. This allows KVM to do the right thing if next_rip was
>> known prior to injection, e.g. if L1 injects a soft event into L2, and
>> there is no backing INTn instruction, e.g. if L1 is injecting an
>> arbitrary event.
>>
>> Note, it's impossible to guarantee architectural correctness given SVM's
>> architectural flaws. E.g. if the guest executes INTn (no KVM injection),
>> an exit occurs while vectoring the INTn, and the guest modifies the code
>> stream while the exit is being handled, KVM will compute the incorrect
>> next_rip due to "skipping" the wrong instruction. A future enhancement
>> to make this less awful would be for KVM to detect that the decoded
>> instruction is not the correct INTn and drop the to-be-injected soft
>> event (retrying is a lesser evil compared to shoving the wrong RIP on the
>> exception stack).
>>
>> Reported-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> ---
>> arch/x86/kvm/svm/nested.c | 28 +++++++-
>> arch/x86/kvm/svm/svm.c | 140 +++++++++++++++++++++++++++-----------
>> arch/x86/kvm/svm/svm.h | 6 +-
>> 3 files changed, 130 insertions(+), 44 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 461c5f247801..0163238aa198 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -609,6 +609,21 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>> }
>> }
>>
>> +static inline bool is_evtinj_soft(u32 evtinj)
>> +{
>> + u32 type = evtinj & SVM_EVTINJ_TYPE_MASK;
>> + u8 vector = evtinj & SVM_EVTINJ_VEC_MASK;
>> +
>> + if (!(evtinj & SVM_EVTINJ_VALID))
>> + return false;
>> +
>> + /*
>> + * Intentionally return false for SOFT events, SVM doesn't yet support
>> + * re-injecting soft interrupts.
>> + */
>> + return type == SVM_EVTINJ_TYPE_EXEPT && kvm_exception_is_soft(vector);
>> +}
>> +
>> static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>> unsigned long vmcb12_rip)
>> {
>> @@ -677,6 +692,16 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>> else if (boot_cpu_has(X86_FEATURE_NRIPS))
>> vmcb02->control.next_rip = vmcb12_rip;
>>
>> + if (is_evtinj_soft(vmcb02->control.event_inj)) {
>> + svm->soft_int_injected = true;
>> + svm->soft_int_csbase = svm->vmcb->save.cs.base;
>> + svm->soft_int_old_rip = vmcb12_rip;
>> + if (svm->nrips_enabled)
>> + svm->soft_int_next_rip = svm->nested.ctl.next_rip;
>> + else
>> + svm->soft_int_next_rip = vmcb12_rip;
>> + }
>> +
>> vmcb02->control.virt_ext = vmcb01->control.virt_ext &
>> LBR_CTL_ENABLE_MASK;
>> if (svm->lbrv_enabled)
>> @@ -849,6 +874,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>>
>> out_exit_err:
>> svm->nested.nested_run_pending = 0;
>> + svm->soft_int_injected = false;
>>
>> svm->vmcb->control.exit_code = SVM_EXIT_ERR;
>> svm->vmcb->control.exit_code_hi = 0;
>> @@ -1618,7 +1644,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>> nested_copy_vmcb_control_to_cache(svm, ctl);
>>
>> svm_switch_vmcb(svm, &svm->nested.vmcb02);
>> - nested_vmcb02_prepare_control(svm, save->rip);
>> + nested_vmcb02_prepare_control(svm, svm->vmcb->save.rip);
>
> Is this change intentional?

It looks to me the final code is correct since "svm->vmcb->save"
contains L2 register save, while "save" has L1 register save.

It was the patch 1 from this series that was incorrect in
using "save->rip" here instead.

Thanks,
Maciej

\
 
 \ /
  Last update: 2022-04-28 15:38    [W:0.216 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site