Regardless of the way how we skip instruction, interrupt shadow needs to be
cleared.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kvm/svm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
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);
}
--
2.20.1
On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <[email protected]> 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 <[email protected]>
Jim Mattson <[email protected]> writes:
> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <[email protected]> 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.
--
Vitaly
Jim Mattson <[email protected]> writes:
> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <[email protected]> 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 :-)
--
Vitaly
On 31/07/19 15:50, Vitaly Kuznetsov wrote:
> Jim Mattson <[email protected]> writes:
>
>> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <[email protected]> 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 <[email protected]> wrote:
>
> On 31/07/19 15:50, Vitaly Kuznetsov wrote:
> > Jim Mattson <[email protected]> writes:
> >
> >> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <[email protected]> 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 04:45:21PM -0700, Jim Mattson wrote:
> On Wed, Jul 31, 2019 at 4:37 PM Sean Christopherson
> <[email protected]> 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
>> <[email protected]> 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 <[email protected]> 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
> >> <[email protected]> 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.
On Wed, Jul 31, 2019 at 01:27:53PM -0700, Jim Mattson wrote:
> On Wed, Jul 31, 2019 at 9:37 AM Paolo Bonzini <[email protected]> wrote:
> >
> > On 31/07/19 15:50, Vitaly Kuznetsov wrote:
> > > Jim Mattson <[email protected]> writes:
> > >
> > >> On Thu, Jun 20, 2019 at 4:02 AM Vitaly Kuznetsov <[email protected]> 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
<[email protected]> 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.