Looking at the patch again, there two points you may want to change, see inline.
If you want, I’ll send a v2. Nadav Nadav Amit <[email protected]> wrote: > From: Nadav Amit <[email protected]> > > This adds support for guest data and I/O breakpoints during instruction > emulation. > > Watchpoints are examined during data and io interceptions: segmented_read, > segmented_write, em_in, em_out, segmented_read_std and kvm_fast_pio_out. > > When such a breakpoint is triggered, trap is reported by DB_VECTOR > exception. > > Signed-off-by: Andrey Isakov <[email protected]> > Signed-off-by: Rami Burstein <[email protected]> > Signed-off-by: Nadav Amit <[email protected]> > --- > arch/x86/include/asm/kvm_emulate.h | 3 ++ > arch/x86/kvm/emulate.c | 32 +++++++++++++++++ > arch/x86/kvm/x86.c | 74 +++++++++++++++++++++++++++++--------- > 3 files changed, 92 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_emulate.h > b/arch/x86/include/asm/kvm_emulate.h > index e16466e..f6d5d6c 100644 > --- a/arch/x86/include/asm/kvm_emulate.h > +++ b/arch/x86/include/asm/kvm_emulate.h > @@ -211,6 +211,9 @@ struct x86_emulate_ops { > void (*get_cpuid)(struct x86_emulate_ctxt *ctxt, > u32 *eax, u32 *ebx, u32 *ecx, u32 *edx); > void (*set_nmi_mask)(struct x86_emulate_ctxt *ctxt, bool masked); > + bool (*check_watchpoint)(struct x86_emulate_ctxt *ctxt, > + unsigned long addr, unsigned int length, > + int type); > }; > > typedef u32 __attribute__((vector_size(16))) sse128_t; > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index b372a75..4e91b7b 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -24,6 +24,7 @@ > #include "kvm_cache_regs.h" > #include <linux/module.h> > #include <asm/kvm_emulate.h> > +#include <asm/debugreg.h> > #include <linux/stringify.h> > #include <asm/debugreg.h> > > @@ -564,6 +565,17 @@ static int emulate_db(struct x86_emulate_ctxt *ctxt) > return emulate_exception(ctxt, DB_VECTOR, 0, false); > } > > +static void emulate_db_trap(struct x86_emulate_ctxt *ctxt) > +{ > + /* > + * If a fault is later encountered, the exception information will be > + * overridden. Otherwise the trap would be handled after the emulation > + * is completed. > + */ > + (void)emulate_exception(ctxt, DB_VECTOR, 0, false); > + ctxt->have_exception = true; > +} > + > static int emulate_gp(struct x86_emulate_ctxt *ctxt, int err) > { > return emulate_exception(ctxt, GP_VECTOR, err, true); > @@ -776,6 +788,10 @@ static int segmented_read_std(struct x86_emulate_ctxt > *ctxt, > rc = linearize(ctxt, addr, size, false, &linear); > if (rc != X86EMUL_CONTINUE) > return rc; > + > + if (ctxt->ops->check_watchpoint(ctxt, linear, size, DR_RW_READ)) > + emulate_db_trap(ctxt); > + > return ctxt->ops->read_std(ctxt, linear, data, size, &ctxt->exception); > } > > @@ -1369,6 +1385,10 @@ static int segmented_read(struct x86_emulate_ctxt > *ctxt, > rc = linearize(ctxt, addr, size, false, &linear); > if (rc != X86EMUL_CONTINUE) > return rc; > + > + if (ctxt->ops->check_watchpoint(ctxt, linear, size, DR_RW_READ)) > + emulate_db_trap(ctxt); > + > return read_emulated(ctxt, linear, data, size); > } > > @@ -1383,6 +1403,10 @@ static int segmented_write(struct x86_emulate_ctxt > *ctxt, > rc = linearize(ctxt, addr, size, true, &linear); > if (rc != X86EMUL_CONTINUE) > return rc; > + > + if (ctxt->ops->check_watchpoint(ctxt, linear, size, DR_RW_WRITE)) > + emulate_db_trap(ctxt); > + > return ctxt->ops->write_emulated(ctxt, linear, data, size, > &ctxt->exception); > } > @@ -3729,11 +3753,19 @@ static int em_in(struct x86_emulate_ctxt *ctxt) > &ctxt->dst.val)) > return X86EMUL_IO_NEEDED; > > + if (ctxt->ops->check_watchpoint(ctxt, ctxt->src.val, ctxt->dst.bytes, > + DR_RW_PORT)) > + emulate_db_trap(ctxt); > + > return X86EMUL_CONTINUE; > } > > static int em_out(struct x86_emulate_ctxt *ctxt) > { > + if (ctxt->ops->check_watchpoint(ctxt, ctxt->dst.val, ctxt->src.bytes, > + DR_RW_PORT)) > + emulate_db_trap(ctxt); > + > ctxt->ops->pio_out_emulated(ctxt, ctxt->src.bytes, ctxt->dst.val, > &ctxt->src.val, 1); > /* Disable writeback. */ > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3e4d032..ba75f76 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4831,6 +4831,55 @@ static void emulator_set_nmi_mask(struct > x86_emulate_ctxt *ctxt, bool masked) > kvm_x86_ops->set_nmi_mask(emul_to_vcpu(ctxt), masked); > } > > +static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 len, > + u32 dr7, unsigned long *db) > +{ > + u32 dr6 = 0; > + int i; > + u32 enable, dr7_rw, dr7_len; > + unsigned long align_db; > + > + enable = dr7; > + dr7_rw = dr7 >> 16; > + dr7_len = dr7_rw >> 2; > + for (i = 0; i < 4; i++, enable >>= 2, dr7_rw >>= 4, dr7_len >>= 4) { > + if (!(enable & 3)) > + continue; > + > + /* Check type matches, but on writes, check read bp */ > + if (((dr7_rw & 3) != type && > + !((dr7_rw & 3) == DR_RW_READ && type == DR_RW_WRITE))) > + continue; > + align_db = db[i] & ~((unsigned long)dr7_len & 3); > + if (addr < (align_db + (dr7_len & 3) + 1) && > + align_db < (addr + len)) > + dr6 |= (1 << i); > + } > + return dr6; > +} > + > +static bool emulator_check_watchpoint(struct x86_emulate_ctxt *ctxt, > + unsigned long addr, unsigned int length, > + int type) > +{ > + u32 dr6; > + struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); > + > + if (likely(!(vcpu->arch.dr7 & DR7_BP_EN_MASK)) || > + (type == DR_RW_PORT && !(emulator_get_cr(ctxt, 4) & X86_CR4_DE))) You may want to call kvm_get_cr4 instead of emulator_get_cr . > + return false; > + > + dr6 = kvm_vcpu_check_hw_bp(addr, type, length, vcpu->arch.dr7, > + vcpu->arch.db); > + > + if (dr6 != 0) { > + __kvm_set_dr(vcpu, DR_STATUS, > + (vcpu->arch.dr6 & ~DR_TRAP_BITS) | dr6); I think there is no need to mask DR_TRAP_BIT. It may cause a problem if we have multiple breakpoints on the same instruction. > + return true; > + } > + return false; > +} > + > static const struct x86_emulate_ops emulate_ops = { > .read_gpr = emulator_read_gpr, > .write_gpr = emulator_write_gpr, > @@ -4869,6 +4918,7 @@ static const struct x86_emulate_ops emulate_ops = { > .intercept = emulator_intercept, > .get_cpuid = emulator_get_cpuid, > .set_nmi_mask = emulator_set_nmi_mask, > + .check_watchpoint = emulator_check_watchpoint, > }; > > static void toggle_interruptibility(struct kvm_vcpu *vcpu, u32 mask) > @@ -5118,21 +5168,6 @@ static void kvm_set_hflags(struct kvm_vcpu *vcpu, > unsigned emul_flags) > kvm_smm_changed(vcpu); > } > > -static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7, > - unsigned long *db) > -{ > - u32 dr6 = 0; > - int i; > - u32 enable, rwlen; > - > - enable = dr7; > - rwlen = dr7 >> 16; > - for (i = 0; i < 4; i++, enable >>= 2, rwlen >>= 4) > - if ((enable & 3) && (rwlen & 15) == type && db[i] == addr) > - dr6 |= (1 << i); > - return dr6; > -} > - > static void kvm_vcpu_check_singlestep(struct kvm_vcpu *vcpu, unsigned long > rflags, int *r) > { > struct kvm_run *kvm_run = vcpu->run; > @@ -5173,7 +5208,7 @@ static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu > *vcpu, int *r) > (vcpu->arch.guest_debug_dr7 & DR7_BP_EN_MASK)) { > struct kvm_run *kvm_run = vcpu->run; > unsigned long eip = kvm_get_linear_rip(vcpu); > - u32 dr6 = kvm_vcpu_check_hw_bp(eip, 0, > + u32 dr6 = kvm_vcpu_check_hw_bp(eip, DR_RW_EXECUTE, 1, > vcpu->arch.guest_debug_dr7, > vcpu->arch.eff_db); > > @@ -5190,7 +5225,7 @@ static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu > *vcpu, int *r) > if (unlikely(vcpu->arch.dr7 & DR7_BP_EN_MASK) && > !(kvm_get_rflags(vcpu) & X86_EFLAGS_RF)) { > unsigned long eip = kvm_get_linear_rip(vcpu); > - u32 dr6 = kvm_vcpu_check_hw_bp(eip, 0, > + u32 dr6 = kvm_vcpu_check_hw_bp(eip, DR_RW_EXECUTE, 1, > vcpu->arch.dr7, > vcpu->arch.db); > > @@ -5346,6 +5381,11 @@ int kvm_fast_pio_out(struct kvm_vcpu *vcpu, int size, > unsigned short port) > unsigned long val = kvm_register_read(vcpu, VCPU_REGS_RAX); > int ret = emulator_pio_out_emulated(&vcpu->arch.emulate_ctxt, > size, port, &val, 1); > + > + if (emulator_check_watchpoint(&vcpu->arch.emulate_ctxt, port, size, > + DR_RW_PORT)) > + kvm_queue_exception(vcpu, DB_VECTOR); > + > /* do not return to emulator after return from userspace */ > vcpu->arch.pio.count = 0; > return ret; > -- > 2.1.4 -- 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
