| Message ID | 20190620110240.25799-4-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/5] x86: KVM: svm: don't pretend to advance RIP in case wrmsr_interception() results in #GP | expand |
On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Regardless of the way how we skip instruction, interrupt shadow needs to be > cleared. This change is definitely an improvement, but the existing code seems to assume that we never call skip_emulated_instruction on a POP-SS/MOV-to-SS/STI. Is that enforced anywhere? Reviewed-by: Jim Mattson <jmattson@google.com>
Jim Mattson <jmattson@google.com> writes: > On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Regardless of the way how we skip instruction, interrupt shadow needs to be >> cleared. > > This change is definitely an improvement, but the existing code seems > to assume that we never call skip_emulated_instruction on a > POP-SS/MOV-to-SS/STI. Is that enforced anywhere? Hm, good question. I'll try to check before v1.
Jim Mattson <jmattson@google.com> writes: > On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Regardless of the way how we skip instruction, interrupt shadow needs to be >> cleared. > > This change is definitely an improvement, but the existing code seems > to assume that we never call skip_emulated_instruction on a > POP-SS/MOV-to-SS/STI. Is that enforced anywhere? (before I send v1 of the series) I looked at the current code and I don't think it is enforced, however, VMX version does the same and honestly I can't think of a situation when we would be doing 'skip' for such an instruction.... and there's nothing we can easily enforce from skip_emulated_instruction() as we have no idea what the instruction is... I can of course be totally wrong and would appreciate if someone more knowledgeable chimes in :-)
On 31/07/19 15:50, Vitaly Kuznetsov wrote: > Jim Mattson <jmattson@google.com> writes: > >> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >>> >>> Regardless of the way how we skip instruction, interrupt shadow needs to be >>> cleared. >> >> This change is definitely an improvement, but the existing code seems >> to assume that we never call skip_emulated_instruction on a >> POP-SS/MOV-to-SS/STI. Is that enforced anywhere? > > (before I send v1 of the series) I looked at the current code and I > don't think it is enforced, however, VMX version does the same and > honestly I can't think of a situation when we would be doing 'skip' for > such an instruction.... and there's nothing we can easily enforce from > skip_emulated_instruction() as we have no idea what the instruction > is... I agree, I think a comment is worthwhile but we can live with the limitation. Paolo
On Wed, Jul 31, 2019 at 9:37 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 31/07/19 15:50, Vitaly Kuznetsov wrote: > > Jim Mattson <jmattson@google.com> writes: > > > >> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >>> > >>> Regardless of the way how we skip instruction, interrupt shadow needs to be > >>> cleared. > >> > >> This change is definitely an improvement, but the existing code seems > >> to assume that we never call skip_emulated_instruction on a > >> POP-SS/MOV-to-SS/STI. Is that enforced anywhere? > > > > (before I send v1 of the series) I looked at the current code and I > > don't think it is enforced, however, VMX version does the same and > > honestly I can't think of a situation when we would be doing 'skip' for > > such an instruction.... and there's nothing we can easily enforce from > > skip_emulated_instruction() as we have no idea what the instruction > > is... Can't we still coerce kvm into emulating any instruction by leveraging a stale ITLB entry? The 'emulator' kvm-unit-test did this before the KVM forced emulation prefix was introduced, but I haven't checked to see if the original (admittedly fragile) approach still works. Also, for POP-SS, you could always force emulation by mapping the %rsp address beyond guest physical memory. The hypervisor would then have to emulate the instruction to provide bus-error semantics. > I agree, I think a comment is worthwhile but we can live with the > limitation. I think we can live with the limitation, but I'd really prefer to see a KVM exit with KVM_INTERNAL_ERROR_EMULATION for an instruction that kvm doesn't emulate properly. That seems better than just a comment that the virtual CPU doesn't behave as architected. (I realize that I am probably in the minority here.)
On Wed, Jul 31, 2019 at 01:27:53PM -0700, Jim Mattson wrote: > On Wed, Jul 31, 2019 at 9:37 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > On 31/07/19 15:50, Vitaly Kuznetsov wrote: > > > Jim Mattson <jmattson@google.com> writes: > > > > > >> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > >>> > > >>> Regardless of the way how we skip instruction, interrupt shadow needs to be > > >>> cleared. > > >> > > >> This change is definitely an improvement, but the existing code seems > > >> to assume that we never call skip_emulated_instruction on a > > >> POP-SS/MOV-to-SS/STI. Is that enforced anywhere? > > > > > > (before I send v1 of the series) I looked at the current code and I > > > don't think it is enforced, however, VMX version does the same and > > > honestly I can't think of a situation when we would be doing 'skip' for > > > such an instruction.... and there's nothing we can easily enforce from > > > skip_emulated_instruction() as we have no idea what the instruction > > > is... > > Can't we still coerce kvm into emulating any instruction by leveraging > a stale ITLB entry? The 'emulator' kvm-unit-test did this before the > KVM forced emulation prefix was introduced, but I haven't checked to > see if the original (admittedly fragile) approach still works. Also, > for POP-SS, you could always force emulation by mapping the %rsp > address beyond guest physical memory. The hypervisor would then have > to emulate the instruction to provide bus-error semantics. > > > I agree, I think a comment is worthwhile but we can live with the > > limitation. > > I think we can live with the limitation, but I'd really prefer to see > a KVM exit with KVM_INTERNAL_ERROR_EMULATION for an instruction that > kvm doesn't emulate properly. That seems better than just a comment > that the virtual CPU doesn't behave as architected. (I realize that I > am probably in the minority here.) At a glance, the full emulator models behavior correctly, e.g. see toggle_interruptibility() and setters of ctxt->interruptibility. I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI fast paths as the only (VMX) path that would incorrectly handle a MOV/POP SS. Reading the guest's instruction stream to detect MOV/POP SS would defeat the whole "fast path" thing, not to mention both paths aren't exactly architecturally compliant in the first place.
On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > At a glance, the full emulator models behavior correctly, e.g. see > toggle_interruptibility() and setters of ctxt->interruptibility. > > I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI > fast paths as the only (VMX) path that would incorrectly handle a > MOV/POP SS. Reading the guest's instruction stream to detect MOV/POP SS > would defeat the whole "fast path" thing, not to mention both paths aren't > exactly architecturally compliant in the first place. The proposed patch clears the interrupt shadow in the VMCB on all paths through svm's skip_emulated_instruction. If this happens at the tail end of emulation, it doesn't matter if the full emulator does the right thing.
On Wed, Jul 31, 2019 at 04:45:21PM -0700, Jim Mattson wrote: > On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > At a glance, the full emulator models behavior correctly, e.g. see > > toggle_interruptibility() and setters of ctxt->interruptibility. > > > > I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI > > fast paths as the only (VMX) path that would incorrectly handle a > > MOV/POP SS. Reading the guest's instruction stream to detect MOV/POP SS > > would defeat the whole "fast path" thing, not to mention both paths aren't > > exactly architecturally compliant in the first place. > > The proposed patch clears the interrupt shadow in the VMCB on all > paths through svm's skip_emulated_instruction. If this happens at the > tail end of emulation, it doesn't matter if the full emulator does the > right thing. Unless I'm missing something, skip_emulated_instruction() isn't called in the emulation case, x86_emulate_instruction() updates %rip directly, e.g.: if (writeback) { unsigned long rflags = kvm_x86_ops->get_rflags(vcpu); toggle_interruptibility(vcpu, ctxt->interruptibility); vcpu->arch.emulate_regs_need_sync_to_vcpu = false; kvm_rip_write(vcpu, ctxt->eip); if (r == EMULATE_DONE && ctxt->tf) kvm_vcpu_do_singlestep(vcpu, &r); if (!ctxt->have_exception || exception_type(ctxt->exception.vector) == EXCPT_TRAP) __kvm_set_rflags(vcpu, ctxt->eflags); /* * For STI, interrupts are shadowed; so KVM_REQ_EVENT will * do nothing, and it will be requested again as soon as * the shadow expires. But we still need to check here, * because POPF has no interrupt shadow. */ if (unlikely((ctxt->eflags & ~rflags) & X86_EFLAGS_IF)) kvm_make_request(KVM_REQ_EVENT, vcpu); }
On 01/08/19 01:56, Sean Christopherson wrote: > On Wed, Jul 31, 2019 at 04:45:21PM -0700, Jim Mattson wrote: >> On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson >> <sean.j.christopherson@intel.com> wrote: >> >>> At a glance, the full emulator models behavior correctly, e.g. see >>> toggle_interruptibility() and setters of ctxt->interruptibility. >>> >>> I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI >>> fast paths as the only (VMX) path that would incorrectly handle a >>> MOV/POP SS. Reading the guest's instruction stream to detect MOV/POP SS >>> would defeat the whole "fast path" thing, not to mention both paths aren't >>> exactly architecturally compliant in the first place. >> >> The proposed patch clears the interrupt shadow in the VMCB on all >> paths through svm's skip_emulated_instruction. If this happens at the >> tail end of emulation, it doesn't matter if the full emulator does the >> right thing. > > Unless I'm missing something, skip_emulated_instruction() isn't called in > the emulation case, x86_emulate_instruction() updates %rip directly, e.g.: Indeed. skip_emulated_instruction() is only used when the vmexit code takes care of emulation directly. Paolo > if (writeback) { > unsigned long rflags = kvm_x86_ops->get_rflags(vcpu); > toggle_interruptibility(vcpu, ctxt->interruptibility); > vcpu->arch.emulate_regs_need_sync_to_vcpu = false; > kvm_rip_write(vcpu, ctxt->eip); > if (r == EMULATE_DONE && ctxt->tf) > kvm_vcpu_do_singlestep(vcpu, &r); > if (!ctxt->have_exception || > exception_type(ctxt->exception.vector) == EXCPT_TRAP) > __kvm_set_rflags(vcpu, ctxt->eflags); > > /* > * For STI, interrupts are shadowed; so KVM_REQ_EVENT will > * do nothing, and it will be requested again as soon as > * the shadow expires. But we still need to check here, > * because POPF has no interrupt shadow. > */ > if (unlikely((ctxt->eflags & ~rflags) & X86_EFLAGS_IF)) > kvm_make_request(KVM_REQ_EVENT, vcpu); > } >
On Wed, Jul 31, 2019 at 5:13 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 01/08/19 01:56, Sean Christopherson wrote: > > On Wed, Jul 31, 2019 at 04:45:21PM -0700, Jim Mattson wrote: > >> On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson > >> <sean.j.christopherson@intel.com> wrote: > >> > >>> At a glance, the full emulator models behavior correctly, e.g. see > >>> toggle_interruptibility() and setters of ctxt->interruptibility. > >>> > >>> I'm pretty sure that leaves the EPT misconfig MMIO and APIC access EOI > >>> fast paths as the only (VMX) path that would incorrectly handle a > >>> MOV/POP SS. Reading the guest's instruction stream to detect MOV/POP SS > >>> would defeat the whole "fast path" thing, not to mention both paths aren't > >>> exactly architecturally compliant in the first place. > >> > >> The proposed patch clears the interrupt shadow in the VMCB on all > >> paths through svm's skip_emulated_instruction. If this happens at the > >> tail end of emulation, it doesn't matter if the full emulator does the > >> right thing. > > > > Unless I'm missing something, skip_emulated_instruction() isn't called in > > the emulation case, x86_emulate_instruction() updates %rip directly, e.g.: > > Indeed. skip_emulated_instruction() is only used when the vmexit code > takes care of emulation directly. Mea culpa. I had incorrectly assumed that "skip_emulated_instruction" was used when an instruction was emulated. I retract my objection. Having now been twice bitten by misleading function names, I'll be more careful in the future.
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 68f1f0218c95..f980fc43372d 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -783,13 +783,15 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu) EMULATE_DONE) pr_err_once("KVM: %s: unable to skip instruction\n", __func__); - return; + goto clear_int_shadow; } if (svm->next_rip - kvm_rip_read(vcpu) > MAX_INST_SIZE) printk(KERN_ERR "%s: ip 0x%lx next 0x%llx\n", __func__, kvm_rip_read(vcpu), svm->next_rip); kvm_rip_write(vcpu, svm->next_rip); + +clear_int_shadow: svm_set_interrupt_shadow(vcpu, 0); }
Regardless of the way how we skip instruction, interrupt shadow needs to be cleared. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/svm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)