On Mon, Feb 22, 2010 at 06:51:23PM +0100, Jan Kiszka wrote:
> Support both guest- as well as host-owned EFLAGS.TF while emulating
> instructions. For guest-owned TF, we simply inject DB and update DR6.BS
> after completing an instruction that has TF set on entry. To support
> guest single-stepping under host control, we store the pending step
> along with its CS and RIP and trigger a corresponding user space exit
> once guest execution is about to resume. This check is is also required
> in the VMX emulation loop during invalid guest states.
>
Emulator currently is a total mess. It is not a good time to add more mess
there right now IMO.
> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> ---
> arch/x86/include/asm/kvm_host.h | 5 +++
> arch/x86/kvm/vmx.c | 6 +++
> arch/x86/kvm/x86.c | 65 ++++++++++++++++++++++++++++++++++-----
> 3 files changed, 68 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index d46e791..d69d8aa 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -362,8 +362,11 @@ struct kvm_vcpu_arch {
> u64 *mce_banks;
>
> /* used for guest single stepping over the given code position */
> + bool singlestep_pending;
> u16 singlestep_cs;
> + u16 singlestep_pending_cs;
> unsigned long singlestep_rip;
> + unsigned long singlestep_pending_rip;
If we are going to have many of those rip/cs pairs may be it is better
to add structure linear_ip and have functions is_same_ip().
> /* fields used by HYPER-V emulation */
> u64 hv_vapic;
> };
> @@ -820,4 +823,6 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
> void kvm_define_shared_msr(unsigned index, u32 msr);
> void kvm_set_shared_msr(unsigned index, u64 val, u64 mask);
>
> +int kvm_check_guest_singlestep(struct kvm_vcpu *vcpu);
> +
> #endif /* _ASM_X86_KVM_HOST_H */
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index d772476..317828f 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3489,6 +3489,12 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
> goto out;
> if (need_resched())
> schedule();
> +
> + if (unlikely(vcpu->arch.singlestep_pending)) {
> + ret = kvm_check_guest_singlestep(vcpu);
> + if (ret == 0)
> + goto out;
> + }
> }
>
> vmx->emulation_required = 0;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 19e8b28..6ebebb9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3441,6 +3441,27 @@ static void cache_all_regs(struct kvm_vcpu *vcpu)
> vcpu->arch.regs_dirty = ~0;
> }
>
> +static u16 get_segment_selector(struct kvm_vcpu *vcpu, int seg)
> +{
> + struct kvm_segment kvm_seg;
> +
> + kvm_get_segment(vcpu, &kvm_seg, seg);
> + return kvm_seg.selector;
> +}
> +
> +static void queue_singlestep(struct kvm_vcpu *vcpu)
> +{
> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> + vcpu->arch.singlestep_pending = true;
> + vcpu->arch.singlestep_pending_cs =
> + get_segment_selector(vcpu, VCPU_SREG_CS);
> + vcpu->arch.singlestep_pending_rip = kvm_rip_read(vcpu);
Why should we remember rip where TF happened? We should exit
immediately to userspace anyway, no?
> + } else {
> + vcpu->arch.dr6 |= DR6_BS;
> + kvm_queue_exception(vcpu, DB_VECTOR);
What if instruction emulation generated fault?
> + }
> +}
> +
> int emulate_instruction(struct kvm_vcpu *vcpu,
> unsigned long cr2,
> u16 error_code,
> @@ -3449,6 +3470,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
> int r, shadow_mask;
> struct decode_cache *c;
> struct kvm_run *run = vcpu->run;
> + bool singlestep;
>
> kvm_clear_exception_queue(vcpu);
> vcpu->arch.mmio_fault_cr2 = cr2;
> @@ -3515,8 +3537,12 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
> }
> }
>
> + singlestep = vcpu->arch.emulate_ctxt.eflags & X86_EFLAGS_TF;
> +
> if (emulation_type & EMULTYPE_SKIP) {
> kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.decode.eip);
> + if (singlestep)
> + queue_singlestep(vcpu);
Instruction that wasn't emulated shouldn't generate faults.
> return EMULATE_DONE;
> }
>
> @@ -3549,6 +3575,9 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>
> kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
>
> + if (singlestep)
> + queue_singlestep(vcpu);
> +
if vcpu->mmio_is_write == true we can still exit with DO_MMIO, so
instruction is not yes completely executed. This queue_singlestep
mechanism looks bogus anyway. emulate_instruction() caller should
initiate exit to userspace space if required.
> if (vcpu->mmio_is_write) {
> vcpu->mmio_needed = 0;
> return EMULATE_DO_MMIO;
> @@ -4450,6 +4479,26 @@ out:
> return r;
> }
>
> +int kvm_check_guest_singlestep(struct kvm_vcpu *vcpu)
> +{
> + unsigned long rip = kvm_rip_read(vcpu);
> +
> + vcpu->arch.singlestep_pending = false;
> +
> + if (vcpu->arch.singlestep_pending_cs !=
> + get_segment_selector(vcpu, VCPU_SREG_CS) ||
> + vcpu->arch.singlestep_pending_rip != rip)
> + return 1;
> +
Again how this check can be false?
> + vcpu->run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1;
> + vcpu->run->debug.arch.dr7 = 0;
> + vcpu->run->exit_reason = KVM_EXIT_DEBUG;
> + vcpu->run->debug.arch.pc = get_segment_base(vcpu, VCPU_SREG_CS) + rip;
> + vcpu->run->debug.arch.exception = DB_VECTOR;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_check_guest_singlestep);
>
> static int __vcpu_run(struct kvm_vcpu *vcpu)
> {
> @@ -4471,6 +4520,12 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>
> r = 1;
> while (r > 0) {
> + if (unlikely(vcpu->arch.singlestep_pending)) {
> + r = kvm_check_guest_singlestep(vcpu);
> + if (r == 0)
> + break;
> + }
> +
Why not use existing mechanism to cause run loop to exit to userspace
i.e return 0 from vcpu_enter_guest(), instead of adding special cases here?
> if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE)
> r = vcpu_enter_guest(vcpu);
> else {
> @@ -4828,14 +4883,6 @@ static gpa_t get_tss_base_addr_read(struct kvm_vcpu *vcpu,
> return kvm_mmu_gva_to_gpa_read(vcpu, base_addr, NULL);
> }
>
> -static u16 get_segment_selector(struct kvm_vcpu *vcpu, int seg)
> -{
> - struct kvm_segment kvm_seg;
> -
> - kvm_get_segment(vcpu, &kvm_seg, seg);
> - return kvm_seg.selector;
> -}
> -
> static int kvm_load_realmode_segment(struct kvm_vcpu *vcpu, u16 selector, int seg)
> {
> struct kvm_segment segvar = {
> @@ -5607,6 +5654,8 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
> vcpu->arch.dr6 = DR6_FIXED_1;
> vcpu->arch.dr7 = DR7_FIXED_1;
>
> + vcpu->arch.singlestep_pending = false;
> +
> return kvm_x86_ops->vcpu_reset(vcpu);
> }
>
> --
> 1.6.0.2
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
