Avi Kivity wrote: > Jan Kiszka wrote: >> When single-stepping, we have to ensure that the INT1 can make it >> through even if the guest itself is uninterruptible due to MOV SS or >> STI. VMENTRY will fail otherwise. >> >> Signed-off-by: Jan Kiszka <[email protected]> >> --- >> >> arch/x86/kvm/vmx.c | 10 ++++++++-- >> 1 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 3a422dc..8e83102 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -1010,6 +1010,7 @@ static void vmx_cache_reg(struct kvm_vcpu *vcpu, >> enum kvm_reg reg) >> static int set_guest_debug(struct kvm_vcpu *vcpu, struct >> kvm_guest_debug *dbg) >> { >> int old_debug = vcpu->guest_debug; >> + u32 interruptibility; >> unsigned long flags; >> >> vcpu->guest_debug = dbg->control; >> @@ -1017,9 +1018,14 @@ static int set_guest_debug(struct kvm_vcpu >> *vcpu, struct kvm_guest_debug *dbg) >> vcpu->guest_debug = 0; >> >> flags = vmcs_readl(GUEST_RFLAGS); >> - if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) >> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >> flags |= X86_EFLAGS_TF | X86_EFLAGS_RF; >> - else if (old_debug & KVM_GUESTDBG_SINGLESTEP) >> + /* We must be interruptible when single-stepping */ >> + interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO); >> + if (interruptibility & 3) >> + vmcs_write32(GUEST_INTERRUPTIBILITY_INFO, >> + interruptibility & ~3); >> > > Could just write unconditionally - it's not like the write has any > effect on speed. vmcs_clear_bits() will do it cleanly. > > But I'm worried about correctness. Suppose we're singlestepping a sti; > hlt sequence. While we'll clear interruptibility, probably receive the > debug trap (since that's a high priority exception), but then inject the > interrupt before the hlt, hanging the guest. So we probably need to > restore interruptibility on exit. >
There was some issue with the original patch, but I think I have a safe version now that also works as good as the old one. Please see below, including comments. I'm still open to further concerns or better approaches. Sheng, maybe you can provide some more details on how one is supposed to handle this hairy case with VMX. > This looks like a good candidate for a test case. > This will be more complicated as I'm currently able to handle: kvmctl would have to be extended to interact with the guest debug interface of kvm, setting appropriate breakpoints and handling the callbacks. Jan -----------> When single-stepping over STI and MOV SS, we must clear the corresponding interruptibility bits in the guest state. Otherwise vmentry fails as it then expects bit 14 (BS) in pending debug exceptions being set, but that's not correct for the guest debugging case. Note that clearing those bits is safe as we check for interruptibility based on the original state and do not inject interrupts or NMIs if guest interruptibility was blocked. Signed-off-by: Jan Kiszka <[email protected]> --- arch/x86/kvm/vmx.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ec37635..26f732c 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2477,6 +2477,11 @@ static void do_interrupt_requests(struct kvm_vcpu *vcpu, { vmx_update_window_states(vcpu); + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) + vmcs_clear_bits(GUEST_INTERRUPTIBILITY_INFO, + GUEST_INTR_STATE_STI | + GUEST_INTR_STATE_MOV_SS); + if (vcpu->arch.nmi_pending && !vcpu->arch.nmi_injected) { if (vcpu->arch.interrupt.pending) { enable_nmi_window(vcpu); @@ -3263,6 +3268,11 @@ static void vmx_intr_assist(struct kvm_vcpu *vcpu) vmx_update_window_states(vcpu); + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) + vmcs_clear_bits(GUEST_INTERRUPTIBILITY_INFO, + GUEST_INTR_STATE_STI | + GUEST_INTR_STATE_MOV_SS); + if (vcpu->arch.nmi_pending && !vcpu->arch.nmi_injected) { if (vcpu->arch.interrupt.pending) { enable_nmi_window(vcpu); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
