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 <namit@xxxxxxxxxxxxxxxxx> wrote:
> From: Nadav Amit <nadav.amit@xxxxxxxxx>
>
> 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 <andreyisakov7@xxxxxxxxx>
> Signed-off-by: Rami Burstein <ramibnet@xxxxxxxxx>
> Signed-off-by: Nadav Amit <nadav.amit@xxxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
